Community
Participate
Working Groups
This bug will be used a as a placeholder to log issues I discover during review and verification of the implementation for bug 186342 for M4 test week. Source code references are w.r.t the patch referred to by https://bugs.eclipse.org/bugs/show_bug.cgi?id=186342#c196 Right now there is one issue: (1) What are the changes in Scanner.java for ? Even without those changes all tests pass. Changes to Scanner.java almost always need to be mirrored into PublicScanner.java and in this case, only one file has changed. As this change, even if only a minor one, constitutes a change deep in the entrails, we should justify this with a regression test. Otherwise, if this change is not integral to this feature we should consider backing out this particular change.
Also including this query here: Is the TODO mentioned here (https://bugs.eclipse.org/bugs/show_bug.cgi?id=186342#c171) done? >+ variant using a generic constructor to challenge an alternative flow in > BinaryTypeBinding.createMethod: this revealed an omission to propagate > parameterNonNullness from an original MethodBinding to its > ParameterizedGenericMethodBinding version. > Fixed for several subclasses of MethodBinding. I omitted: > + ProblemMethodBinding: obviously irrelevant, right? > + SyntheticMethodBinding: is probably relevant. TODO > + PolymorphicMethodBinding: frankly I couldn't think of a way to get > parameter annotations into one of these.
(In reply to comment #0) > (1) What are the changes in Scanner.java for ? Even without those changes all > tests pass. Changes to Scanner.java almost always need to be mirrored into > PublicScanner.java and in this case, only one file has changed. Ayush pointed me to https://bugs.eclipse.org/bugs/show_bug.cgi?id=186342#c118 which claims this scanner change was made to address a regression in CompletionTests.testCompletionVariableName32. This test did not fail for me when I removed the scanner changes.
I'll assign issue numbers (2) and (3) for these two brought over from https://bugs.eclipse.org/bugs/show_bug.cgi?id=186342#c198 (2_) Just checking - was this part done? >+ SyntheticMethodBinding: is probably relevant. TODO (3) And is there a bug for this: >Created attachment 207657 [details] [details] >alternative strategy for internally encoding nullness defaults Just a clarification that an "issue" listed here need not necessarily result in a code change and even when it does need not go into M4. It only means that the issue must be given due consideration right away. I have finished reviewing 41/59 files that went into the commit. So far no major issues have been found. A bunch of minor ones found are: (4) OPTION_ReportNullSpecInsufficientInfo is missing in the list returned by org.eclipse.jdt.internal.compiler.impl.CompilerOptions.warningOptionNames() Is this intentional ? (5) org.eclipse.jdt.internal.compiler.impl.CompilerOptions.resetDefaults() is not resetting defaultNonNullness - Is this intentional ? (6) Could we have reused org.eclipse.jdt.internal.compiler.lookup.MethodScope.insideTypeAnnotation instead of inventing org.eclipse.jdt.internal.compiler.ast.ASTNode.IsMemberValueReference ? At the point where we use this bit to avoid reporting a forward reference, the insideTypeAnnotation field has been updated properly for us to have to have used it, though it is not readily available there - being a member of MethodScope, while the static type at the use site is the parent type BlockScope. (7) Do the other calls to forwardReference need similar treatment ? (I didn't check in any detail) (8) just a nitpick: "can not" should be cannot.
One concern I have regarding earlier/eagerer(?!) processing of annotations: I understand the rationale that necessitates this. However I haven't studied the patch enough to know if the code that was processing annotations in the "usual place" has been removed or is still there. It looks like to avoid a test failure in org.eclipse.jdt.core.tests.compiler.regression.EnumTest.test180() *even when the master switch was off* some new code changes were needed to be done. It would have been ideal if all the eager processing of annotations was controlled by the master switch so that upon upgrading to a new eclipse a project will have ZERO IMPACT if they don't plan to use the new feature. If the original annotation processing code continues to be in place, I would recommend using the master switch at all eager processing sites, given how close this is to the milestone. After a few weeks, this can be removed if deemed a good idea. Stephan/Ayush, please clarify where we stand on this. In case the old code paths have been deleted, could you point me to the relevant places ?
Created attachment 207842 [details] Fix for an AIOOBE found during stress test (9) While doing stress tests which the new analysis enabled on the code base of the JDT/Core with many null-annotations added I observed an AIOOBE that can easily be fixed by the attached patch. Analysis showed that the actual array size of this.expectedTypes can be more than one behind the value in nullCount. I propose to include the fix in M4 based on the following assessment: - the bug does not occur when the analysis is disabled + AIOOBE is bad and the fix is local and small. A regression test will follow.
Created attachment 207843 [details] test & fix for a wrong warning (10) The same stress test as in the previous comment also revealed a bogus warning (redunant null check). The attached patch contains a regression test and a straight forward fix. Analysis: a message send, where the target method has the @Nullable annotation reports its null-status as POTENTIALLY_NULL, which is OK for raising desired warnings. However, when assigning the result to a variable that previously hat status NULL, this fails to signal that the variable can also have a non-null value after the assignment. The simple fix is to report POTENTIALLY_NULL | POTENTIALLY_NON_NULL Assessment: - also this bug only occurs with the new analysis enabled - the wrong warning is no showstopper + the fix is well localized and small
(In reply to comment #0) > (1) What are the changes in Scanner.java for ? Even without those changes all > tests pass. Changes to Scanner.java almost always need to be mirrored into > PublicScanner.java and in this case, only one file has changed. > > As this change, even if only a minor one, constitutes a change deep in the > entrails, we should justify this with a regression test. Otherwise, if this > change is not integral to this feature we should consider backing out this > particular change. I'll try to reconstruct the regression in a test case as a basis for your decission about possible backing out.
(In reply to comment #3) > (2_) Just checking - was this part done? > >+ SyntheticMethodBinding: is probably relevant. TODO Still TODO, I'm not sure if any change is required. Will report back soon. > (3) And is there a bug for this: > >Created attachment 207657 [details] [details] [details] > >alternative strategy for internally encoding nullness defaults Clearly for consideration post M4. > (4) OPTION_ReportNullSpecInsufficientInfo is missing in the list returned > by org.eclipse.jdt.internal.compiler.impl.CompilerOptions.warningOptionNames() > Is this intentional ? > > (5) org.eclipse.jdt.internal.compiler.impl.CompilerOptions.resetDefaults() is > not resetting defaultNonNullness - Is this intentional ? Both: Pure oversight. I'll fix it. > (6) Could we have reused > org.eclipse.jdt.internal.compiler.lookup.MethodScope.insideTypeAnnotation > instead of inventing > org.eclipse.jdt.internal.compiler.ast.ASTNode.IsMemberValueReference ? > At the point where we use this bit to avoid reporting a forward reference, > the insideTypeAnnotation field has been updated properly for us to have to > have used it, though it is not readily available there - being a member of > MethodScope, while the static type at the use site is the parent type > BlockScope. Interesting suggestion, I'll check and report back. > > (7) Do the other calls to forwardReference need similar treatment ? (I didn't > check in any detail) good point, I'll check. > (8) just a nitpick: "can not" should be cannot. Where did you find "can not"? Assessment: (4)+(5) are bugs (2)+(7) are due checks for completeness (3)+(6) future considerations for better code quality (8) ? do you agree?
(In reply to comment #4) > One concern I have regarding earlier/eagerer(?!) processing of annotations: > I understand the rationale that necessitates this. However I haven't studied > the patch enough to know if the code that was processing annotations in the > "usual place" has been removed or is still there. Nothing has been removed, essentially method Argument.bind(..) was split into two, so that the new Argument.createBinding() can now be called separatedly, but just calling the original method also works. > It looks like to avoid a test failure in > org.eclipse.jdt.core.tests.compiler.regression.EnumTest.test180() > *even when the master switch was off* some new code changes were needed to > be done. It would have been ideal if all the eager processing of annotations > was controlled by the master switch so that upon upgrading to a new eclipse > a project will have ZERO IMPACT if they don't plan to use the new feature. If desired this can be achieved by protecting the call to createArgumentBindings (inside SourceTypeBinding.resolveTypesFor(MB)) by if (this.scope.compilerOptions().isAnnotationBasedNullAnalysisEnabled) This suffices to fix the problem in EnumTest.test180(), i.e., in this case the change in QualifiedNameReference (using ASTNode.IsMemberValueReference) is only needed when the analysis is switched *on*.
(In reply to comment #7) > (In reply to comment #0) > > (1) What are the changes in Scanner.java for ? Even without those changes all > > tests pass. Changes to Scanner.java almost always need to be mirrored into > > PublicScanner.java and in this case, only one file has changed. > > > > As this change, even if only a minor one, constitutes a change deep in the > > entrails, we should justify this with a regression test. Otherwise, if this > > change is not integral to this feature we should consider backing out this > > particular change. > > I'll try to reconstruct the regression in a test case as a basis for > your decission about possible backing out. Update: this regression is no longer reproducable. In intermediate versions I simply called Argument.bind(..) from STB.resolveTypesFor(..) but at one point I split bind() into two methods (the same split as also mentioned in comment 9). This split lessens the effect of resolving annotations earlier, which achieves that also CompletionTests.testCompletionVariableName32 passes even without the change in Parser. If you like we can backout the change in Parser for now. We can then check after M4 if my claim, that the changed version is safer, actually holds. On a side note we *could* also change CompletionOnArgumentName to intercept (override) the new method createBinding instead of bind. On the one hand this would restore the mentioned regression but on the positive side this would mean that CompletionNodeFound is thrown a bit earlier. Not a big deal but in line with the strategy to throw CompletionNodeFound as soon as the binding is available.
(In reply to comment #8) > > (6) Could we have reused > > org.eclipse.jdt.internal.compiler.lookup.MethodScope.insideTypeAnnotation > > instead of inventing > > org.eclipse.jdt.internal.compiler.ast.ASTNode.IsMemberValueReference ? [...] > Interesting suggestion, I'll check and report back. Post M4 is OK. We may have to introduce a getter method at BlockScope level for this. > > (8) just a nitpick: "can not" should be cannot. > > Where did you find "can not"? org.eclipse.jdt.annotation.NonNull documentation. That said, I found a few dozens of existing usages roughtly half inside API javadocs and messages. I'll let you take a call. (we don't want to change existing usages now) > Assessment: > (4)+(5) are bugs Since these are trivial and safe, let us include them for M4. > (2)+(7) are due checks for completeness > (3)+(6) future considerations for better code quality > (8) ? > do you agree? Yes. (In reply to comment #9) > If desired this can be achieved by protecting the call to > createArgumentBindings (inside SourceTypeBinding.resolveTypesFor(MB)) > by > if (this.scope.compilerOptions().isAnnotationBasedNullAnalysisEnabled) Yes, please ! Let us do this for M4. Is this the only place such a treatment would be needed/possible ?
(In reply to comment #5) > Created attachment 207842 [details] > Fix for an AIOOBE found during stress test [...] > A regression test will follow. Is this included in the patch below ? (In reply to comment #6) > Created attachment 207843 [details] > test & fix for a wrong warning Both look candidates for M4. After Ayush has a chance to review it, we will finalize.
(In reply to comment #10) > If you like we can backout the change in Parser for now. > We can then check after M4 if my claim, that the changed version is safer, > actually holds. You mean the Scanner, yes please, let us restore this code to the original. and revisit this post M4. So if we number the gating of createArgumentBinding by the master switch as issue (11), at this time there is agreement that (1), (4), (5), (8 ?), (9), (10), (11) are candidates for M4. Let us target to deliver them on Monday, followed by another patch if needed on Tuesday. Stephan, can you post a tested patch with these by Monday ? Thanks.
Here are a few more: None of them urgent enough to be fixed for M4. (12) Nit: org.eclipse.jdt.internal.compiler.ast.EqualExpression.checkNullComparison: Comment reads: // check if either method is annotated @NonNull and compared to null: I think you meant // check if either is a method annotated @NonNull and compared to null: (13) Nit: Argument#bind(): Why is the call to the new extracted method createBinding happening in the "wrong" place ? It could have been made in the same place from which the extraction happened, i.e just after the localVariableHiding diagnostic. I don't see a problem per se with the new place, just that it makes a reviewer spend wasteful cycles trying to understand if there is some side effect that needs to be understood. (can be left as it is) (14) Nit: Argument#bind(): This (pre-existing) comment should have been deleted long ago. It makes no sense there now: (Long ago, there was a call to the ctor of LVB there) //true stand for argument instead of just local (15) ReturnStatement#checkAgainstNullAnnotation(): What motivates the try-catch block for the NPE ? (can leave it as it is regardless) (16) org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.findDefaultNullness: The line that reads: while (currentType instanceof SourceTypeBinding) { Could this have been the cheaper check: while (currentType != null) ? (If so fix post M4) (17) org.eclipse.jdt.internal.compiler.lookup.MethodBinding.fillInDefaultNonNullness for (int i = 0; i < this.parameterNonNullness.length; i++) { length could have been extracted into a local since it is loop invariant (no need to fix now) Same comment for org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.validateAnnotations() (18) Same method: I think you meant to have boolean added = false; and if (added) this.tagBits |= TagBits.HasParameterAnnotations; outside the loop. (19) org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.addNullnessAnnotation Should this have been named addNonNullAnnotation to match org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.addParameterNonNullAnnotation They both do the same thing, one on parameters and the other on return type.
(20) org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.scanMethodForNullAnnotation For loop: Length could have been extracted into a local. Same comment for org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.scanTypeForNullAnnotation (21) org.eclipse.jdt.internal.compiler.env.IBinaryMethod.getNumParameterAnnotations() is a highly misleading name. We should change it to getTotalParameterCount(). Does the current method always evaluate to Util.getParameterCount(methodInfo.getMethodDescriptor()) ? If so, do we even need this new method ? (22) org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.scanTypeForNullAnnotation I don't see how this method makes sense for any case other than package-info.java There are two effects: (a) BinaryTypeBinding's tagBits gets updated (b) The package containing the BTB - its nullnessDefaultAnnotation gets updated. I cannot see why (a) matters since a type cannot be split across binary & source (b) can matter because part of a package can be in binary form and the rest in source and if package-info carried a default annotation specification, that influences the source part of the package too. If there is no major misunderstanding here, why wouldn't we check for package-info class file right at the top and get out of there quickly ?
Created attachment 207876 [details] accumulated fixes (In reply to comment #13) > (In reply to comment #10) > > > If you like we can backout the change in Parser for now. > > We can then check after M4 if my claim, that the changed version is safer, > > actually holds. > > You mean the Scanner, yes please, let us restore this code to the original. Scanner, sure. Sorry for the confusion. > and revisit this post M4. So if we number the gating of createArgumentBinding > by the master switch as issue (11), at this time there is agreement that > > (1), (4), (5), (8 ?), (9), (10), (11) > > are candidates for M4. Let us target to deliver them on Monday, followed by > another patch if needed on Tuesday. Stephan, can you post a tested patch with > these by Monday ? Thanks. This patch accumulates the changes for the above list, just the regression test for item (9) is still missing - I need to distill existing an witness to a small test case. Patch has been tested using RunJDTCoreTests which only brought to light that two test cases in the converter suite are now back to their original result. This result is actually wrong, but the real cause is bug 164660 which is still unresolved. The patch in bug 186342 produced the right outcome in these tests because annotations are resolved earlier. As of item (11) we are back to the previous state. I've documented the connection in the test. Apart from that change I saw all green.
I have completed the review and here is the final batch of comments: (23) org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.validateAnnotations Should be renamed to be specific to null annotations. (24) Extract to local: (a) org.eclipse.jdt.internal.compiler.ast.Statement.analyseArguments arguments.length can be extracted into a local (for loop) (b) org.eclipse.jdt.internal.compiler.lookup.MethodVerifier15.checkNullSpecInheritance inheritedMethod.parameterNonNullness.length can be extracted into local. currentMethod.parameterNonNullness.length can be extracted into local. (c) org.eclipse.jdt.internal.compiler.lookup.PackageBinding.isPackageOfQualifiedTypeName packageName.length : extract to local. (d) org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.validateAnnotations this.binding.parameters.length can be extracted into a local. (e) org.eclipse.jdt.internal.compiler.problem.ProblemReporter.findAnnotationSourceStart annotations.length can be extracted to local. (25) org.eclipse.jdt.internal.compiler.ast.Statement.analyseArguments: Will it work well with varargs methods ? for (int i = 0; i < arguments.length; i++) { if (methodBinding.parameterNonNullness[i] == Boolean.TRUE) { TypeBinding expectedType = methodBinding.parameters[i]; Expression argument = arguments[i]; int nullStatus = argument.nullStatus(flowInfo); // slight loss of precision: should also use the null info from the receiver. if (nullStatus != FlowInfo.NON_NULL) // if required non-null is not provided flowContext.recordNullityMismatch(currentScope, argument, nullStatus, expectedType); } } This loop above assumes arguments and parameters can be indexed lock step with each other. (26) If 25 is an issue, check arg varargs index safety everywhere - Devise regression tests expressly for this purpose. (27) Surround the entire of the method body of org.eclipse.jdt.internal.compiler.lookup.PackageBinding.checkIfNullAnnotationType with a if (env.globalOptions.isAnnotationBasedNullAnalysisEnabled) (28) Rather than inventing org.eclipse.jdt.internal.compiler.lookup.PackageBinding.checkIfNullAnnotationType we could have used org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding.computeId. It would have been cleaner to continue to centralize the id computation in one place. (29) Should org.eclipse.jdt.internal.compiler.problem.ProblemReporter.parameterLackingNonNullAnnotation have been named parameterLackingNullAnnotation ? (30) No need to fix this: It would have been better if the expected types maintenance in FlowContext was pushed down to Fina*Flow*Context and Loop*Flow*Context even if this meant duplication. Such a solution would have integrated better with the existing code (See management of nullLocals, nullReferences and nullCheckTypes in both classes) (31) org.eclipse.jdt.internal.compiler.flow.FlowContext.recordNullityMismatch: Did you mean to have the statements: // no reason to defer, so report now: char[][] annotationName = currentScope.environment().getNonNullAnnotationName(); currentScope.problemReporter().nullityMismatch(expression, expectedType, nullStatus, annotationName); inside the if (expression.localVariableBinding() != null) { // flowContext cannot yet handle ... block ??? (32) For org.eclipse.jdt.internal.compiler.problem.ProblemReporter.nullityMismatch diagnostics, could you give me an example that explains why LFC and FFC would want to implement deferred checking ? i.e is there a case where there would be a material difference in reporting ? I couldn't think of a case where the problem id would change - did I overlook something ?
(In reply to comment #16) > Created attachment 207876 [details] > accumulated fixes Thanks Stephan, I'll get this released in time for M4. Here is what I would recommend for the rest of the comments: Many of them are editorial in nature and if after due consideration, you choose not to follow up some of them and treat them as general review guidelines for future work it is fine by me. For those you follow up I would request 2 separate patches: (i) Only the changes made for (3),(25),(26) (if they call for changes) need further code review. (ii) A tested patch with all other changes can be released without review any time *after* M4 ships, as early as possible.
It is very evident to a reviewer that there is a lot of planning, care and diligence that have gone into this work. The code integrates very well with the existing body of code - architecturally and structurally speaking. Thanks a lot for this work Stephan and look forward to ongoing contributions from you !
(In reply to comment #18) > (ii) A tested patch with all other changes can be released without review > any time *after* M4 ships, as early as possible. I've created bug 365519 as a scratch pad for these changes.
Created attachment 207888 [details] regression test for item (9) Here's the promised regression test for item (9). The part of attachment 207876 [details] that affects FlowContext toggles PASS/ERROR in this new test.
Regarding this one: > (2) Just checking - was this part done? >> + SyntheticMethodBinding: is probably relevant. TODO I could not find any path how a SyntheticMethodBinding could be used during analyseCode, hence I conclude they don't need values to be copied to parameterNonNullness. -> no action. This leaves us with the following status: M4 candidates: ============== (1), (4), (5), (8), (9), (10), (11) resolved as per comment 16 with attachment 207876 [details] plus regression test for (9) from comment 21 (attachment 207888 [details]) Plus possibly: (25), (26) TODO Post M4: ======== (3): bug 365531 (12), (14), (15), (16), (18), (19), (23), (27) (improved), (29) -> patch prepared, via bug 365519 (17), (20), (24) -> trivial, via bug 365519 No action planned: ================== (2) - see above (this comment) (13), (28)?, (30), (31), (32) - comments in bug 365519 Pending assessment: =================== (6), (7), (21), (22)
(In reply to comment #17) > (24) Extract to local: May be we should add some support in the compiler at some future point to at least diagnose such cases: while it is generally held true that the quality of jit compiler generated code is way more important than the byte code quality, there can be lots of opportunities around the traditional classic optimizer transformations such loop invariant code motion, common subexpression elimination etc.
(In reply to comment #18) > (In reply to comment #16) > > Created attachment 207876 [details] [details] > > accumulated fixes > > Thanks Stephan, I'll get this released in time for M4. This patch and ... (In reply to comment #21) > Created attachment 207888 [details] > regression test for item (9) > > Here's the promised regression test for item (9). ... this patch both look good and released for 3.8 M4 via commit c077066d38e166a56e4eb4a0d6b6c8b7125fd6f2. (In reply to comment #22) [...] > M4 candidates: [...] > Plus possibly: (25), (26) > TODO OK, we can consider this one too, if a patch comes in time. Jay - FYI, please include what has already been released for the next I build.
(33) The javadoc for org.eclipse.jdt.core.JavaCore.COMPILER_NONNULL_IS_DEFAULT and user documentation should both be updated to specify that this option has no effect on binary types.
(34) import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; public class X { void foo (@Nullable Object o) { } } class Y extends X { @Override void foo (@NonNull Object o) { } } we report: Illegal redefinition of parameter o, inherited method from X declares this parameter as @Nullable should this instead be: Illegal redefinition of parameter o, supertype method from X declares this parameter as @Nullable (35) The following program should not compile, but does. import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; class X { public void foo(@Nullable @NonNull Object o) { } }
(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(); } }
(In reply to comment #26) > Illegal redefinition of parameter o, inherited method from X declares this > parameter as @Nullable > > should this instead be: > > Illegal redefinition of parameter o, supertype method from X declares this > parameter as @Nullable I did consider this too but looking at existing messages in messages.properties, I concluded Stephan's choice as valid. We've always used "inherited method from...". Anyway, if we chose to go with Srikanth's suggestion, I guess its better worded as "supertype method in X.." > > (35) The following program should not compile, but does. > > import org.eclipse.jdt.annotation.NonNull; > import org.eclipse.jdt.annotation.Nullable; > > class X { > public void foo(@Nullable @NonNull Object o) { > } > } Semantically this code is ok. If both @Nullable and @NonNull are marker annotations applicable to parameters I should be able to use them together like this. So instead of giving a compiler error, maybe we can give a null analaysis specific error/warning when two different "null annotations" are applied at the same element.
(In reply to comment #28) > > (35) The following program should not compile, but does. > > > > import org.eclipse.jdt.annotation.NonNull; > > import org.eclipse.jdt.annotation.Nullable; > > > > class X { > > public void foo(@Nullable @NonNull Object o) { > > } > > } > > Semantically this code is ok. If both @Nullable and @NonNull are marker > annotations applicable to parameters I should be able to use them together like > this. So instead of giving a compiler error, maybe we can give a null analaysis > specific error/warning when two different "null annotations" are applied at > the same element. Exactly, it's legal Java but makes no sense for null analysis. I filed bug 365662 for this.
(In reply to 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(); > } > } I had briefly thought about this and than forget to implement it. Thanks for mentioning. I put it into the same bucket as item (35): bug 365662.
(In reply to comment #25) > (33) The javadoc for org.eclipse.jdt.core.JavaCore.COMPILER_NONNULL_IS_DEFAULT > and user documentation should both be updated to specify that this option > has no effect on binary types. OK. Summarizing items post comment 22: (33) post M4, put into bucket bug 365519 (34) undecided between comment 26 and comment 28 (35) + (37): post M4, bug 365662 (36): this item - errh - oops, where is it? :) --- I'm investigating item (25) now.
Created attachment 207952 [details] tests & implementation for item (25) (In reply to comment #24) > (In reply to comment #18) > > Plus possibly: (25), (26) > > TODO > > OK, we can consider this one too, if a patch comes in time. > Jay - FYI, please include what has already been released for > the next I build. OK, here you go. I hope we still have sufficient time for a review of this patch. (In reply to comment #17) > (25) org.eclipse.jdt.internal.compiler.ast.Statement.analyseArguments: > Will it work well with varargs methods ? > for (int i = 0; i < arguments.length; i++) { > if (methodBinding.parameterNonNullness[i] == Boolean.TRUE) { > TypeBinding expectedType = methodBinding.parameters[i]; > Expression argument = arguments[i]; > int nullStatus = argument.nullStatus(flowInfo); // slight loss > of precision: should also use the null info from the receiver. > if (nullStatus != FlowInfo.NON_NULL) // if required non-null is > not provided > flowContext.recordNullityMismatch(currentScope, argument, > nullStatus, expectedType); > } > } > This loop above assumes arguments and parameters can be indexed lock step with > each other. Indeed, varargs pose a problem. Without a fix the attached regression tests throw AIOOBE. The patch adds explicit handling of varargs to this method. > (26) If 25 is an issue, check arg varargs index safety everywhere - Devise > regression tests > expressly for this purpose. Fortunately, I'm routing all analyses of method/ctor call sites through this one method Statement.analyseArguments(..), thus I believe this is only location affected by this issue. Tests cover message sends, (qualified) allocation expressions, and explicit constructor calls (super and this). All JDT/Core tests pass with this patch. Actually, on first run I saw 4 failures, which, however, were not repeatable: org.eclipse.jdt.core.tests.model.AttachSourceTests.testExternalFolder1() org.eclipse.jdt.core.tests.model.AttachSourceTests.testZIPArchive1() org.eclipse.jdt.core.tests.model.JavaElementDeltaTests.testAddExternalLibFolder1() org.eclipse.jdt.core.tests.model.JavaElementDeltaTests.testAddExternalLibFolder2() My imagination fails me in drawing a connection between the patch and these failures. More likely some heavy disk-activity during the test run disturbed these tests? As said, when re-running these tests were green again.
(In reply to comment #28) > > class X { > > public void foo(@Nullable @NonNull Object o) { > > } > > } > > Semantically this code is ok. If both @Nullable and @NonNull are marker > annotations applicable to parameters I should be able to use them together like > this. Nitpicking: You meant syntactically it is OK. Semantically it is garbage which is why we should issue a diagnostic.
(In reply to comment #28) > (In reply to comment #26) > > Illegal redefinition of parameter o, inherited method from X declares this > > parameter as @Nullable > > > > should this instead be: > > > > Illegal redefinition of parameter o, supertype method from X declares this > > parameter as @Nullable > I did consider this too but looking at existing messages in > messages.properties, I concluded Stephan's choice as valid. We've always used > "inherited method from...". > > Anyway, if we chose to go with Srikanth's suggestion, I guess its better worded > as "supertype method in X.." Personally I prefer the term "inherited method" when the current type does not override the method and prefer the terms "overriding method" and "overridden method" when the current type overrides super type method. A quick scan shows several messages using the phrase "inherited method". I don't know that these are used uniformly - so I am OK with leaving this message as it is.
(In reply to comment #32) > Created attachment 207952 [details] > tests & implementation for item (25) > OK, here you go. > I hope we still have sufficient time for a review of this patch. A good part of the day got washed out due to some git issues - though I was not directly involved in the solution. I'll take a look at it tomorrow and if we have an opportunity we can release this, though we won't ask for a build expressly for this. [...] > My imagination fails me in drawing a connection between the patch and these > failures. More likely some heavy disk-activity during the test run > disturbed these tests? As said, when re-running these tests were green again. Satyam ran all tests again with this patch and there were no issues.
What should be the right behavior here: import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; // @NonNullByDefault public class X { @NonNullByDefault public void foo(@Nullable String [] args) { class local { void zoo(Object o) { } }; new local().zoo(null); // No error with defaults applying to foo } }
(In reply to comment #36) > What should be the right behavior here: I have raised bug 365836 to track this.
"Redundant null check: The variable f cannot be null at this location" Do we want to introduce a variant of this message for a @NonNull annotated local ? The phrase: "at this location" is redundant here as the variable cannot be null at any location. (IMO it is OK either way)
(In reply to comment #38) > "Redundant null check: The variable f cannot be null at this location" > > Do we want to introduce a variant of this message for a @NonNull annotated > local ? The phrase: "at this location" is redundant here as the variable > cannot be null at any location. (IMO it is OK either way) Makes sense. Should be no problem to make that distinction. I filed bug 365859 for this.
(In reply to comment #35) > (In reply to comment #32) > > Created attachment 207952 [details] [details] > > tests & implementation for item (25) > > > OK, here you go. > > I hope we still have sufficient time for a review of this patch. > > A good part of the day got washed out due to some git issues - though > I was not directly involved in the solution. I'll take a look at it > tomorrow and if we have an opportunity we can release this, though > we won't ask for a build expressly for this. So this one fell victim to the time sink daemon? It's an AIOOBE, but null-annotating varargs methods is probably not the first thing everybody will do with M4. Well, we still have a few hours until today's 18:00 I-Build :) I should probably mention one conceptual issue that is surfaced by this patch: we haven't yet a way to say whether "@NonNull Object[]" means a non-null array of Object or an array of non-null Object. I know of existing work on this problem but I hadn't yet found the time to investigate in detail (would JSR 308 help here??). Still, despite that conceptual issue, the patch should be the best we can do right now: avoid AIOOBE and do a best effort analysis for var args: Read "@NonNull Object ... o" as any number of "@NonNull Object" arguments. MessageSends will be appropriately checked against this interpretation. Only inside the method when encountering expressions like "o[1]" we're currently out of luck.
(In reply to comment #40) > (In reply to comment #35) > > (In reply to comment #32) > > > Created attachment 207952 [details] [details] [details] > > > tests & implementation for item (25) > > > > > OK, here you go. > > > I hope we still have sufficient time for a review of this patch. > > > > A good part of the day got washed out due to some git issues - though > > I was not directly involved in the solution. I'll take a look at it > > tomorrow and if we have an opportunity we can release this, though > > we won't ask for a build expressly for this. > > So this one fell victim to the time sink daemon? > It's an AIOOBE, but null-annotating varargs methods is probably not the > first thing everybody will do with M4. I am sorry we ran out of time on this. I have spawned bug 365983 and added a reference to the patch with fix and tests from here. Let us have a distinct bugzilla entry opened for every clear cut defect discovered during testing so that each issue can be tracked individually for verification purpose, the current one can be used for discussions, questions on gray areas etc. At this point only the varargs fix and the alternate strategy for propagating null defaults need review. Patches for these could be attached to the respective bugs. For all miscellaneous issues, if released early enough in M5, no review is needed: If you want, you can resort to cumulative testing and release as long as there are comments in code, bugzilla and git commit logs that allow us to recognize the pieces if needed.
(38) We should verify that these methods AbstractMethodDeclaration.validateAnnotations() and all its overrides Statement.analyseArguments Statement.checkAssignmentAgainstNullAnnotation Scope.validateNullAnnotation ReturnStatement.checkAgainstNullAnnotation BinaryTypeBinding.scanMethodForNullAnnotation BinaryTypeBinding.scanTypeForNullAnnotation MethodVerifier15.checkNullSpecInheritance PackageBinding.checkIfNullAnnotationPackage PackageBinding.checkIfNullAnnotationType SourceTypeBinding.evaluateNullAnnotations do not have side effects and if so short circuit the calls with a check for the global master switch being on. Some of these (not all) return quickly if the master switch is off or otherwise some state indicates so, but I think we should eliminate the call overhead itself. There is no point going through an elaborate call sequence, marshalling several arguments only to beat a retreat. This is a win-win from a performance point of view as well as code being better self documenting. In general, if a method is called from several dozens of call sites it would be worth pushing the check into the method and avoid cluttering the numerous call sites, but in the case of these methods, the call sites are just a few, so there is no need to worry about code clutter. (I only cursorily checked that these methods don't have side effects)
(In reply to comment #42) > (38) We should verify that these methods [...] > do not have side effects ... For example, the newly added method AbstractMethodDeclaration.analyseArguments has some side effects and so should not be short circuited.
Couple of questions: - Does Nullable on Local variable make sense? I do see that NonNullByDefault doesn't really apply on local variables. - Similarly NonNullByDefault doesn't have to be retained in the class file. Is it required?
NonNullableByDefault takes an argument value. Isn't value false required to be appropriately mapped in the UI?
(In reply to comment #45) > NonNullableByDefault takes an argument value. Isn't value false required to be > appropriately mapped in the UI? No, this value matters only at the sites where the annotation is applied.
(In reply to comment #44) > Couple of questions: > - Does Nullable on Local variable make sense? I do see that NonNullByDefault > doesn't really apply on local variables. Good question. In fact I don't expect many locals to be specified as Nullable. Yet it is a valid specification and thus I don't see reason to prohibit. It can actually be useful, e.g., if assigning the value from a call to an unannotated method. If the local is unannotated, too, we remain silent about any dereference because nullness state is UNKNOWN. By marking the local as Nullable we say: I think the unannotated method can indeed return null, let's be careful. Then later in the sequence of statements the compiler can remind us of this and flag unchecked dereference with a warning. It's because of the three-valued nature of our null-analysis (nullable- unknown-nonnull) that specifying a local as nullable does make a (small) difference. > - Similarly NonNullByDefault doesn't have to be retained in the class file. Is > it required? I hadn't thought about this, and basically you're right, the effect of NonNullByDefault is expanded into individual synthetic NonNull annotations. Retaining the individual annotations should normally suffice, at least from a compiler POV. This is actually related to item (22) from comment 15, which at a closer look also gives the final answer: for normal types NonNullByDefault need not be retained, but for package-info.java it must be. => This annotation must retain its current @Retention status.
(In reply to comment #46) > (In reply to comment #45) > > NonNullableByDefault takes an argument value. Isn't value false required to be > > appropriately mapped in the UI? > > No, this value matters only at the sites where the annotation is applied. As we don't have another annotation like NullableByDefault which probably another custom annotation could have been using, they will not be able to use this annotation.
Null annotation mapping doesn't seem to allow multiple mappings. If dependent jars use a custom null annotation library, we will not be able to read them. Am I missing anything here?
(In reply to comment #42) > (38) We should verify that these methods > [...] > > do not have side effects and if so short circuit the calls with a check > for the global master switch being on. Moved to bug 365519 comment 8 ff.
(In reply to comment #48) > (In reply to comment #46) > > (In reply to comment #45) > > > NonNullableByDefault takes an argument value. Isn't value false required to be > > > appropriately mapped in the UI? > > > > No, this value matters only at the sites where the annotation is applied. > As we don't have another annotation like NullableByDefault which probably > another custom annotation could have been using, they will not be able to use > this annotation. So by "mapped in the UI" you mean configurable in the preferences? IIRC this came up in the discussion with Markus in Ludwigsburg and we agreed that this one annotation (@NonNullByDefault(false)) is a JDT- specific solution for which we don't see an easy way of providing compatibility with other sets of annotations. Note that most others simply don't have the concept of canceling a default. We preferred the economy of avoiding yet another annotation (e.g., @NullDefaultUnspecified) over the compatibility argument in this particular case.
(In reply to comment #51) > IIRC this came up in the discussion with Markus in Ludwigsburg and we > agreed that this one annotation (@NonNullByDefault(false)) is a JDT- > specific solution for which we don't see an easy way of providing > compatibility with other sets of annotations. > Note that most others simply don't have the concept of canceling a default. > We preferred the economy of avoiding yet another annotation > (e.g., @NullDefaultUnspecified) over the compatibility argument in this > particular case. Thanks for the explanation. This sounds ok.
(In reply to comment #49) > Null annotation mapping doesn't seem to allow multiple mappings. If dependent > jars use a custom null annotation library, we will not be able to read them. Am > I missing anything here? This question has some history, so let's use the time machine: bug 186342 comment 12 (2007-06-22): > As users might even mix different libraries, this could even be a list: > org.eclipse.jdt.core.compiler.notNullAnnotation=org.eclipse.runtime.NotNull;edu.umd.cs.findbugs.annotations.NonNull bug 186342 comment 40 (2010-10-27): > [...] as they are not a standard > I suggest we keep the concept of configurable annotation names > and provide the above as defaults, OK? bug 186342 comment 42 (2010-12-02): > (1) Annotation names are configurable via these keys: > OPTION_NullableAnnotationNames : comma-separated list of qualified > type names that will be recognized as "nullable" annotation. > OPTION_NonNullAnnotationNames : comma-separated list of qualified > type names that will be recognized as "non-null" annotation. bug 186342 comment 43 (2010-12-02): > Should we account for building against different libraries using different > annotation types? One option would be to leave this to the profile > mechanism proposed in bug 331651, which would enable us to use the simpler > model (only one set of annotation names) within the compiler. reply in bug 186342 comment 44 (2010-12-06): > I think this sounds reasonable. For the scenario of this bug, we should just > try and keep things as straightforward as possible bug 186342 comment 45 (2010-12-19): > I no longer support multiple equivalent annotation types, but only one > type for nonnull and one type for nullable (see bug 186342 comment 43). Finally, at ECE2011 Markus and me briefly touched this question and concluded that if desired by users, supporting multiple sets of annotation types can be added via a future RFE.
(In reply to comment #53) Thanks for your patience in replying to this. I got lost in looking at bug 186342 :(.
(In reply to comment #54) > (In reply to comment #53) > Thanks for your patience in replying to this. I got lost in looking at bug > 186342 :(. I fully understand. 200+ comments spanning 4+ years of discussion is a bit overwhelming..
Here's a fresh status checkpoint for everything post comment 31: DONE: + items (25), (26) have moved to bug 365983 + discussion regarding item (34) has terminated in comment 34: no action. + comment 36 has spawned bug 365836. + comment 38 has spawned bug 365859 + item (38) has moved into the bug 365519 bucket (for partial action) Other items could be settled by additional explanations (no action now). TODO: This leaves us with only two items undecided/unassigned: (21) and (22). Neither is critical, for both items my answer will probably start with "yes, but".
(In reply to comment #56) > TODO: > This leaves us with only two items undecided/unassigned: (21) and (22). > > Neither is critical, for both items my answer will probably start > with "yes, but". For (21) let us do the name change now. as the current choice is not appropriate. We can defer the other questions around (21) and all of (22) for later.
(In reply to comment #45) > NonNullableByDefault takes an argument value. Isn't value false required to be > appropriately mapped in the UI? At first reading I thought by "mapped" you meant "rendered". This reminded me that the story of visualization isn't finished yet, see bug 366007.
(In reply to comment #58) > (In reply to comment #45) > > NonNullableByDefault takes an argument value. Isn't value false required to be > > appropriately mapped in the UI? > > At first reading I thought by "mapped" you meant "rendered". So did I and responded with comment# 46, but not sure now.
I've pushed in commit 40ad9904e939ba54abece52421044021edd6e1c0 a fix for item (22). At this point all items raised in this bug are either resolved or have been forked into separate bugs. At the time of this writing these have already been resolved: - bug 365519, bug 365836, bug 365983 while only these are still open: - bug 365662 (target 3.8 M5), bug 365859 (target 3.8) With these results I consider this bug resolved.
(In reply to comment #53) > Finally, at ECE2011 Markus and me briefly touched this question and concluded > that if desired by users, supporting multiple sets of annotation types can > be added via a future RFE. We now have this RFE, its bug 369079.
Verified for 3.8M5 using code inspection
