Bug 400421 - [compiler] Null analysis for fields does not take

@com.google.inject.Inject into account

5 files changed, 168 insertions, 4 deletions

diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullAnnotationTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullAnnotationTest.java
index 2b671b7958..33c6bf7404 100644
--- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullAnnotationTest.java
+++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullAnnotationTest.java
@@ -21,6 +21,31 @@ import org.eclipse.jdt.core.JavaCore;
 // see bug 186342 - [compiler][null] Using annotations for null checking
 public class NullAnnotationTest extends AbstractNullAnnotationTest {
 
+private static final String GOOGLE_INJECT_NAME = "com/google/inject/Inject.java";
+private static final String GOOGLE_INJECT_CONTENT = 
+	"package com.google.inject;\n" + 
+	"import static java.lang.annotation.ElementType.*;\n" + 
+	"import java.lang.annotation.Retention;\n" + 
+	"import static java.lang.annotation.RetentionPolicy.RUNTIME;\n" + 
+	"import java.lang.annotation.Target;\n" +
+	"@Target({ METHOD, CONSTRUCTOR, FIELD })\n" + 
+	"@Retention(RUNTIME)\n" + 
+	"public @interface Inject {\n" + 
+	"\n" + 
+	"  boolean optional() default false;\n" + 
+	"}";
+
+private static final String JAVAX_INJECT_NAME = "javax/inject/Inject.java";
+private static final String JAVAX_INJECT_CONTENT = 
+	"package javax.inject;\n" + 
+	"import static java.lang.annotation.ElementType.*;\n" + 
+	"import java.lang.annotation.Retention;\n" + 
+	"import static java.lang.annotation.RetentionPolicy.RUNTIME;\n" + 
+	"import java.lang.annotation.Target;\n" +
+	"@Target({ METHOD, CONSTRUCTOR, FIELD })\n" + 
+	"@Retention(RUNTIME)\n" + 
+	"public @interface Inject {}\n";
+
 public NullAnnotationTest(String name) {
 	super(name);
 }
@@ -4135,6 +4160,87 @@ public void test_nonnull_field_14b() {
 		"");
 }
 
