Bug 369039 - Checking 'nullable' annotation for overridden elements could be more relax
Summary: Checking 'nullable' annotation for overridden elements could be more relax
Status: VERIFIED WORKSFORME
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.8   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.8 M5   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-19 03:59 EST by Egidijus Vaisnora CLA
Modified: 2012-01-23 00:44 EST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Egidijus Vaisnora CLA 2012-01-19 03:59:18 EST
It is all about 3.8 M4 Null analysis feature. I had intention to convert my project to one, which is 'null' annotation aware by making default "Use non-null as project-wide default" (it is very useful feature). 
However I stuck with plenty of errors generated because of overridden "legacy" code, which do not use 'not-null' annotations. 
Solutions are 
- to change "legacy" code supplying with annotation. It is not acceptable, because part of overridden code is not developed by me
- to put everywhere annotation 'nullable'. It is not acceptable either. There are about 270 errors for single project.

Personally I do not see the reason, why such strict checking is required. But
my proposal is to have message option for overridden 'legacy' code with possibillty to choose "error/warning/ignore" message.
Comment 1 Srikanth Sankaran CLA 2012-01-19 04:38:30 EST
Stephan, please take a look.
Comment 2 Stephan Herrmann CLA 2012-01-19 06:53:31 EST
(In reply to comment #0)
> It is all about 3.8 M4 Null analysis feature.

Thanks for trying this feature.

> I had intention to convert my
> project to one, which is 'null' annotation aware by making default "Use
> non-null as project-wide default" (it is very useful feature). 
> However I stuck with plenty of errors generated because of overridden "legacy"
> code, which do not use 'not-null' annotations. 
> Solutions are 
> - to change "legacy" code supplying with annotation. It is not acceptable,
> because part of overridden code is not developed by me
> - to put everywhere annotation 'nullable'. It is not acceptable either. There
> are about 270 errors for single project.

We have one mechanism in place for this situation and another one in the development pipeline:

- As of 3.8 M4 you can declare any class that subclasses a legacy class with
  @NonNullByDefault(false)
  The effect of this declaration is to *cancel* the surrounding default to 
  the effect that types in signatures of this class are by default 
  *unspecified* wrt nullness.

- For the future we plan (in bug 331651) to provide a mechanism by which
  you'll be able to provide a separate file for such legacy libraries in
  which you can specify the null contracts of that library, so, e.g., if
  methods in that library de-facto already ensure non-null results then
  you can externally add a @NonNull to this method and leverage this
  specification in your code.

 
> Personally I do not see the reason, why such strict checking is required.

We want our analysis to be exact. 
If it says OK, the code should indeed be OK.
If we silently allow, e.g., overriding a legacy method with one that 
specifies @NonNull parameters, calls referring to the super class can 
legally pass null. Dynamic binding will cause such null to be passed
into your @NonNull parameter and thus the guarantees we were trying to
give are vain.

> But my proposal is to have message option for overridden 'legacy' code with
> possibillty to choose "error/warning/ignore" message.

Please let me know whether you think @NonNullByDefault(false) can be used
to serve your purpose until we have bug 331651.
Comment 3 Egidijus Vaisnora CLA 2012-01-19 10:20:03 EST
(In reply to comment #2)

Thank you for quick respond. 

> We want our analysis to be exact. 
> If it says OK, the code should indeed be OK.
> If we silently allow, e.g., overriding a legacy method with one that 
> specifies @NonNull parameters, calls referring to the super class can 
> legally pass null. Dynamic binding will cause such null to be passed
> into your @NonNull parameter and thus the guarantees we were trying to
> give are vain.

Hmmm... it explains me the problem. 

> > But my proposal is to have message option for overridden 'legacy' code with
> > possibillty to choose "error/warning/ignore" message.
> 
> Please let me know whether you think @NonNullByDefault(false) can be used
> to serve your purpose until we have bug 331651.

I am using FindBug annotations. What is FindBug replacement for the "ParametersAreNonnullByDefault(false)"?
If I correctly understood, 331651 fix will allow to provide explicit annotations for already built jar's or it will be some kind of filter?
Comment 4 Stephan Herrmann CLA 2012-01-19 12:36:57 EST
(In reply to comment #3)
> > > But my proposal is to have message option for overridden 'legacy' code with
> > > possibillty to choose "error/warning/ignore" message.
> > 
> > Please let me know whether you think @NonNullByDefault(false) can be used
> > to serve your purpose until we have bug 331651.
> 
> I am using FindBug annotations.

In that case please be aware that FindBugs' "Nullable" is different from
ours (i.e., it has no precise definition), our Nullable corresponds to
FindBugs' CheckForNull.

> What is FindBug replacement for the
> "ParametersAreNonnullByDefault(false)"?

Oops, I don't think "ParametersAreNonnullByDefault(false)" exists.

JDT supports @NonNullByDefault(false), whereas FindBugs only supports
ParametersAreNonnullByDefault (without a parameter), right?
But since FindBugs won't see the global default configured for the JDT
leaving subclasses of legacy classes just unannotated should be the
best approximation, no?

> If I correctly understood, 331651 fix will allow to provide explicit
> annotations for already built jar's or it will be some kind of filter?

The former.
Comment 5 Stephan Herrmann CLA 2012-01-19 16:18:07 EST
This bug has digressed into discussing FindBugs, so let's try to focus
on JDT again:

I'm not in favor of the original proposal because it would significantly undermine the analysis: we would give guarantees backed by obligations that can easily by-passed. By specifying, e.g., a method parameter as @NonNull we can relieve the implementation from checking that parameter against null *because* we can ensure that no client will pass null. If a client can pass null via a legacy super class that guarantee is broken (and you will blame our analysis :) ).

(In reply to comment #2)
> Please let me know whether you think @NonNullByDefault(false) can be used
> to serve your purpose until we have bug 331651.

I believe this is best solution we can offer for subclasses of legacy classes. Therefore I'm closing as WORKSFORME. Feel free to open with more specific details, if you see issues that cannot be solved using this approach.