Comment 1 Stephan Herrmann 2011-12-05 15:18:19 EST

See also bug 365387 comment 27:

> (37) Should this produce a redundant annotation message ? 
> 
> import org.eclipse.jdt.annotation.NonNull;
> import org.eclipse.jdt.annotation.NonNullByDefault;
> import org.eclipse.jdt.annotation.Nullable;
> 
> @NonNullByDefault
> class X {
>     @NonNullByDefault Object foo( Object o) {
>         return new Object();
>     }
> }

Answer: yes.

Comment 2 Ayushman Jain 2011-12-06 00:20:44 EST

(In reply to comment #0)
> 
> is legal Java but makes no sense for null analysis.
> The compiler should give an appropriate warning here.

What I really meant in bug 365387#c28 was that the diagnostic should only appear if null analysis is *enabled*, not otherwise, no?

Comment 3 Srikanth Sankaran 2011-12-06 00:34:02 EST

(In reply to comment #2)
> (In reply to comment #0)
> > 
> > is legal Java but makes no sense for null analysis.
> > The compiler should give an appropriate warning here.
> 
> What I really meant in bug 365387#c28 was that the diagnostic should only
> appear if null analysis is *enabled*, not otherwise, no?

The slightly long answer is : The error should come from the "annotation
processor" and not from the compiler per se. Here the compiler doubles as
the null annotation processor (only) when the master switch in on, so yes
a diagnostic should be produced only when null analysis is enabled via
annotations. Purely from a compiler view point, there is nothing wrong
with the program.

Comment 4 Ayushman Jain 2011-12-06 09:18:51 EST

When documenting the new warning, please also update the doc with following comments, captured from https://bugs.eclipse.org/bugs/show_bug.cgi?id=364820#c24
> A few comments regarding ref-preferences-errors.htm:
> 
> - the entry for "Potential null pointer access" mentions the limitations
>   of the analysis - we might want to add a reference to null annotations here.
>   E.g.:
>   "The quality of the analysis can be improved by using null-annotations,
>   which can be enabled using the option 
>   <b>Enable annotation-based null analysis</b>."
> 
> - entry "Use non-null as workspace-wide default" should account for the
>   fact that the label changes to ".. project-wide default", when 
>   configuring per-project preferences
>   - watch out for the citation in the "Redundnant null annotation" entry.

Comment 5 Stephan Herrmann 2011-12-06 09:36:34 EST

While we're at it: more doc-update to do:
 
(In reply to bug 364820 comment #19)
> Created attachment 207968 [details]
> jdt api options file

I found another problem:
inside the entry "Reporting Redundant Null Annotations" there's a stale
copy of "Insufficient nullness information is usually a consequence of using
other unannotated variables or methods."
To be removed.

Comment 6 Srikanth Sankaran 2011-12-20 03:37:06 EST

(In reply to comment #1)
> See also bug 365387 comment 27:
> 
> > (37) Should this produce a redundant annotation message ? 

> Answer: yes.

One  more test case:

import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;

@SuppressWarnings("unused")
public class X {
    @NonNullByDefault 
    public void foo(String [] args) {
    	@NonNullByDefault
        class local {
            class Deeply {
                Object zoo() {
                    return new Object();
                }
            }
        };
    }
}

Comment 7 Stephan Herrmann 2012-01-19 15:25:29 EST

Created attachment 209776 [details]
Tests & fix

Straight-forward patch under test.

Comment 8 Stephan Herrmann 2012-01-19 16:40:36 EST

Patch was successfully tested (after rebasing on HEAD).

In case s.o. was going to ask: none of the new checks will be performed unless annotation based null analysis is enabled (I specifically added a test to verify that the new diagnostics are silent when the master switch is off).

Under this premise I chose not to add further configuration options, so redundant defaults will always be flagged by a warning, and contradictory annotations will be flagged as errors (since there's no sound interpretation). Please let me know if you feel that these should be separately configurable.

As a consequence of the above, the patch contains neither API nor doc changes.

Comment 9 Stephan Herrmann 2012-01-19 16:53:58 EST

Released for 3.8 M5 via commit 4fd98abe69a6425880abe243fa431e365710bef2

Comment 10 Srikanth Sankaran 2012-01-19 17:06:20 EST

(In reply to comment #8)

> As a consequence of the above, the patch contains neither API nor doc changes.

Are the doc changes called out in comment#4 and comment#5 still valid ?

Comment 11 Ayushman Jain 2012-01-20 10:13:29 EST

(In reply to comment #10)
> (In reply to comment #8)
> 
> > As a consequence of the above, the patch contains neither API nor doc changes.
> 
> Are the doc changes called out in comment#4 and comment#5 still valid ?

To be fixed with bug 369246

Comment 12 Srikanth Sankaran 2012-01-24 16:31:13 EST

Verified for 3.8 M5 using build id: I20120122-2000