Bogus null contract violation diagnostic at yield statements

[srikanth-sankaran]

What it does

• Fixes [Switch Expression][Null] Bogus null contract violation diagnostic at yield statements #2522

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[srikanth-sankaran]

@stephan-herrmann my first contribution to null analysis after 10 years hiatus. Be gentle and kind in review comments 😆

[srikanth-sankaran]

but the provided value is null reads better as but the provided value may be null but that is nitpicking

[stephan-herrmann]

but the provided value is null reads better as but the provided value may be null but that is nitpicking

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?

[stephan-herrmann]

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.

[stephan-herrmann]

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.

[srikanth-sankaran]

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

[stephan-herrmann]

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.

[srikanth-sankaran]

but the provided value is null reads better as but the provided value may be null but that is nitpicking

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?

No, please ignore that comment - I didn't realize we also have a variant but the provided value is inferred as @Nullable where the expression may be but also may not be null. So we are good.

[stephan-herrmann]

Thanks, @srikanth-sankaran, for insisting. After more scrutiny I see no problem with this PR. Please go ahead.