Community
Participate
Working Groups
Bug 365859 in JDT/Core started out as a simple request to differentiate two kinds of nullness problems, but after feedback regarding the messages we took the opportunity to re-think classification and wording. As I hope the discussion in bug 365859 will soon come to a conclusion I am asking for corresponding changes in the preference UI. Perhaps feedback on the envisioned UI changes will help consolidate the core-level messages, too. The affected options are currently: Violation of null specification: [E/W/I] Potential violation of null specification: [E/W/I] Insufficient information for null analysis: [E/W/I] We identified room for improvement regarding each of the three groups. "Violation": If we buy into the reasoning that enabling null annotations means to consider these as an extension of the type system, then I can find no substantial reason, why definite violations (reported as "Type mismatch") should ever be downgraded to warning or even ignore. => I propose to remove this option from the UI, corresponding irritants will be effective as error as soon as null annotations are enabled. => FUP in 371968: Alignment from "Null pointer access" back to this group is no longer possible. "Potential violation...": discussions have shown that the concept of potential type errors is confusing. Still we want to express that errors in this group have these two properties: - they are ranked as type mismatch, because null could otherwise legally flow into a @NonNull variable - they are detected with the help of flow analysis. Java does have a concept of type inference (visible, e.g., in (b ? new String() : new Integer()). Loosely alluding to this concept I'm proposing the label: Conflict between null specification and null inference [E/W/I] The corresponding compiler message would be: Type mismatch: required '@NonNull Cat' but the provided value is inferred to '@Nullable Cat' Suggestions for another pair of related phrases that convey the same content are welcome. Finally, for the "insufficient info..." group I realized the close analogy to unchecked conversions like "List<String> l = new List()". Thus, I'd like to propose for the UI: Unchecked conversion from legacy type to @NonNull type [E/W/I] Corresponding to a compiler message of: Type safety: The expression of type 'Cat' needs unchecked conversion to conform to '@NonNull Cat' I realize that the previous wording may be perceived as smoother than the new proposals, but this smoothness is false peace, I'm afraid. We do have to educate users so they understand the new feature, and users OTOH have to be open-minded for adopting. So a tiny moment of startling could actually help :) Outlook: what do you think would be the best place for educational material regarding annotation based null analysis? What should go into the user doc, what to the wiki etc.? Sorry, for the additional churn, but I feel things are getting clearer now (hoping the wording faithfully conveys the ideas). Removing an unnecessary option before the final release also sounds like a good idea to me :)
(In reply to comment #0) > => I propose to remove this option from the UI, corresponding irritants will be > effective as error as soon as null annotations are enabled. Sure, I will agree the moment you also remove the option to set 'Null Pointer Access' problem :-) Till then it is a -1 from me. > "Potential violation...": discussions have shown that the concept of potential > type errors is confusing. Still we want to express that errors in this group > have these two properties: > - they are ranked as type mismatch, because null could otherwise legally flow > into a @NonNull variable Ok, so far. > - they are detected with the help of flow analysis. ... but is this part really necessary to the user? A type mismatch is a type mismatch, no? I would probably club the two options in one and just call the option - Violation of null specification. > I'm proposing the label: > > Conflict between null specification and null inference [E/W/I] Maybe not be the best possible label, but definitely better than 'Potential violation....'. So I am OK with this. > The corresponding compiler message would be: > > Type mismatch: required '@NonNull Cat' but the provided value is inferred > to > '@Nullable Cat' Again, it may be improved but I am OK with this as well. > Outlook: what do you think would be the best place for educational material > regarding annotation based null analysis? What should go into the user doc, > what to the wiki etc.? Educational material should definitely go in the user doc. You could probably add 'Static analysis' subtopic under 'Concepts' or something similar under 'Tasks'.
(In reply to comment #1) > (In reply to comment #0) > > => I propose to remove this option from the UI, corresponding irritants will be > > effective as error as soon as null annotations are enabled. > Sure, I will agree the moment you also remove the option to set 'Null Pointer > Access' problem :-) I can't. That one is not protected by "Enable annotation based null analysis". > Till then it is a -1 from me. If that's a management decision I'll have to accept it, otherwise I'm really waiting for an argumentation or an example that supports your point. > > "Potential violation...": discussions have shown that the concept of potential > > type errors is confusing. Still we want to express that errors in this group > > have these two properties: > > - they are ranked as type mismatch, because null could otherwise legally flow > > into a @NonNull variable > > Ok, so far. > > > - they are detected with the help of flow analysis. > ... but is this part really necessary to the user? A type mismatch is a type > mismatch, no? I would probably club the two options in one and just call the > option - Violation of null specification. If flow analysis is involved we're in a gray area regarding type checking: We admit unspecified locals because usually we can infer their nullness pretty well. We actually invite users to omit null annotations, relying on our analysis. However, the analysis has known weaknesses (e.g., lack of correlation analysis), where our inference will not be able to see s.t. that the user may actually see. In these cases it can be safe to ignore a warning because the user may "know better". Also a better analysis might "know better". Such "knowing better" doesn't apply to definite spec violations. That's the difference, which I consider significant. > > Outlook: what do you think would be the best place for educational material > > regarding annotation based null analysis? What should go into the user doc, > > what to the wiki etc.? > Educational material should definitely go in the user doc. You could probably > add 'Static analysis' subtopic under 'Concepts' or something similar under > 'Tasks'. Great. I will draft s.t. for M7 hoping that's not too late.
(In reply to comment #2) > If that's a management decision I'll have to accept it, otherwise I'm really > waiting for an argumentation or an example that supports your point. Well, I am not in management :-) - It just feels wrong to me that a user will be able to set 'Null pointer access' to warning but not 'Violation of null specification'. - We also added a dialog to keep these two in sync, that also gets broken. > Such "knowing better" doesn't apply to definite spec violations. That's the > difference, which I consider significant. Ok, point taken. > Great. I will draft s.t. for M7 hoping that's not too late. Docs can be updated till RC2 (or RC3?). Also we (JDT/UI) tend to delay updating doc till RC phase as doc changes can't break something ;-) So you have time here.
(In reply to comment #3) > - It just feels wrong to me that a user will be able to set 'Null pointer > access' to warning but not 'Violation of null specification'. Right, on the one hand we have "it just feels wrong". On the other hand I can give you examples were unfortunate combinations of ignoring this problem and enabling other related issues can lead to plainly wrong diagnostics, i.e., with the option we - in a manner of speaking - empower the user to inject bugs into the compiler. Which one is stronger? :) > - We also added a dialog to keep these two in sync, that also gets broken. It actually is quite helpful I believe, especially when it recommends to set definite null pointer access to error (see comment 1 :)). From the four directions of syncing problems only one will no longer work: from definite null pointer access to definite spec violation. But the primary intention behind the dialog was from spec-violation to null pointer access any way, so I don't see a great loss. Do you? Would it be difficult to adjust the dialog accordingly? > > Great. I will draft s.t. for M7 hoping that's not too late. > Docs can be updated till RC2 (or RC3?). Also we (JDT/UI) tend to delay > updating doc till RC phase as doc changes can't break something ;-) > So you have time here. Great.
(In reply to comment #3) > (In reply to comment #2) > > If that's a management decision I'll have to accept it, otherwise I'm really > > waiting for an argumentation or an example that supports your point. > Well, I am not in management :-) Markus, could you please take a look at this and let us know of your opinion on this ? TIA.
Different people like to use JDT Core compiler options in different ways, some prefer to see everything as a warning, others set some of the options to error. For instance - o.e.jdt.core project has 'Null pointer access' and 'Dead Code' set to warning, on the other hand o.e.jdt.ui has set these options to error. I think both ways of working are OK, as it really depends on people and how they want to use the tool. Enforcing a way of working which is not defined by a standard, i.e. the Java Language specification in this case, looks extreme to me. In any case I do not see what is to be gained by removing 'Violation of null specification' option. I rest my arguments as far as this option is concerned.
(In reply to comment #0) > "Violation": If we buy into the reasoning that enabling null annotations means > to consider these as an extension of the type system I agree with this. > then I can find no > substantial reason, why definite violations (reported as "Type mismatch") > should ever be downgraded to warning or even ignore. > => I propose to remove this option from the UI, corresponding irritants will be > effective as error as soon as null annotations are enabled. I disagree. We should keep a way for users to enable annotation-based null analysis, but set all generated problems to Warning. After all, the annotations are not enforced at run time, and even if they are wrong, the code could still compile and run successfully. Especially for projects that are transitioning to use null annotations, even setting definite violations to Warning can be helpful. Until all problems are solved, the workspace is not polluted with errors. Another scenario is a build server that builds a project where not all of the contributors use the latest Eclipse compiler. There, the null analysis could still report helpful warnings, but an error could unnecessarily break a build. Note that the "Violation of null specification" option already now only offers [E/W], but not Ignore (should completely disable annotation analysis instead). Re option names: I agree that all 3 current names are not self-explanatory. How about: Conflict between null annotation and reference: [E/W] Conflict between null annotations and null inference: [E/W/I] Unchecked conversion from non-annotated type to @NonNull type: [E/W/I] I'm not too sure about the last one. I like the "Unchecked conversion", but I also agree with Satyam that "legacy" is not a good term to use. Maybe we need the full truth: Unchecked conversion from type without null annotation to @NonNull type: [E/W/I]
(In reply to comment #7) > Re option names: I agree that all 3 current names are not self-explanatory. > How about: > > Conflict between null annotation and reference: [E/W] > Conflict between null annotations and null inference: [E/W/I] > Unchecked conversion from non-annotated type to @NonNull type: [E/W/I] > > I'm not too sure about the last one. I like the "Unchecked conversion", but I > also agree with Satyam that "legacy" is not a good term to use. Maybe we need > the full truth: > > Unchecked conversion from type without null annotation to @NonNull type: > [E/W/I] This is a bit incomplete :-) The corresponding problem messages would be the ones from comment 0? - Type mismatch: required '@NonNull Cat' but the provided value is inferred to '@Nullable Cat' - Type safety: The expression of type 'Cat' needs unchecked conversion to conform to '@NonNull Cat'
> This is a bit incomplete :-) I assume you mean my comment is incomplete, not the option names. > The corresponding problem messages would be the > ones from comment 0? Yes. To get a better distinction from generics-related type safety problems, the problem messages could also start with "Null type mismatch:" and "Null type safety:".
(In reply to comment #9) > > This is a bit incomplete :-) > I assume you mean my comment is incomplete, not the option names. Yup :-) Your proposal from comment 7 and comment 9 is also agreeable to me.
(In reply to comment #9) > To get a better distinction from generics-related type safety problems, the > problem messages could also start with "Null type mismatch:" and "Null type > safety:". And the warning as a result of "Conflict between null annotations and null inference" still needs to be ironed out. Most of the discussion is on bug 365859, but this is the trickier part, especially since we debunked the "potential" out. :)
(In reply to comment #7) > (In reply to comment #0) > > => I propose to remove this option from the UI, corresponding irritants will > > be effective as error as soon as null annotations are enabled. > > I disagree. We should keep a way for users to enable annotation-based null > analysis, but set all generated problems to Warning. After all, the annotations > are not enforced at run time, and even if they are wrong, the code could still > compile and run successfully. > > Especially for projects that are transitioning to use null annotations, ... I'm not convinced the definite violation errors are the ones that would pollute a project; sounds like the result of an awkward strategy for annotating. Meanwhile bug 365859 comment 36 contains an example for what I meant in comment 4: "unfortunate combinations of ignoring this problem and enabling other related issues can lead to plainly wrong diagnostics, i.e., with the option we - in a manner of speaking - empower the user to inject bugs into the compiler." I still believe this potential damage weighs more then the flexibility. > Note that the "Violation of null specification" option already now only offers > [E/W], but not Ignore (should completely disable annotation analysis instead). Sorry, I forgot, indeed. So ignoring definite violations is already a bit more difficult, at least :) From all proposed UI labels, only this looks strange to me: (In reply to comment #7) > Conflict between null annotation and reference: [E/W] To me this sounds too similar to dereference. (In reply to comment #9) > > The corresponding problem messages would be the > > ones from comment 0? > Yes. > > To get a better distinction from generics-related type safety problems, the > problem messages could also start with "Null type mismatch:" and "Null type > safety:". Sounds good. Bug 365859 has been pushed with these changes.
(In reply to comment #12) > Sounds good. Bug 365859 has been pushed with these changes. If we are converging, can we have this for M6 so we can achieve closure ? Thanks.
I'd go with the option names from comment 7: > Conflict between null annotation and reference: [E/W] > Conflict between null annotations and null inference: [E/W/I] > Unchecked conversion from non-annotated type to @NonNull type: [E/W/I] Will jdt.core also update the JavaCore.COMPILER_PB_*NULL*SPEC options to match these labels?
(In reply to comment #14) > I'd go with the option names from comment 7: > > > Conflict between null annotation and reference: [E/W] > > Conflict between null annotations and null inference: [E/W/I] > > Unchecked conversion from non-annotated type to @NonNull type: [E/W/I] > > Will jdt.core also update the JavaCore.COMPILER_PB_*NULL*SPEC options to match > these labels? Yes that makes sense. We should adjust JavaCore options, as well as the CompilerOptions and the doc. Something as follows: JavaCore.COMPILER_PB_NULL_ANNOTATION_REFERENCE_CONFLICT JavaCore.COMPILER_PB_NULL_ANNOTATION_INFERENCE_CONFLICT JavaCore.COMPILER_PB_NULL_UNCHECKED_CONVERSION
Pushed label changes on the preference page: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=df4b48a66b62c7cbb53a09b37e22e8eba9c080b7 Keeping this bug open to follow changes in the JavaCore option APIs (Ayush will do comment 15 via bug 365859). Stephan & Srikanth: Last chance to raise objections against the option names.
(In reply to comment #12) > From all proposed UI labels, only this looks strange to me: > > Conflict between null annotation and reference: [E/W] > > To me this sounds too similar to dereference. Sorry, I initially missed this comment. Do you have a better suggestion? I also thought about Conflict between null annotation and usage ... but I found "reference" more exact, since this is also for problems when a method overrides another method in an incompatible way. Note that we don't use "dereference" anywhere in the UI. We usually say "access", so I don't think "Conflict between null annotation and reference" is confusing.
(In reply to comment #17) > (In reply to comment #12) > > From all proposed UI labels, only this looks strange to me: > > > Conflict between null annotation and reference: [E/W] > > > > To me this sounds too similar to dereference. > > Sorry, I initially missed this comment. Do you have a better suggestion? As you know my 1. preference is - drop the option Next, in order to match the error messages, I could think of s.t. like - "Definite null type mismatch" But for that to work we would need to separate out the override-related errors. -> Not for 3.8 (?) If these options are ruled out I don't see how we can do much better than: - "Violation of null specification" If the similarity among labels is important I could think of - "Conflicting null annotations" But that again wouldn't match all problems in this group. I don't know if any of these comments helps. Maybe, for optimal correspondence the preference page just needs a preview displaying the actual error messages controlled by each individual option :) (In reply to comment #15) > JavaCore.COMPILER_PB_NULL_ANNOTATION_REFERENCE_CONFLICT Similar comments as for the label. > JavaCore.COMPILER_PB_NULL_ANNOTATION_INFERENCE_CONFLICT > JavaCore.COMPILER_PB_NULL_UNCHECKED_CONVERSION These two are fine with me.
We're not going to drop the first option, but I agree that we should keep its name COMPILER_PB_NULL_SPECIFICATION_VIOLATION and UI label "Violation of null specification". Ayush, can you please prepare a patch for jdt.core and attach it to bug 365859? I'll take care of the UI then (don't need a patch).
Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=e4b13447adb6965a8f37f71161368b392e186ec4
(In reply to comment #7) > Note that the "Violation of null specification" option already now only > offers [E/W], but not Ignore (should completely disable annotation analysis > instead). > > Re option names: I agree that all 3 current names are not self-explanatory. > How about: > > Conflict between null annotation and reference: [E/W] > Conflict between null annotations and null inference: [E/W/I] > Unchecked conversion from non-annotated type to @NonNull type: [E/W/I] Right now missing "Ignore" option for "Conflict between null annotation and reference" is really frustrating: as there is lot of "legacy" code which "inherits" NonNull annotation without explicitly tagging them in source (see bug 388281). Se we can't enable the entire annotation based null analysis for JDT compiler, because otherwise we will get TONS of meaningless warnings. Can we enable [E/W/I] for "Conflict between null annotation and reference" please? This is only setting without "Ignore" option. Should I open a new bug for it?
(In reply to comment #21) > Right now missing "Ignore" option for "Conflict between null annotation and > reference" is really frustrating: as there is lot of "legacy" code which > "inherits" NonNull annotation without explicitly tagging them in source (see > bug 388281). > > Se we can't enable the entire annotation based null analysis for JDT > compiler, because otherwise we will get TONS of meaningless warnings. Each occurrence of "conflict between null annotation and reference" is a bug. Either a wrongly placed annotationn or an unsafe dereference. Can you give an example how this situtation could be "meaningless"? > Can we enable [E/W/I] for "Conflict between null annotation and reference" > please? This is only setting without "Ignore" option. Should I open a new > bug for it? I don't believe you'll be able to convince me that this is a good idea. Telling the compiler to continue analysing with known wrong premisses is worse than disabling annotation based analysis. Rather than injecting errors into the analysis we should move forward with bug 388281. Since nobody raised his voice for including this into the beta feature (null annotations for fields), we should try to get this into master actually. Am I missing anything? Is there a problem that even bug 388281 cannot resolve?
(In reply to comment #22) Sorry, I was referring to the option description specified in bugzilla comment, but I meant the "Violation of null specification" option (org.eclipse.jdt.core.compiler.problem.nullSpecViolation). Looks like the final version contains different UI text then the original string in bugzilla. > (In reply to comment #21) > > Right now missing "Ignore" option for "Conflict between null annotation and > > reference" is really frustrating: as there is lot of "legacy" code which > > "inherits" NonNull annotation without explicitly tagging them in source (see > > bug 388281). > > > > Se we can't enable the entire annotation based null analysis for JDT > > compiler, because otherwise we will get TONS of meaningless warnings. > > Each occurrence of "conflict between null annotation and reference" is a bug. > Either a wrongly placed annotationn or an unsafe dereference. > Can you give an example how this situtation could be "meaningless"? That's easy: you have a "core" library specifying a contract and a 3rd party client code which is not under your control. The code in the "core" library like below will be always full with warnings: @Nonnull public static IResponse execute(@Nonnull IRequest request) { if (request == null) { throw new NullPointerException("Request class is null"); } ... In this code Eclipse reports: "Null comparison always yields false: The variable request is specified as @Nonnull". So as I can't trust the 3rd party code, AFAIK I have only one option to have "clean" code today - disable annotation based code analysis completely. > > Can we enable [E/W/I] for "Conflict between null annotation and reference" > > please? This is only setting without "Ignore" option. Should I open a new > > bug for it? > > I don't believe you'll be able to convince me that this is a good idea. > > Telling the compiler to continue analysing with known wrong premisses is > worse than disabling annotation based analysis. > > Rather than injecting errors into the analysis we should move forward with > bug 388281. Since nobody raised his voice for including this into the beta > feature (null annotations for fields), we should try to get this into master > actually. bug 388281 would definitely help, because second example of "meaningless" warnings is related to inherited annotation information. > Am I missing anything? Is there a problem that even bug 388281 cannot > resolve? The problem with 3rd party code would remain unresolved.
(In reply to comment #23) I've moved the discussion to bug 385440 comment 27.
