Skip to content

Commit 184ea87

Browse files
stephan-herrmannstephan-herrmann
authored
Spurious Warnings With Nullable Fields and Null Analysis checking (eclipse-jdt#4718)
+ syntactic analysis now stores known NULL along with known NON_NULL + use FlowContext.INSIDE_NEGATION to switch between NULL & NON_NULL + treat OR_OR_EXPRESSION.right like an else branch Fixes eclipse-jdt#4717
1 parent bc76e39 commit 184ea87

File tree

11 files changed

+207
-27
lines changed

11 files changed

+207
-27
lines changed

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Assignment.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
128128
int timeToLive = (this.bits & InsideExpressionStatement) != 0
129129
? 2 // assignment is statement: make info survives the end of this statement
130130
: 1; // assignment is expression: expire on next event.
131-
flowContext.recordNullCheckedFieldReference((Reference) this.lhs, timeToLive);
131+
flowContext.recordNullCheckedFieldReference((Reference) this.lhs, timeToLive, FlowInfo.NON_NULL);
132132
}
133133
}
134134
}

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ private void checkNullComparison(BlockScope scope, FlowContext flowContext, Flow
5555
// - method/field annotated @NonNull
5656
// - allocation expression, some literals, this reference (see inside expressionNonNullComparison(..))
5757
// these checks do not leverage the flowInfo.
58-
boolean checkEquality = ((this.bits & OperatorMASK) >> OperatorSHIFT) == EQUAL_EQUAL;
58+
boolean checkEquality = ((this.bits & OperatorMASK) >> OperatorSHIFT) == EQUAL_EQUAL
59+
^ ((flowContext.tagBits & FlowContext.INSIDE_NEGATION) != 0);
5960
if ((flowContext.tagBits & FlowContext.HIDE_NULL_COMPARISON_WARNING_MASK) == 0) {
6061
if (leftStatus == FlowInfo.NON_NULL && rightStatus == FlowInfo.NULL) {
6162
leftNonNullChecked = scope.problemReporter().expressionNonNullComparison(this.left, checkEquality);
@@ -64,7 +65,6 @@ private void checkNullComparison(BlockScope scope, FlowContext flowContext, Flow
6465
}
6566
}
6667

67-
boolean contextualCheckEquality = checkEquality ^ ((flowContext.tagBits & FlowContext.INSIDE_NEGATION) != 0);
6868
// perform flowInfo-based checks for variables and record info for syntactic null analysis for fields:
6969
if (!leftNonNullChecked) {
7070
LocalVariableBinding local = this.left.localVariableBinding();
@@ -73,12 +73,13 @@ private void checkNullComparison(BlockScope scope, FlowContext flowContext, Flow
7373
checkVariableComparison(scope, flowContext, flowInfo, initsWhenTrue, initsWhenFalse, local, rightStatus, this.left);
7474
}
7575
} else if (this.left instanceof Reference reference
76-
&& ((contextualCheckEquality ? rightStatus == FlowInfo.NON_NULL : rightStatus == FlowInfo.NULL))
76+
&& (rightStatus == FlowInfo.NON_NULL || rightStatus == FlowInfo.NULL)
7777
&& shouldPerformSyntacticAnalsysisFor(scope, reference))
7878
{
7979
FieldBinding field = reference.lastFieldBinding();
8080
if (field != null && (field.type.tagBits & TagBits.IsBaseType) == 0) {
81-
flowContext.recordNullCheckedFieldReference((Reference) this.left, 1);
81+
int checkedInfo = checkEquality ? rightStatus : FlowInfo.nullInverse(rightStatus);
82+
flowContext.recordNullCheckedFieldReference((Reference) this.left, 1, checkedInfo);
8283
}
8384
}
8485
}
@@ -89,12 +90,13 @@ && shouldPerformSyntacticAnalsysisFor(scope, reference))
8990
checkVariableComparison(scope, flowContext, flowInfo, initsWhenTrue, initsWhenFalse, local, leftStatus, this.right);
9091
}
9192
} else if (this.right instanceof Reference reference
92-
&& ((contextualCheckEquality ? leftStatus == FlowInfo.NON_NULL : leftStatus == FlowInfo.NULL))
93+
&& (leftStatus == FlowInfo.NON_NULL || leftStatus == FlowInfo.NULL)
9394
&& shouldPerformSyntacticAnalsysisFor(scope, reference))
9495
{
9596
FieldBinding field = reference.lastFieldBinding();
9697
if (field != null && (field.type.tagBits & TagBits.IsBaseType) == 0) {
97-
flowContext.recordNullCheckedFieldReference((Reference) this.right, 1);
98+
int checkedInfo = checkEquality ? leftStatus : FlowInfo.nullInverse(leftStatus);
99+
flowContext.recordNullCheckedFieldReference((Reference) this.right, 1, checkedInfo);
98100
}
99101
}
100102
}
@@ -132,7 +134,7 @@ public void syntacticFieldAnalysisForFalseBranch(FlowInfo flowInfo, FlowContext
132134
{
133135
FieldBinding field = ((Reference)this.left).lastFieldBinding();
134136
if (field != null && (field.type.tagBits & TagBits.IsBaseType) == 0) {
135-
flowContext.recordNullCheckedFieldReference((Reference) this.left, 1);
137+
flowContext.recordNullCheckedFieldReference((Reference) this.left, 1, FlowInfo.NON_NULL);
136138
}
137139
}
138140
int leftStatus = this.left.nullStatus(flowInfo, flowContext);
@@ -142,7 +144,7 @@ public void syntacticFieldAnalysisForFalseBranch(FlowInfo flowInfo, FlowContext
142144
{
143145
FieldBinding field = ((Reference)this.right).lastFieldBinding();
144146
if (field != null && (field.type.tagBits & TagBits.IsBaseType) == 0) {
145-
flowContext.recordNullCheckedFieldReference((Reference) this.right, 1);
147+
flowContext.recordNullCheckedFieldReference((Reference) this.right, 1, FlowInfo.NON_NULL);
146148
}
147149
}
148150
}

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/IfStatement.java‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.eclipse.jdt.internal.compiler.codegen.CodeStream;
2525
import org.eclipse.jdt.internal.compiler.flow.FlowContext;
2626
import org.eclipse.jdt.internal.compiler.flow.FlowInfo;
27+
import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
2728
import org.eclipse.jdt.internal.compiler.impl.Constant;
2829
import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
2930
import org.eclipse.jdt.internal.compiler.lookup.FieldBinding;
@@ -72,8 +73,15 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
7273
int initialComplaintLevel = (flowInfo.reachMode() & FlowInfo.UNREACHABLE) != 0 ? Statement.COMPLAINED_FAKE_REACHABLE : Statement.NOT_COMPLAINED;
7374

7475
FieldBinding[] nullCheckedFields = null;
75-
if (currentScope.compilerOptions().isAnnotationBasedResourceAnalysisEnabled && this.condition instanceof EqualExpression) { // simple checks only
76-
nullCheckedFields = flowContext.nullCheckedFields(); // store before info expires
76+
Reference[] nullFields = null;
77+
CompilerOptions options = currentScope.compilerOptions();
78+
if (this.condition instanceof EqualExpression) {
79+
if (options.isAnnotationBasedResourceAnalysisEnabled) { // simple checks only
80+
nullCheckedFields = flowContext.nullCheckedFields(); // store before info expires
81+
}
82+
if (options.isAnnotationBasedNullAnalysisEnabled && options.enableSyntacticNullAnalysisForFields) {
83+
nullFields = flowContext.nullFieldReferences(FlowInfo.NULL); // store (f==null) before info expires
84+
}
7785
}
7886

7987
Constant cst = this.condition.optimizedBooleanConstant();
@@ -103,7 +111,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
103111
// No need if the whole if-else construct itself lies in unreachable code
104112
this.bits |= ASTNode.IsElseStatementUnreachable;
105113
}
106-
boolean reportDeadCodeForKnownPattern = !isKnowDeadCodePattern(this.condition) || currentScope.compilerOptions().reportDeadCodeInTrivialIfStatement;
114+
boolean reportDeadCodeForKnownPattern = !isKnowDeadCodePattern(this.condition) || options.reportDeadCodeInTrivialIfStatement;
107115
if (this.thenStatement != null) {
108116
// Save info for code gen
109117
this.thenInitStateIndex = currentScope.methodScope().recordInitializationStates(thenFlowInfo);
@@ -123,8 +131,16 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
123131
}
124132
// any null check from the condition is now expired
125133
flowContext.expireNullCheckedFieldInfo();
134+
boolean thenExit = (thenFlowInfo.tagBits & FlowInfo.UNREACHABLE_OR_DEAD) != 0;
135+
if (nullFields != null && (this.elseStatement != null || thenExit)) {
136+
// fields known to be null in "then" are safe (a) in "else" or (b) after "if" if then terminates abruptly
137+
for (Reference nullField : nullFields) {
138+
int ttl = this.elseStatement != null ? 1 : 2;
139+
flowContext.recordNullCheckedFieldReference(nullField, ttl, FlowInfo.NON_NULL);
140+
}
141+
}
126142
// code gen: optimizing the jump around the ELSE part
127-
if ((thenFlowInfo.tagBits & FlowInfo.UNREACHABLE_OR_DEAD) != 0) {
143+
if (thenExit) {
128144
this.bits |= ASTNode.ThenExit;
129145
}
130146

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/InstanceOfExpression.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
9292
if (currentScope.compilerOptions().enableSyntacticNullAnalysisForFields) {
9393
FieldBinding field = ((Reference)this.expression).lastFieldBinding();
9494
if (field != null && (field.type.tagBits & TagBits.IsBaseType) == 0) {
95-
flowContext.recordNullCheckedFieldReference((Reference) this.expression, 1);
95+
flowContext.recordNullCheckedFieldReference((Reference) this.expression, 1, FlowInfo.NON_NULL);
9696
}
9797
}
9898
}

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/MessageSend.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,7 @@ private FlowInfo analyseNullAssertion(BlockScope currentScope, Expression argume
498498
{
499499
FieldBinding field = ((Reference)argument).lastFieldBinding();
500500
if (field != null && (field.type.tagBits & TagBits.IsBaseType) == 0) {
501-
flowContext.recordNullCheckedFieldReference((Reference) argument, 3); // survive this assert as a MessageSend and as a Statement
501+
flowContext.recordNullCheckedFieldReference((Reference) argument, 3, FlowInfo.NON_NULL); // survive this assert as a MessageSend and as a Statement
502502
}
503503
}
504504
}

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/OR_OR_Expression.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,13 @@ public FlowInfo analyseCode(
6464
return mergedInfo;
6565
}
6666

67+
boolean oldNegation = (flowContext.tagBits & FlowContext.INSIDE_NEGATION) != 0;
68+
flowContext.tagBits |= FlowContext.INSIDE_NEGATION; // record field nullness from !left for use in right, seen as an else branch
6769
FlowInfo leftInfo = this.left.analyseCode(currentScope, flowContext, flowInfo);
6870
if ((flowContext.tagBits & FlowContext.INSIDE_NEGATION) == 0)
6971
flowContext.expireNullCheckedFieldInfo();
72+
if (!oldNegation)
73+
flowContext.tagBits &= ~FlowContext.INSIDE_NEGATION;
7074

7175
// rightInfo captures the flow (!left then right):
7276
FlowInfo rightInfo = leftInfo.initsWhenFalse().unconditionalCopy();

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Reference.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public boolean checkNPE(BlockScope scope, FlowContext flowContext, FlowInfo flow
5555
protected boolean checkNullableFieldDereference(Scope scope, FieldBinding field, long sourcePosition, FlowContext flowContext, int ttlForFieldCheck) {
5656
if (field != null) {
5757
if (ttlForFieldCheck > 0 && scope.compilerOptions().enableSyntacticNullAnalysisForFields)
58-
flowContext.recordNullCheckedFieldReference(this, ttlForFieldCheck);
58+
flowContext.recordNullCheckedFieldReference(this, ttlForFieldCheck, FlowInfo.NON_NULL);
5959
// preference to type annotations if we have any
6060
if ((field.type.tagBits & TagBits.AnnotationNullable) != 0) {
6161
scope.problemReporter().dereferencingNullableExpression(sourcePosition, scope.environment());

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -696,11 +696,11 @@ else if ((statement.bits & ASTNode.DocumentedFallthrough) == 0) // the case is n
696696
caseInits.markAsDefinitelyNonNull(reference.localVariableBinding());
697697
} else if (reference.lastFieldBinding() != null) {
698698
if (this.scope.compilerOptions().enableSyntacticNullAnalysisForFields)
699-
switchContext.recordNullCheckedFieldReference(reference, 2); // survive this case statement and into the next
699+
switchContext.recordNullCheckedFieldReference(reference, 2, FlowInfo.NON_NULL); // survive this case statement and into the next
700700
}
701701
} else if (this.expression instanceof FieldReference) {
702702
if (this.scope.compilerOptions().enableSyntacticNullAnalysisForFields)
703-
switchContext.recordNullCheckedFieldReference((FieldReference) this.expression, 2); // survive this case statement and into the next
703+
switchContext.recordNullCheckedFieldReference((FieldReference) this.expression, 2, FlowInfo.NON_NULL); // survive this case statement and into the next
704704
}
705705
}
706706
}

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/flow/FlowContext.java‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,12 @@ public class FlowContext implements TypeConstants {
6262
// array to store the provided and expected types from the potential error location (for display in error messages):
6363
public TypeBinding[][] providedExpectedTypes = null;
6464

65-
// record field references known to be non-null
65+
// record field references with known null status
6666
// this array will never shrink, only grow. reset happens by nulling expired entries
67-
// this array grows in lock step with timesToLiveForNullCheckInfo, which controls expiration
67+
// this array grows in lock step with timesToLiveForNullCheckInfo, which controls expiration, and nullCheckInfo
6868
private Reference[] nullCheckedFieldReferences = null;
6969
private int[] timesToLiveForNullCheckInfo = null;
70+
private int[] nullCheckInfo = null; // either FlowInfo.NULL or FlowInfo.NON_NULL
7071

7172
public static final int DEFER_NULL_DIAGNOSTIC = 0x1;
7273
public static final int PREEMPT_NULL_DIAGNOSTIC = 0x2;
@@ -121,6 +122,7 @@ public void copyNullCheckedFieldsFrom(FlowContext other) {
121122
if (fieldReferences != null && fieldReferences.length > 0 && fieldReferences[0] != null) {
122123
this.nullCheckedFieldReferences = other.nullCheckedFieldReferences;
123124
this.timesToLiveForNullCheckInfo = other.timesToLiveForNullCheckInfo;
125+
this.nullCheckInfo = other.nullCheckInfo;
124126
}
125127
}
126128

@@ -129,49 +131,76 @@ public void copyNullCheckedFieldsFrom(FlowContext other) {
129131
*
130132
* @param reference Can be a SingleNameReference, a FieldReference or a QualifiedNameReference resolving to a field
131133
* @param timeToLive control how many expire events are needed to expire this information
134+
* @param info FlowInfo.NON_NULL or FlowInfo.NULL representing the nullness of the given field after a check
132135
*/
133-
public void recordNullCheckedFieldReference(Reference reference, int timeToLive) {
136+
public void recordNullCheckedFieldReference(Reference reference, int timeToLive, int info) {
134137
if (this.nullCheckedFieldReferences == null) {
135138
// first entry:
136139
this.nullCheckedFieldReferences = new Reference[] { reference, null };
137140
this.timesToLiveForNullCheckInfo = new int[] { timeToLive, -1 };
141+
this.nullCheckInfo = new int[] { info, 0 };
138142
} else {
139143
int len = this.nullCheckedFieldReferences.length;
140144
// insert into first empty slot:
141145
for (int i=0; i<len; i++) {
142146
if (this.nullCheckedFieldReferences[i] == null) {
143147
this.nullCheckedFieldReferences[i] = reference;
144148
this.timesToLiveForNullCheckInfo[i] = timeToLive;
149+
this.nullCheckInfo[i] = info;
145150
return;
146151
}
147152
}
148153
// grow arrays:
149154
System.arraycopy(this.nullCheckedFieldReferences, 0, this.nullCheckedFieldReferences=new Reference[len+2], 0, len);
150155
System.arraycopy(this.timesToLiveForNullCheckInfo, 0, this.timesToLiveForNullCheckInfo=new int[len+2], 0, len);
156+
System.arraycopy(this.nullCheckInfo, 0, this.nullCheckInfo=new int[len+2], 0, len);
151157
this.nullCheckedFieldReferences[len] = reference;
152158
this.timesToLiveForNullCheckInfo[len] = timeToLive;
159+
this.nullCheckInfo[len] = info;
153160
}
154161
}
155162

156163
public FieldBinding[] nullCheckedFields() {
157164
if (this.nullCheckedFieldReferences == null)
158165
return Binding.NO_FIELDS;
159166
int len = this.nullCheckedFieldReferences.length;
160-
// insert into first empty slot:
161167
int count = 0;
162168
for (int i=0; i<len; i++) {
163-
if (this.nullCheckedFieldReferences[i] != null)
169+
if (this.nullCheckedFieldReferences[i] != null && this.nullCheckInfo[i] == FlowInfo.NON_NULL)
164170
count++;
165171
}
166172
FieldBinding[] result = new FieldBinding[count];
167173
count = 0;
168174
for (int i=0; i<len; i++) {
169-
if (this.nullCheckedFieldReferences[i] != null)
175+
if (this.nullCheckedFieldReferences[i] != null && this.nullCheckInfo[i] == FlowInfo.NON_NULL)
170176
result[count++] = this.nullCheckedFieldReferences[i].lastFieldBinding();
171177
}
172178
return result;
173179
}
174180

181+
/**
182+
* Answer all field references which are known to have nullness like 'info'.
183+
* @param info either FlowInfo.NULL or FlowInfo.NON_NULL
184+
* @return an array of selected field references or {@code null}
185+
*/
186+
public Reference[] nullFieldReferences(int info) {
187+
if (this.nullCheckedFieldReferences == null)
188+
return null;
189+
int len = this.nullCheckedFieldReferences.length;
190+
int count = 0;
191+
for (int i=0; i<len; i++) {
192+
if (this.nullCheckedFieldReferences[i] != null && this.nullCheckInfo[i] == info)
193+
count++;
194+
}
195+
Reference[] result = new Reference[count];
196+
count = 0;
197+
for (int i=0; i<len; i++) {
198+
if (this.nullCheckedFieldReferences[i] != null && this.nullCheckInfo[i] == info)
199+
result[count++] = this.nullCheckedFieldReferences[i];
200+
}
201+
return result;
202+
203+
}
175204
/** If a null checked field has been recorded recently, increase its time to live. */
176205
public void extendTimeToLiveForNullCheckedField(int t) {
177206
if (this.timesToLiveForNullCheckInfo != null) {
@@ -185,7 +214,7 @@ public void extendTimeToLiveForNullCheckedField(int t) {
185214
* Forget any information about fields that were previously known to be non-null.
186215
*
187216
* Will only cause any effect if CompilerOptions.enableSyntacticNullAnalysisForFields
188-
* (implicitly by guards before calls to {@link #recordNullCheckedFieldReference(Reference, int)}).
217+
* (implicitly by guards before calls to {@link #recordNullCheckedFieldReference(Reference, int, int)}).
189218
*/
190219
public void expireNullCheckedFieldInfo() {
191220
if (this.nullCheckedFieldReferences != null) {
@@ -199,7 +228,7 @@ public void expireNullCheckedFieldInfo() {
199228
/**
200229
* Is the given field reference equivalent to a reference that is freshly known to be non-null?
201230
* Can only return true if CompilerOptions.enableSyntacticNullAnalysisForFields
202-
* (implicitly by guards before calls to {@link #recordNullCheckedFieldReference(Reference, int)}).
231+
* (implicitly by guards before calls to {@link #recordNullCheckedFieldReference(Reference, int, int)}).
203232
*/
204233
public boolean isNullcheckedFieldAccess(Reference reference) {
205234
if (this.nullCheckedFieldReferences == null) // always null unless CompilerOptions.enableSyntacticNullAnalysisForFields
@@ -211,7 +240,7 @@ public boolean isNullcheckedFieldAccess(Reference reference) {
211240
continue;
212241
}
213242
if (checked.isEquivalent(reference)) {
214-
return true;
243+
return this.nullCheckInfo[i] == FlowInfo.NON_NULL;
215244
}
216245
}
217246
return false;

‎org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/flow/FlowInfo.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -680,6 +680,14 @@ public String toString(){
680680
*/
681681
abstract public void resetAssignmentInfo(LocalVariableBinding local);
682682

683+
public static int nullInverse(int status) {
684+
return switch(status) {
685+
case NULL -> NON_NULL;
686+
case NON_NULL -> NULL;
687+
default -> 0;
688+
};
689+
}
690+
683691
/**
684692
* Check whether 'tagBits' contains either {@link TagBits#AnnotationNonNull} or {@link TagBits#AnnotationNullable},
685693
* and answer the corresponding null status ({@link #NON_NULL} etc.).

0 commit comments

Comments
 (0)