+// A @NonNull field is assumed to be initialized by the injection framework
+// [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
+// https://bugs.eclipse.org/bugs/show_bug.cgi?id=400421
+public void test_nonnull_field_15() {
+	runConformTestWithLibs(
+		new String[] {
+			GOOGLE_INJECT_NAME,
+			GOOGLE_INJECT_CONTENT,
+			"X.java",
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"import com.google.inject.Inject;\n" +
+			"public class X {\n" +
+			"    @NonNull @Inject Object o;\n" +
+			"    @NonNullByDefault class Inner {\n" +
+			"        @Inject String s;\n" +
+			"    }\n" +
+			"}\n",
+		},
+		null /*customOptions*/,
+		"");
+}
+
+// Injection is optional, don't rely on the framework
+// [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
+// https://bugs.eclipse.org/bugs/show_bug.cgi?id=400421
+public void test_nonnull_field_16() {
+	runNegativeTestWithLibs(
+		new String[] {
+			GOOGLE_INJECT_NAME,
+			GOOGLE_INJECT_CONTENT,
+			"X.java",
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"import com.google.inject.Inject;\n" +
+			"public class X {\n" +
+			"    @Inject(optional=true) @NonNull Object o;\n" +
+			"    @NonNullByDefault class Inner {\n" +
+			"        @Inject(optional=true) String s;\n" +
+			"        @Inject(optional=false) String t;\n" + // don't complain here
+			"    }\n" +
+			"}\n",
+		},
+		null /*customOptions*/,
+		"----------\n" + 
+		"1. ERROR in X.java (at line 4)\n" + 
+		"	@Inject(optional=true) @NonNull Object o;\n" + 
+		"	                                       ^\n" + 
+		"The @NonNull field o may not have been initialized\n" + 
+		"----------\n" + 
+		"2. ERROR in X.java (at line 6)\n" + 
+		"	@Inject(optional=true) String s;\n" + 
+		"	                              ^\n" + 
+		"The @NonNull field s may not have been initialized\n" + 
+		"----------\n");
+}
+
+// Using javax.inject.Inject, slight variations
+// [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
+// https://bugs.eclipse.org/bugs/show_bug.cgi?id=400421
+public void test_nonnull_field_17() {
+	runNegativeTestWithLibs(
+		new String[] {
+			JAVAX_INJECT_NAME,
+			JAVAX_INJECT_CONTENT,
+			"X.java",
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"import javax.inject.Inject;\n" +
+			"public class X {\n" +
+			"    @NonNull @Inject static String s; // warn since injection of static field is less reliable\n" + // variation: static field
+			"    @NonNull @Inject @Deprecated Object o;\n" +
+			"    public X() {}\n" + // variation: with explicit constructor
+			"}\n",
+		},
+		null /*customOptions*/,
+		"----------\n" + 
+		"1. ERROR in X.java (at line 4)\n" + 
+		"	@NonNull @Inject static String s; // warn since injection of static field is less reliable\n" + 
+		"	                               ^\n" + 
+		"The @NonNull field s may not have been initialized\n" + 
+		"----------\n");
+}
+
 // access to a nullable field - field reference
 // https://bugs.eclipse.org/bugs/show_bug.cgi?id=331649
 public void test_nullable_field_1() {
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ConstructorDeclaration.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ConstructorDeclaration.java
index 5378bf797a..1338801712 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ConstructorDeclaration.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/ConstructorDeclaration.java
@@ -16,6 +16,7 @@
  *								bug 383690 - [compiler] location of error re uninitialized final field should be aligned
  *								bug 331649 - [compiler][null] consider null annotations for fields
  *								bug 383368 - [compiler][null] syntactic null analysis for field references
+ *								bug 400421 - [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
  *******************************************************************************/
 package org.eclipse.jdt.internal.compiler.ast;
 
@@ -189,10 +190,12 @@ public void analyseCode(ClassScope classScope, InitializationFlowContext initial
 									? (ASTNode) this.scope.referenceType().declarationOf(field.original())
 									: this);
 					} else if (field.isNonNull()) {
+						FieldDeclaration fieldDecl = this.scope.referenceType().declarationOf(field.original());
+						if (!isValueProvidedUsingAnnotation(fieldDecl))
 							this.scope.problemReporter().uninitializedNonNullField(
 								field,
 								((this.bits & ASTNode.IsDefaultConstructor) != 0) 
-									? (ASTNode) this.scope.referenceType().declarationOf(field.original())
+									? (ASTNode) fieldDecl
 									: this);
 					}
 				}
@@ -208,6 +211,29 @@ public void analyseCode(ClassScope classScope, InitializationFlowContext initial
 	}
 }
 
+boolean isValueProvidedUsingAnnotation(FieldDeclaration fieldDecl) {
+	// a member field annotated with @Inject is considered to be initialized by the injector 
+	if (fieldDecl.annotations != null) {
+		int length = fieldDecl.annotations.length;
+		for (int i = 0; i < length; i++) {
+			Annotation annotation = fieldDecl.annotations[i];
+			if (annotation.resolvedType.id == TypeIds.T_JavaxInjectInject) {
+				return true; // no concept of "optional"
+			} else if (annotation.resolvedType.id == TypeIds.T_ComGoogleInjectInject) {
+				MemberValuePair[] memberValuePairs = annotation.memberValuePairs();
+				if (memberValuePairs == Annotation.NoValuePairs)
+					return true;
+				for (int j = 0; j < memberValuePairs.length; j++) {
+					// if "optional=false" is specified, don't rely on initialization by the injector:
+					if (CharOperation.equals(memberValuePairs[j].name, TypeConstants.OPTIONAL))
+						return memberValuePairs[j].value instanceof FalseLiteral;
+				}
+			}
+		}
+	}
+	return false;
+}
+
 /**
  * Bytecode generation for a constructor
  *
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java
index 64a24e0222..20a162b977 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java
@@ -20,6 +20,7 @@
  *								bug 388281 - [compiler][null] inheritance of null annotations as an option
  *								bug 395002 - Self bound generic class doesn't resolve bounds properly for wildcards for certain parametrisation.
  *								bug 392862 - [1.8][compiler][null] Evaluate null annotations on array types
+ *								bug 400421 - [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
  *      Jesper S Moller - Contributions for
  *								bug 382701 - [1.8][compiler] Implement semantic analysis of Lambda expressions & Reference expression
  *******************************************************************************/
