-
Notifications
You must be signed in to change notification settings - Fork 130
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.
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
Bogus null contract violation diagnostic at yield statements #3154
Bogus null contract violation diagnostic at yield statements #3154
Conversation
@stephan-herrmann my first contribution to null analysis after 10 years hiatus. Be gentle and kind in review comments 😆 |
|
Is that a linguistic hint? AFAICS there is no doubt about null at the two error locations in the new test, so 'is' is more precise than 'may be'. Do you disagree about this analysis? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome back to null analysis, @srikanth-sankaran :)
You found the location that triggered incorrect behaviour. I hope you find my detailed comment helpful to further improve the change.
if (flowInfo.reachMode() == FlowInfo.REACHABLE && currentScope.compilerOptions().isAnnotationBasedNullAnalysisEnabled) | ||
checkAgainstNullAnnotation(currentScope, flowContext, flowInfo, this.expression); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 return
ed 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
There was a problem hiding this comment.
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.
No, please ignore that comment - I didn't realize we also have a variant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @srikanth-sankaran, for insisting. After more scrutiny I see no problem with this PR. Please go ahead.
What it does
How to test
Author checklist