Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bogus null contract violation diagnostic at yield statements #3154

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Bogus null contract violation diagnostic at yield statements #3154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9029a38
Pattern matching on sealed classes cannot infer NonNull (JDK 21)
srikanth-sankaran Oct 24, 2024
a687b5f
Additional test that uses a switch expression as return expression
srikanth-sankaran Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ...pse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/YieldStatement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl

flowInfo = this.expression.analyseCode(currentScope, flowContext, flowInfo);
this.expression.checkNPEbyUnboxing(currentScope, flowContext, flowInfo);
if (flowInfo.reachMode() == FlowInfo.REACHABLE && currentScope.compilerOptions().isAnnotationBasedNullAnalysisEnabled)
checkAgainstNullAnnotation(currentScope, flowContext, flowInfo, this.expression);
Comment on lines -58 to -59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you successfully removed a bogus analysis for the price that NPE caused by yield will not be detected at all, right?

I admit that the current code is only useful in the narrow situation of return switch(v) { ... yield x; }

I believe the better course of action would be to identify the expected annotated type to which the yield value will be assigned, without taking the shortcut to the method return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you successfully removed a bogus analysis for the price that NPE caused by yield will not be detected at all, right?

I admit that the current code is only useful in the narrow situation of return switch(v) { ... yield x; }

Hold on, please take a look again!

I added a new test via a follow up commit that proves that a null injected by yield of a switch expression that is the directly returned will be detected: See org.eclipse.jdt.core.tests.compiler.regression.NullAnnotationTests21.testIssue2522_2()

Why would yield require special treatment under return anymore than ternary expression would ?

I believe the better course of action would be to identify the expected annotated type to which the yield value will be assigned, without taking the shortcut to the method return type.

This happens automatically! Just the same way this is complained against:

@NonNull Stuff l = switch (pm) {
					case Stuff s -> s;
					case null -> null; // error: Null type mismatch: required 'PatternMatching.@NonNull Stuff' but the provided value is null
					};

So do you have a snippet that shows a case where we would not detect a null injection due to the change here ?? I

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I forgot about how a switch expression collects result expressions and how various null-checking methods will drill into those result expressions, without the yield expression needing to do anything. The fact that I wrote my previous review comment on a train is only a weak excuse.


targetContext.recordAbruptExit();
targetContext.expireNullCheckedFieldInfo();
Expand Down
112 changes: 112 additions & 0 deletions ...ts.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullAnnotationTests21.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,4 +1132,116 @@ public static void main(String[] args) {
runner.classLibraries = this.LIBS;
runner.runConformTest();
}

// https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2522
// Pattern matching on sealed classes cannot infer NonNull (JDK 21)
public void testIssue2522() {
Runner runner = getDefaultRunner();
runner.testFiles = new String[] {
"PatternMatching.java",
"""
import org.eclipse.jdt.annotation.*;

public sealed interface PatternMatching {

record Stuff() implements PatternMatching {}

@NonNull
static Stuff match(PatternMatching pm, int v) {
if (v == 0) {
Stuff r = switch (pm) {
case Stuff s -> s;
case null -> throw new NullPointerException();
};
return r; // no error here - good
} else if (v == 1) {
Stuff r = switch (pm) {
case Stuff s -> s;
case null -> throw new NullPointerException();
};
return null; // get error here - good
} else if (v == 2) {
Stuff r = switch (pm) {
case Stuff s -> s;
};
return r; // no error here -- good
} else if (v == 3) {
Stuff r = switch (pm) {
case Stuff s -> null; // <<<<<---------------------------- Line 28 - error Why ???
};
return r; // get error here - good
} else if (v == 4) {
Stuff r = switch (pm) {
case Stuff s -> s;
case null -> null; // <<<-------------------------------- Line 34 - error Why ??
};
return new Stuff(); // no error here // good
}
return new Stuff();
}
}
"""
};
runner.expectedCompilerLog =
"""
----------
1. ERROR in PatternMatching.java (at line 20)
return null; // get error here - good
^^^^
Null type mismatch: required 'PatternMatching.@NonNull Stuff' but the provided value is null
----------
2. ERROR in PatternMatching.java (at line 30)
return r; // get error here - good
^
Null type mismatch: required 'PatternMatching.@NonNull Stuff' but the provided value is null
----------
""";
runner.runNegativeTest();
}

// https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2522
// Pattern matching on sealed classes cannot infer NonNull (JDK 21)
public void testIssue2522_2() {
Runner runner = getDefaultRunner();
runner.testFiles = new String[] {
"PatternMatching.java",
"""
import org.eclipse.jdt.annotation.*;

public sealed interface PatternMatching {

record Stuff() implements PatternMatching {}

@NonNull
static Stuff match(PatternMatching pm, int v) {
if (v == 0) {
return switch (pm) {
case Stuff s -> s;
case null -> throw new NullPointerException();
}; // no error here - good
} else if (v == 2) {
return switch (pm) {
case Stuff s -> s;
}; // no error here -- good
} else if (v == 3) {
return switch (pm) {
case Stuff s -> null; // get error here - good
};
}
return new Stuff();
}
}
"""
};
runner.expectedCompilerLog =
"""
----------
1. ERROR in PatternMatching.java (at line 20)
case Stuff s -> null; // get error here - good
^^^^
Null type mismatch: required 'PatternMatching.@NonNull Stuff' but the provided value is null
----------
""";
runner.runNegativeTest();
}
}
Loading