Community
Participate
Working Groups
REPRODUCING THE PROBLEM: Create a workspace with two java projects. In the Java-Compiler "Errors/Warning" -settings turn on "enabled annotation-based null-analysis". Put the following Class into the first project: import org.eclipse.jdt.annotation.NonNullByDefault; @NonNullByDefault public class Class2 { public Class2(String nonNullArg) { assert nonNullArg != null; } public static Class2 create(String nonNullArg) { return new Class2(nonNullArg); } } Now create this class in the other project, configure the build path settings. public class Class1 { public static Class2 works() { return Class2.create(null); } public static Class2 bug() { return new Class2(null); } } Do a clean build. PROBLEM: If you open Class1 in the editors, you will see two warnings, but in the problems view, there will be only one warning for "works", but no warning in "bug". ANALYSIS: When compiling for the editor, Class2 is accessed via a SourceTypeBinding, and org.eclipse.jdt.internal.compiler.lookup.MethodBinding.parameterNonNullness is initialized in both cases. The compilation for the "problems view" accesses Class2 as BinaryTypeBinding. In BinaryTypeBinding, there is code to inherit the nullness-settings in org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.scanFieldForNullAnnotation(IBinaryField, FieldBinding) and org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.scanTypeForNullDefaultAnnotation(IBinaryType, PackageBinding, BinaryTypeBinding), but not for methods in org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.scanMethodForNullAnnotation(IBinaryMethod, MethodBinding). It still works for the "Class2#create" invocation from "Class1#works", because org.eclipse.jdt.internal.compiler.lookup.MethodBinding.parameterNonNullness is initialized in org.eclipse.jdt.internal.compiler.ast.MessageSend.resolveType(BlockScope) with ImplicitNullAnnotationVerifier. The corresponding code in org.eclipse.jdt.internal.compiler.ast.AllocationExpression.resolveType(BlockScope) seems to be missing. Adding it like in the following patch makes the warnings appear in the problems view. diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/AllocationExpression.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/AllocationExpression.java index 2f4b127..5319d9e 100644 --- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/AllocationExpression.java +++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/AllocationExpression.java @@ -28,6 +28,7 @@ import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants; import org.eclipse.jdt.internal.compiler.codegen.*; import org.eclipse.jdt.internal.compiler.flow.*; +import org.eclipse.jdt.internal.compiler.impl.CompilerOptions; import org.eclipse.jdt.internal.compiler.impl.Constant; import org.eclipse.jdt.internal.compiler.lookup.*; import org.eclipse.jdt.internal.compiler.problem.ProblemReporter; @@ -435,6 +436,11 @@ if (!isDiamond && this.resolvedType.isParameterizedTypeWithActualArguments()) { checkTypeArgumentRedundancy((ParameterizedTypeBinding) this.resolvedType, null, argumentTypes, scope); } + final CompilerOptions compilerOptions = scope.compilerOptions(); + if (compilerOptions.isAnnotationBasedNullAnalysisEnabled && (this.binding.tagBits & TagBits.IsNullnessKnown) == 0) { + new ImplicitNullAnnotationVerifier(compilerOptions.inheritNullAnnotations) + .checkImplicitNullAnnotations(this.binding, null/*srcMethod*/, false, scope); + } return allocationType; }
Here is the commit that removed the inheritance code from BinaryTypeBinding: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=4ac6f89083748b9c4fc37b738ed82ea1a7c9c63b
Thanks for the report, I can reproduce in HEAD of master. Thanks also for digging into the implementation. From a quick glance your analysis hits the nail on the head! Yep, while moving some logic from BinaryTypeBinding.createMethod() to MessageSend.resolveType() I indeed forgot about constructors. This is exactly the kind of bug report we like: small example for reproducing, clear description and even a proposed patch that fixes the issue.
This is a regression which we should also backport to 4.3.1.
Stephan, are we still looking at getting this into SR1? Thanks for looking into this.
Sorry, I was distracted by other business. Till, I'd like to mark you as the author of the commit-to-be, so that your contribution will be given due credit. To prepare the actual commit I have to ask you a few things, noted below. Please let me know to which degree you are willing to meet these requests. For process sake please: - complete and submit the Contributor License Agreement, see http://www.eclipse.org/legal/CLA.php - submit the next version of your patch as an attachment to this bug - before creating the patch, add your name and a reference to this bug to the header comment of each source file touched by you. Please see the existing headers. - after that, please "sign-off" your patch by a comment in this bug like "This contribution complies with http://www.eclipse.org/legal/CoO.php" Technically I'd like to ask you to - check whether class QualifiedAllocationExpression needs the same fix. Ideally, the patch should also contain corresponding unit tests. These would be added to org.eclipse.jdt.core.tests.compiler.regression.NullAnnotationTest from where you may take inspiration how to write such a test. Feel free to either ask for some hints on writing the tests or pass this part on to me. To get this into Kepler SR1 as requested, we should try to get this ready within approx. 10 days. So, Till, do you want to give it a try as outlined above? Thanks!
(In reply to comment #5) > Sorry, I was distracted by other business. > > Till, I'd like to mark you as the author of the commit-to-be, so that your > contribution will be given due credit. > > To prepare the actual commit I have to ask you a few things, noted below. > Please let me know to which degree you are willing to meet these requests. > > For process sake please: > > - complete and submit the Contributor License Agreement, see > http://www.eclipse.org/legal/CLA.php > > - submit the next version of your patch as an attachment to this bug > - before creating the patch, add your name and a reference to this bug to > the header comment of each source file touched by you. > Please see the existing headers. > > - after that, please "sign-off" your patch by a comment in this bug like > "This contribution complies with http://www.eclipse.org/legal/CoO.php" > > > Technically I'd like to ask you to > > - check whether class QualifiedAllocationExpression needs the same fix. I think I can do all of this. > > > Ideally, the patch should also contain corresponding unit tests. These would > be added to org.eclipse.jdt.core.tests.compiler.regression.NullAnnotationTest > from where you may take inspiration how to write such a test. > Feel free to either ask for some hints on writing the tests or pass this > part on to me. I would be great if you would do that because I couldn't find an existing multi-project test scenario in there that can be easily adapted. > > > To get this into Kepler SR1 as requested, we should try to get this ready > within approx. 10 days. Sounds possible > > > So, Till, do you want to give it a try as outlined above? > Thanks!
Created attachment 234469 [details] proposed bugfix
This contribution complies with http://www.eclipse.org/legal/CoO.php
> - complete and submit the Contributor License Agreement, see > http://www.eclipse.org/legal/CLA.php DONE > > - submit the next version of your patch as an attachment to this bug > - before creating the patch, add your name and a reference to this bug to > the header comment of each source file touched by you. > Please see the existing headers. DONE > > - after that, please "sign-off" your patch by a comment in this bug like > "This contribution complies with http://www.eclipse.org/legal/CoO.php" DONE > > > Technically I'd like to ask you to > > - check whether class QualifiedAllocationExpression needs the same fix. You were right, it needed the same fix. > > > Ideally, the patch should also contain corresponding unit tests. These would > be added to org.eclipse.jdt.core.tests.compiler.regression.NullAnnotationTest > from where you may take inspiration how to write such a test. > Feel free to either ask for some hints on writing the tests or pass this > part on to me. As i wrote in my previous response, I would appreciate if you could do that, because i couldn't find an existing multi-project test example in a reasonable amount of time). maybe you can base it on my extended test example: import org.eclipse.jdt.annotation.NonNullByDefault; @NonNullByDefault public class Class2 { public class Class3 { public Class3(String nonNullArg) { assert nonNullArg != null; } } public Class2(String nonNullArg) { assert nonNullArg != null; } public static Class2 create(String nonNullArg) { return new Class2(nonNullArg); } } public class Class1 { public static Class2 works() { return Class2.create(null); } public static Class2 bug() { return new Class2(null); } public static Class2.Class3 qualifiedbug() { return new Class2("").new Class3(null); } } > > > To get this into Kepler SR1 as requested, we should try to get this ready > within approx. 10 days. DONE except for new unit tests (but i successfully ran the existing tests with RunJDTCoreTests)
Thanks, Till! I will take this forward from here.
Stephan, I have found another problem related to the inheritance of NonNullByDefault and have prepared a proposed fix in bug 415269. Maybe you want to consider that for the Kepler SR1, too. Anyway, thank you for your great work.
Created attachment 234507 [details] regression test Here's a test that fails without the fix from this bug and passes after applying attachment 234469 [details]
I noticed a slight difference between how the fix is applied to both classes. The difference does no harm for now, it is obviously motivated by existing differences between both classes. I raised bug 415275 for cleanup to improve maintainability for the future. Till, maybe you want to comment in that bug about the difference how you integrated the fix into both classes? The patch looks good to me. Dani, Jay, may I count your previous comments as the required +1 for release to R4_3_mainteance?
(In reply to comment #13) > The patch looks good to me. > Dani, Jay, may I count your previous comments as the required +1 for release > to R4_3_mainteance? Sure.
(In reply to comment #13) > I noticed a slight difference between how the fix is applied to both classes. > The difference does no harm for now, it is obviously motivated by existing > differences between both classes. > I raised bug 415275 for cleanup to improve maintainability for the future. > Till, maybe you want to comment in that bug about the difference how you > integrated the fix into both classes? > > The patch looks good to me. In that case, please set the review+ flag. > Dani, Jay, may I count your previous comments as the required +1 for release > to R4_3_mainteance? Yes.
Released for 4.3.1 via commit 71fa684d4e8a76828182f4efcbe41eab655d0299 (test) commit c005efbda8be0db83712eb14294777155c3ab919 (fix)
Stephan, this needs to be released for Luna as well, right? I am removing the whiteboard content as the target already points to 4.3.1. But when we decide to release it in master, we need to set the whiteboard content appropriately.
(In reply to comment #17) > But when we > decide to release it in master, we need to set the whiteboard content > appropriately. NOTE: The normal process is 1. commit to 'master' 2. wait that it works (at least one good build without test failures) 3. backport
I'm aware I didn't follow the normal process, but considering the tight schedule for the next SR1 candidate and given release for 4.3.1 has been approved I did this first. As I didn't have the time to run all tests against Luna immediately after, I - changed the target to 4.4 M2 - added the whiteboard note "to be verified for 4.3.1" Next I was going to ask Dani why he reverted these two changes :)
(In reply to comment #19) > I'm aware I didn't follow the normal process, but considering the tight > schedule for > the next SR1 candidate and given release for 4.3.1 has been approved I did > this first. > > As I didn't have the time to run all tests against Luna immediately after, I > - changed the target to 4.4 M2 > - added the whiteboard note "to be verified for 4.3.1" > > Next I was going to ask Dani why he reverted these two changes :) The target milestone reflects the oldest build where the fix is present - unless you want to have a bug report for each release where you backport. I see the time pressure in this case and assume you did run the tests against 4.3.1 before committing.
(In reply to comment #20) > The target milestone reflects the oldest build where the fix is present - > unless you want to have a bug report for each release where you backport. Thanks. So, when I release to Luna I will set resolved and set the whiteboard flag "to be verified for 4.4 M2"? > I see the time pressure in this case and assume you did run the tests > against 4.3.1 before committing. Yes, I did.
(In reply to comment #21) > Thanks. So, when I release to Luna I will set resolved and set the > whiteboard flag > "to be verified for 4.4 M2"? Yes.
Released for 4.4 M2 commit f5be514287a14c34eaf46764a8f15d56d8087272 (test) commit e2e97eb1ed318448b72d60aca6f0daa8c5b48408 (fix)
Verified for 4.3.1 with build M20130828-0800
