Community
Participate
Working Groups
From bug 365387 comment 18: > (ii) A tested patch with all other changes can be released without review > any time *after* M4 ships, as early as possible. This bug serves as a scratch pad for preparing that editorial patch.
To be covered in this bug: (In reply to bug 365387 comment #14) > 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. (In reply to bug 365387 comment #17) > 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. > > [...] > > (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 ?
I fully agree with most points and am preparing a patch to address these. Just for a few items I'm not planning to take action, these are: (In reply to bug 365387 comment #14) > (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) Just an artifact of how the solution was reached from various experiments. Current state considered safe after all the testing. > (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) Sounds like conflicting goals: uniformity vs. avoiding redundancy for the sake of maintainability. > (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 ??? This change would break the following test cases: - test_nonnull_parameter_003() - test_nonnull_parameter_010() through - test_nonnull_parameter_013() and more. Common theme: an expression is involved that is not a local var reference but still has a known nullStatus. So far we have tests with 'null' literal. I'm adding a test to witness that also sends to a @Nullable method are affected. For those scenarios, we must report immediately because they're not covered by deferred checking (and I don't think the should, because deferred checking is intended only to get better info for locals). > (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 ? I can see 4 test cases depending on this detail: - test_nonnull_var_in_constrol_structure_1() - test_nonnull_var_in_constrol_structure_2() - test_nonnull_var_in_constrol_structure_3() - test_nesting_1() common theme: during analyzing a loop body, local variables are seen in null-state UNKNOWN. By direct reporting this would produce RequiredNonNullButProvidedUnknown, only during the deferred check do we have interesting null information and can differentiate accordingly. I'll comment on item (28) in a separate comment.
Created attachment 207883 [details] possible fix for item 28 One last item worth documenting, perhaps discussing: > (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. I'm attaching a possible patch to follow this suggestion. I didn't follow that road because initialization of type ids for null annotations differs from what we have in computeId(): - names for null annotations are not statically known -> cannot use the same structure of checks as in computeId() -> requires a trivial but extensive change in computeId() (avoid "return") - names need to be retrieved from the compiler options, which are not trivially accessible from computeId() - in fact BinaryTypeBinding has to be changed to make this possible at all. - as an optimization, identification of configured annotation types has been broken up into two parts: (1) identifying the package, (2) identifying types within a recognized package. -> Step (1) is best located in PackageBinding; keeping step (2) in the same class improves locality in this regard. PackageBinding.addType() is a unique door through which all type bindings pass before being used inside the compiler. IMHO the last items outweighs the benefits of keeping things local in computeId(), so I suggest the trivial fix: adding a comment in computeId() to alert that there's another method that also contributes ids.
Two more items that might be worth a closer look: (from bug 365387 comment 3): > (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)
(In reply to comment #2) > > (31) org.eclipse.jdt.internal.compiler.flow.FlowContext.recordNullityMismatch: > > > > Did you mean to have the statements: [...] > > inside the if (expression.localVariableBinding() != null) { // flowContext > > cannot yet handle ... > > block ??? > > This change would break the following test cases: [...] > > (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 ? > > I can see 4 test cases depending on this detail: I see, thanks, (31) and(32) are no-ops then. (In reply to comment #3) > Created attachment 207883 [details] > possible fix for item 28 > IMHO the last items outweighs the benefits of keeping things local in > computeId(), so I suggest the trivial fix: adding a comment in computeId() > to alert that there's another method that also contributes ids. Agreed. Let us add a comment in computeId() and retain what has already been released.
(In reply to comment #2) [...] > > (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) > > Sounds like conflicting goals: uniformity vs. avoiding redundancy for the > sake of maintainability. Yes, but the redundancy predates the current body of work and I suspect was intentionally introduced to minimize memory usage. We don't have to change this - perhaps a one line comment each in FFC and LFC near the declaration of nullLocals to document that some state associated with deferred reporting of null diagnostics resides elsewhere would be welcome.
Adding to this bucket from bug 365387 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.
From bug 365387 comment 42 > (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. Initial findings: > AbstractMethodDeclaration.validateAnnotations() and all its overrides only null-analysis, no side effects. If short-circuited this should be renamed to validateNullAnnotations() > Statement.analyseArguments only null-analysis, no side effects. If short-circuited this should be renamed to analyseArgumentsNullness() only null-analysis, no other side effects. > Scope.validateNullAnnotation only null-analysis, two out of three call sites are already protected, only the call from LocalDeclaration could potentially benefit from short-circuiting. All others only serve for null-analysis, no other side effects: > Statement.checkAssignmentAgainstNullAnnotation > ReturnStatement.checkAgainstNullAnnotation > BinaryTypeBinding.scanMethodForNullAnnotation > BinaryTypeBinding.scanTypeForNullAnnotation > MethodVerifier15.checkNullSpecInheritance > PackageBinding.checkIfNullAnnotationPackage > PackageBinding.checkIfNullAnnotationType > SourceTypeBinding.evaluateNullAnnotations
(In reply to comment #8) [...] > Initial findings: > > > AbstractMethodDeclaration.validateAnnotations() and all its overrides > > only null-analysis, no side effects. > If short-circuited this should be renamed to > validateNullAnnotations() Yes, this came up already as (23) > > Statement.analyseArguments > If short-circuited this should be renamed to > analyseArgumentsNullness() Agreed.
(In reply to comment #8) > From bug 365387 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. One CAVEAT: checking the global master switch doesn't come for free. A typical check would be: if (scope.compilerOptions().isAnnotationBasedNullAnalysisEnabled) .... Looking into compilerOptions() this involves retrieving the compilationUnitScope(). So we need two method calls and one loop to get the options. In the current implementation I have already taken care that normally the easiest and typical way out of a method is checked first. So, e.g., inside AbstractMethodDeclaration.validateAnnotations(): if (this.binding != null && this.binding.parameterNonNullness != null) is actually cheaper than retrieving the master switch! So for some of the methods in the list, the additional performance gains we can squeeze out are from pulling some early-exit checks out of the method and to the call-sites. At that point these checks reveal impl. details that I had preferred to remain inside the respective method but maybe with proper commenting the code will remain readable. For these methods I see definite potential for optimization: > MethodVerifier15.checkNullSpecInheritance > SourceTypeBinding.evaluateNullAnnotations Most others should already be pretty close to a sweet spot (and performance tests didn't complain either).
(In reply to comment #10) > For these methods I see definite potential for optimization: > > MethodVerifier15.checkNullSpecInheritance It was looking at this method usage that got me started on this train of thought. Copy & paste is so easy that we have been practicing that art form without pausing to think whether the result of an expression like this.type.scope.compilerOptions().sourceLevel should be cached. So I would recommend identifying places where we think repeated uses are possible (as in the method verifier case) and apply the caching and short circuiting only there.
(In reply to comment #11) > So I would recommend identifying places where we think repeated uses > are possible I meant where we think repeated uses of the cached global option are possible and also cases where we won't significantly add memory foot print.
I've released commit 45457c087850a9bcb1e2d528c971be662d0a4e96 which fixes all relevant items mentioned in this bug, with the following restrictions: We agreed to WONTFIX items (13), (31) and (32) For (7) I had no idea how to construct a test case. Already the original problem (6) is tricky to trigger, and the syntax in that example cannot easily be changed. I'm postponing item (33) to resolve together with bug 366063 so we can maybe make the outcome of that discussion explicit in the docs, too.
(In reply to comment #10) > For these methods I see definite potential for optimization: > > MethodVerifier15.checkNullSpecInheritance > > SourceTypeBinding.evaluateNullAnnotations I see that the check is present in evaluateNullAnnotations(..) but not in MethodVerifier15.checkNullSpecInheritance(..). Is this intentional?
(In reply to comment #14) > (In reply to comment #10) > > For these methods I see definite potential for optimization: > > > MethodVerifier15.checkNullSpecInheritance > > > SourceTypeBinding.evaluateNullAnnotations > > I see that the check is present in evaluateNullAnnotations(..) but not in > MethodVerifier15.checkNullSpecInheritance(..). Is this intentional? Have you seen this comment inside MethodVerifier15.checkNullSpecInheritance(..)? // precondition: caller has checked whether annotation-based null analysis is enabled.
(In reply to comment #15) > Have you seen this comment inside > MethodVerifier15.checkNullSpecInheritance(..)? Yes, I glossed over it. Sorry. :) Verified for 3.8M5 using code inspection.
