Bug 479389 - [null] warn when annotation based null analysis is enabled but null annotations are not on the classpath
Summary: [null] warn when annotation based null analysis is enabled but null annotatio...
Status: RESOLVED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.11   Edit
Hardware: All All
: P3 normal with 3 votes (vote)
Target Milestone: 4.15 M1   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-09 03:38 EDT by Stephan Herrmann CLA
Modified: 2020-05-11 06:15 EDT (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Herrmann CLA 2015-10-09 03:38:53 EDT
From bug 366014 comment 9:

Assume you have a project
- configured for 1.8
- with annotation based null analysis enabled
- with no annotation types on the build path

Then compile the following snippet:

results in
"The type org.eclipse.jdt.annotation.NonNull cannot be resolved. It is indirectly referenced from required .class files"
Comment 1 Stephan Herrmann CLA 2015-10-09 03:46:02 EDT
And here's the snippet:
  (new ArrayList<>()).stream().toArray(String[]::new);

My guess is: the use of "new" leads the compiler to implicitly marking this expression's type as @NonNull. For this we use whatever annotation types have been configured for the project.

However, if this implicit use is the first use of a missing annotation type, the above error message is not optimal. We could perhaps turn this into a more specific error, like "... uses null annotations, but null annotations cannot be found on the build path ...".

Then, this error should offer the existing quickfix to add the annotation bundle to the build path.
Comment 2 Andrey Loskutov CLA 2015-10-09 09:57:26 EDT
From users point of view the JDT preferences UI should check if the configured annotations are available and either automatically pop up a question "you need the library, should we add it to the classpath", and if not, should add a project error marker that the required annotation libraries aren't properly configured. So the error should be shown even before the first use of the library in the compiler. This way users would have immediate feedback that the changes they did on the project setup are incomplete.
Comment 3 Stephan Herrmann CLA 2015-10-12 17:47:57 EDT
Hi Andrey,

(In reply to Andrey Loskutov from comment #2)
> From users point of view the JDT preferences UI should ...

That would be a JDT/UI RFE then. Some follow-up from bug 364815, bug 366013 and bug 365775. 

Well, that bug may already exist: bug 365764, so let's please continue the UI-related discussion there, OK?

I'll keep this bug in JDT/Core to sanitize the current compiler behavior.
Comment 4 Stephan Herrmann CLA 2015-10-12 18:03:11 EDT
(In reply to Stephan Herrmann from comment #3)
> Well, that bug may already exist: bug 365764, so let's please continue the
> UI-related discussion there, OK?

Oops, that bug is a JDT/Core bug, too. So let's try to find out first, if a specific UI-bug is needed.

Here's why we haven't yet implemented the validation part: it is not clear, what should trigger the validation?

- In particular, when null annotations are configured per workspace there simply isn't a trigger for validation.

- The compiler itself doesn't know where it gets its options from, so it has no location to flag problems against.

- For per-project configuration we *could* handle it similar to build path errors.


For a moment I was thinking, yes, maybe the preference dialog should perform the validation, but then the subsequent workflow is unclear: we don't want to fix the problem within the modal dialog. But when an error marker is created by the preference dialog: who is going to ever delete that marker? If there's no builder responsible for the marker, no builder can remove it. 

Any ideas?
Comment 6 Stephan Herrmann CLA 2016-03-25 10:30:22 EDT
Too much on my plate for 4.6. Bulk deferral to 4.7
Comment 7 Stephan Herrmann CLA 2017-05-16 12:05:22 EDT
Ran out of time for 4.7. Bulk move to 4.8.
Comment 8 Manoj N Palat CLA 2018-05-16 12:56:36 EDT
bulk move out of 4.8
Comment 9 Stephan Herrmann CLA 2018-08-26 10:22:46 EDT
During 4.10 I will investigate the minimal solution: make the original error more specific to tell the user *why* the indirectly referenced type is needed.
Comment 10 Corneliud Dirmeier CLA 2018-11-20 02:52:40 EST
This bug is realy a big problem for our framework. We activated the null annotation handling recently in our framework. Now every project that uses the framework gets an error because we force them to use the same null annotation for example when subclassing our abstract class. Therefor we need to think about deactivating the null annotations again. It feels like nobody realy uses this feature in production. Realy sad.
Comment 11 Stephan Herrmann CLA 2018-11-20 07:52:07 EST
Thanks, Cornelius, for chiming in.

(In reply to Cornelius Dirmeier from comment #10)
> This bug is realy a big problem for our framework. We activated the null
> annotation handling recently in our framework. Now every project that uses
> the framework gets an error because we force them to use the same null
> annotation for example when subclassing our abstract class. Therefor we need
> to think about deactivating the null annotations again. 

This looks related but not identical to the scenario in comment 0.
What exactly is your expectation? Is it this:

- Framework X is developed with annotations Y and analysis enabled.
- Application uses X but annotations Z instead of Y, also analysis enabled.
  Or is the application compiled without any annotation types available?
- Application should see contracts of X in terms of annotations Z?

It is important to distinguish between explicit and implicit contracts:

- foo(@NonNull X x) uses a specific annotation for an explicit contract.

- String[]::new implicitly has a signature like
     String @NonNull[] get(int i)
  and since the contract is implicit, using any annotation type to denote
  the signature is questionable.

For explicit contracts the annotation should be seen as part of the API, hence one could even argue that frameworks using null annotations should re-export those annotation types. Wouldn't that completely avoid your problem?

Or are secondary null annotations the solution for you?

The latest plan in this bug was simply to help the user understand the reason by explaining why the indirectly referenced type is needed, despite not being visible in things like "String[]::new".
Comment 12 Corneliud Dirmeier CLA 2018-11-20 08:03:41 EST
You are right that there are different cases to discuss.

First, and that's why I was answering to this issue, I expect to not get a fatal compilation error when there is any kind of misconfiguration like that. I think a warning would be far enough or if it realy has to be an error than it should not fail compilation - as far I have configured my compiler errors to not be treated as fatal errors. But at the moment I get an unresolved compilation error when ever I want to access the class.

The point is, that the application code which uses the framework have other null annotations configured at the moment. May be it is possible to get it work with the secondary annotations. But still I dictate the properties of the downstream projects. That was not expected when we decided to change use the eclipse-null annotations. It seems like the null analysis interpretes the eclipse-null-annotations even if other null-annotations are configured in the settings. I might need to invetigate this scenario some more.

I do not want to export the dependencies of the null-annotations. Exporting the dependency means (in maven) that the dependency needs to be available at runtime. For example if the application project builds a WAR they will have the null annotations packaged. That is defintively not expected.
Comment 13 Corneliud Dirmeier CLA 2018-11-20 08:41:40 EST
Now we have a realy strange behavior in our downstream projects. Long time ago... we started our framework with activated null-annotation-warnings. That time we wanted to use the JSR305 annotations and configured the javax.annotation.Nonnull annotation. But in an early version we had a type and configured javax.annotation.NonNull (second N caps as the eclipse annotation).

It seems like many projects compied our settings and never had any problems. In our settings we fixed the typo of course because we used the annotations, later found out that it is not very good supported by eclipse null annotations (because wrong @Target), ... finally switched to the eclipse null annotations.

The settings in the other projects were wrong all the time and nobody noticed. But now we released our new version using the eclipse null annotations and BAHM. They get a lot of errors saying "The type javax.annotations.NonNull cannott be resolved. It is indirectly referenced from required .class file".

Why? I know the settings are not correct but why it was no problem all the time until now? Is there any comparator for the unqualified name that leads to a false match?
Comment 14 Stephan Herrmann CLA 2018-11-20 09:03:48 EST
(In reply to Cornelius Dirmeier from comment #12)
> First, and that's why I was answering to this issue, I expect to not get a
> fatal compilation error when there is any kind of misconfiguration like
> that. I think a warning would be far enough or if it realy has to be an
> error than it should not fail compilation - as far I have configured my
> compiler errors to not be treated as fatal errors. But at the moment I get
> an unresolved compilation error when ever I want to access the class.

AFAIK, the error was original classified as a build path problem, because generally trying to access a classfile which is not available on the build path means a severe mis-configuration where compilation simply cannot succeed.

We _could_ perhaps differentiate whether the missing classfile is for a regular type (which makes signatures unusable if missing) or an annotation type, where a missing annotation type makes a signature still usable by JLS, just not with its full intended annotated semantics. This would introduce a whole new problem category: non-fatal buildpath problems.

> The point is, that the application code which uses the framework have other
> null annotations configured at the moment. May be it is possible to get it
> work with the secondary annotations.

I do hope so, that's why we added support for more than one set of annotations.

> But still I dictate the properties of
> the downstream projects. That was not expected when we decided to change use
> the eclipse-null annotations. It seems like the null analysis interpretes
> the eclipse-null-annotations even if other null-annotations are configured
> in the settings. I might need to invetigate this scenario some more.

Implicit annotations (like in comment 0) should internally be represented using the *primary* annotation type. Secondary annotation types are only used where explicitly mentioned. If you see differently, please file a new bug.
 
> I do not want to export the dependencies of the null-annotations. Exporting
> the dependency means (in maven) that the dependency needs to be available at
> runtime. For example if the application project builds a WAR they will have
> the null annotations packaged. That is defintively not expected.

Some things I will never understand:

Don't build tools have a notion of build-time-only dependencies? Something that would directly match to RetentionPolicy.CLASS? Ideally, all annotations with that RetentionPolicy should always be required at build time, but never packaged for runtime. For maven there seem to be workarounds for simulating this, see e.g. https://stackoverflow.com/questions/12824112/is-there-a-maven-compiler-only-scope-for-dependency-artifacts

I don't know about your deployments, but I have seen projects pumping tens or hundreds of mega bytes into their deployments without much thought, only when it comes to a 25kb annotation bundle they will cry. What is the actual pain here? How much investment of efforts is justified by this tiny library?
Comment 15 Stephan Herrmann CLA 2018-11-20 09:08:46 EST
(In reply to Cornelius Dirmeier from comment #13)
> Now we have a realy strange behavior in our downstream projects. Long time
> ago... we started our framework with activated null-annotation-warnings.
> That time we wanted to use the JSR305 annotations and configured the
> javax.annotation.Nonnull annotation. But in an early version we had a type
> and configured javax.annotation.NonNull (second N caps as the eclipse
> annotation).
> 
> It seems like many projects compied our settings and never had any problems.
> In our settings we fixed the typo of course because we used the annotations,
> later found out that it is not very good supported by eclipse null
> annotations (because wrong @Target), ... finally switched to the eclipse
> null annotations.
> 
> The settings in the other projects were wrong all the time and nobody
> noticed. But now we released our new version using the eclipse null
> annotations and BAHM. They get a lot of errors saying "The type
> javax.annotations.NonNull cannott be resolved. It is indirectly referenced
> from required .class file".
> 
> Why? I know the settings are not correct but why it was no problem all the
> time until now? Is there any comparator for the unqualified name that leads
> to a false match?

That is weird, indeed. I'm not aware of any code location comparing only unqualified names. If you can reproduce, a new bug with steps to reproduce would be highly welcome.
Comment 16 Corneliud Dirmeier CLA 2018-11-20 09:20:48 EST
Ok, I'll try to reproduce it in a nutshell.

Regarding the dependencies and deployment: All workarounds for maven would be needed in downstream projects, so it is not a good idea out of a framework perspective. I think it is not so offbeat to say that only dependencies in a scope that is important at runtime is also transitive. In general this will lead to clearer dependencies because not every dependency that is used for one artifact at compile time is also necessary for others. Hence in maven these dependencies schould be 'provided' - not transitive.

Additionally I think this is also not the point. I do not want to dictate the kind of null annotations. For our downstream projects it is nice to have these annotations if they decide to use the same. If they decide to use JSR305 they should - eclipse simply shoult not bother. Of course they cannot benefit by our annotations directly if they do not want to. But may be this is indeed another bug.
Comment 17 Marco Descher CLA 2019-05-06 04:39:24 EDT
Still occuring

... .map(MessageParty::new).orElse(null) shows the error, 

replacing this with

... .map(e -> new MessageParty(e)).orElse(null)

does NOT show it anymore
Comment 18 Stephan Herrmann CLA 2019-05-21 18:41:53 EDT
Bulk move to 4.13.

Hopefully I can dedicate a sprint during 4.13 just to static analysis (null and resource).
Comment 19 Eclipse Genie CLA 2020-01-05 06:58:04 EST
New Gerrit change created: https://git.eclipse.org/r/155223
Comment 21 Eclipse Genie CLA 2020-01-05 13:28:51 EST
New Gerrit change created: https://git.eclipse.org/r/155237
Comment 22 Stephan Herrmann CLA 2020-01-05 14:14:22 EST
(In reply to Eclipse Genie from comment #20)
> Gerrit change https://git.eclipse.org/r/155223 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=af36a4d61d16ed2e7dc7a2bcd257172b51335524

This implements a new error, to differentiate the specific build path problem, where a null annotation is configured, but missing AND implicitly used, e.g. via a Class::new method reference.

In this commit the new problem is configurable, but re-reading the comments here, I don't see sufficient justification, why configurability is needed. Hence I will revert that part for M1:

> New Gerrit change created: https://git.eclipse.org/r/155237
Comment 24 Stephan Herrmann CLA 2020-01-05 15:56:42 EST
(In reply to Eclipse Genie from comment #23)
> Gerrit change https://git.eclipse.org/r/155237 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=256ddbc372ca0b473cf9e1359cd51a98fe2f20d9

I released the minimal solution to master for 4.15.
Comment 25 thomas menzel CLA 2020-05-11 06:15:29 EDT
i found an interesting conflict with "enable annotation-based null analysis" setting when workspace setting is ON while the per-project is OFF:

TLDR: if its ON in workspace prefs then it's on for all projects. 

Long Story:
i had still the compile error in my project while other's in my team didnt.
so i read again thru this bug and noticed comment #4 where it talks about project and workspace setting problems.

turns out: on my workspace i had set it to ON and as soon as i turned it OFF the error was gone!

shall i open a new bug or reopen this one?

ENV:
Version: 4.6.1.RELEASE
Build Id: 202004200639

note: i just had switched from the latest JEE eclipse 4.15 to STS 4 which my team mate uses in the hope to get rid of the compile error. my guess the behavior is the same for a vanilla eclipse but havent tested it

--------------
side topic
i also noticed that changing the workspace setting triggered a full build on all my projects but all are configured on this subject as "per project". Hence a change in the workspace settings should not trigger a build there

shall i open a new bug or this side topic as well? if this is  just an extension/consequence of the bug above then i dont think so.