@@ -389,10 +390,22 @@ public void computeId() {
 	switch (this.compoundName.length) {
 
 		case 3 :
-			if (!CharOperation.equals(TypeConstants.JAVA, this.compoundName[0]))
-				return;
+			char[] packageName = this.compoundName[0];
+			// expect only java.*.* and javax.*.*
+			switch (packageName.length) {
+				case 4:
+					if (!CharOperation.equals(TypeConstants.JAVA, packageName))
+						return;
+					break; // continue below ...
+				case 5:
+					if (CharOperation.equals(TypeConstants.JAVAX_ANNOTATION_INJECT_INJECT, this.compoundName))
+						this.id = TypeIds.T_JavaxInjectInject;
+					return;
+				default: return;
+			}
+			// ... at this point we know it's java.*.*
 			
-			char[] packageName = this.compoundName[1];
+			packageName = this.compoundName[1];
 			if (packageName.length == 0) return; // just to be safe
 			char[] typeName = this.compoundName[2];
 			if (typeName.length == 0) return; // just to be safe
@@ -609,6 +622,12 @@ public void computeId() {
 		break;
 
 		case 4:
+			// expect one type from com.*.*.*:
+			if (CharOperation.equals(TypeConstants.COM_GOOGLE_INJECT_INJECT, this.compoundName)) {
+				this.id = TypeIds.T_ComGoogleInjectInject;
+				return;
+			}
+			// otherwise only expect java.*.*.*
 			if (!CharOperation.equals(TypeConstants.JAVA, this.compoundName[0]))
 				return;
 			packageName = this.compoundName[1];
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java
index 8bc923a60e..cfa37cdc07 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java
@@ -15,6 +15,7 @@
  *								bug 349326 - [1.7] new warning for missing try-with-resources
  *								bug 358903 - Filter practically unimportant resource leak warnings
  *								bug 381445 - [compiler][resource] Can the resource leak check be made aware of Closeables.closeQuietly?
+ *								bug 400421 - [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
  *******************************************************************************/
 package org.eclipse.jdt.internal.compiler.lookup;
 
@@ -244,6 +245,14 @@ public interface TypeConstants {
 	char[] RUNTIME = "runtime".toCharArray(); //$NON-NLS-1$
 	char[][] ORG_ECLIPSE_CORE_RUNTIME_ASSERT = new char[][] { ORG, ECLIPSE, CORE, RUNTIME, "Assert".toCharArray()}; //$NON-NLS-1$
 
+	// different @Inject annotations are relevant for @NonNull fields
+	char[] INJECT_PACKAGE = "inject".toCharArray(); //$NON-NLS-1$
+	char[] INJECT_TYPE = "Inject".toCharArray(); //$NON-NLS-1$
+	char[][] JAVAX_ANNOTATION_INJECT_INJECT = new char[][] { JAVAX, INJECT_PACKAGE, INJECT_TYPE };
+	char[][] COM_GOOGLE_INJECT_INJECT = new char[][] {"com".toCharArray(), "google".toCharArray(), INJECT_PACKAGE, INJECT_TYPE }; //$NON-NLS-1$ //$NON-NLS-2$
+	//    detail for the above:
+	char[] OPTIONAL = "optional".toCharArray(); //$NON-NLS-1$
+
 	// Constraints for generic type argument inference
 	int CONSTRAINT_EQUAL = 0;		// Actual = Formal
 	int CONSTRAINT_EXTENDS = 1;	// Actual << Formal
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java
index 5dacf42275..93099291bc 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeIds.java
@@ -16,6 +16,7 @@
  *     							bug 359362 - FUP of bug 349326: Resource leak on non-Closeable resource
  *								bug 186342 - [compiler][null] Using annotations for null checking
  *								bug 358903 - Filter practically unimportant resource leak warnings
+ *								bug 400421 - [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
  *******************************************************************************/
 package org.eclipse.jdt.internal.compiler.lookup;
 
@@ -117,6 +118,9 @@ public interface TypeIds {
 	// java 8
 	final int T_JavaLangFunctionalInterface = 69;
 
+	// new in 3.9 to identify known @Inject annotations
+	final int T_JavaxInjectInject = 69;
+	final int T_ComGoogleInjectInject = 70;
 
 	final int NoId = Integer.MAX_VALUE;