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.

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

[Enhanced Switch][Null] Inconsistent nullness propagation #3319

Closed
srikanth-sankaran opened this issue Nov 16, 2024 · 2 comments · Fixed by #3391
Closed

[Enhanced Switch][Null] Inconsistent nullness propagation #3319

srikanth-sankaran opened this issue Nov 16, 2024 · 2 comments · Fixed by #3391
Assignees
Milestone

Comments

@srikanth-sankaran
Copy link
Contributor

Found by code inspection:

With null analysis enabled, we report two warnings in foo(Object) Null comparison always yields false: The variable o cannot be null at this location but none in foo(X) (with redundant null check set to warning)

import org.eclipse.jdt.annotation.NonNull;

public class X {
	static @NonNull Object foo(Object o) {
		switch (o) {
			case String s -> {
				if (o == null) {
					System.out.println("o cannot be null at all!");
				}
				System.out.println();
			}
			default -> {
				if (o == null) {
					System.out.println("o cannot be null at all!");
				}
				System.out.println();	
			}
		}
		return new Object();
	}
	
	static @NonNull Object foo(X o) {
		switch (o) {
			case X s  -> {
				if (o == null) {
					System.out.println("o cannot be null at all!");
				}
				System.out.println(s);
			}
		}
		return new Object();
	}
}

This is because of the incorrect asymmetry in org.eclipse.jdt.internal.compiler.ast.SwitchStatement.analyseCode(BlockScope, FlowContext, FlowInfo) between a regular case and a default.

@srikanth-sankaran srikanth-sankaran self-assigned this Nov 16, 2024
@srikanth-sankaran srikanth-sankaran changed the title [Enhanced Switch][Null] Inconsistent null propagation [Enhanced Switch][Null] Inconsistent nullness propagation Nov 16, 2024
@srikanth-sankaran
Copy link
Contributor Author

This is because of the incorrect asymmetry in org.eclipse.jdt.internal.compiler.ast.SwitchStatement.analyseCode(BlockScope, FlowContext, FlowInfo) between a regular case and a default.

Not entirely sure of this. Need to study in detail.

At the outset it does look like there is some inconsistent behavior. Cause may be different than hypothesized.

srikanth-sankaran added a commit that referenced this issue Nov 26, 2024
* Reimplement resolution, analysis and bookkeeping involved with `CaseStatement` and resolution of `SwitchStatement` for a streamlined and cleaner implementation resulting in readily obvious control flow and minimization of state.
* Report non-constant label expressions during resolution rather than waiting for flow analysis phase.
* Change how duplicate label expressions are reported. Don't blame the whole case, just the duplicate expression; Don't blame the original, just the duplicate.
* Represent qualified enumerators with qualified names internally so to avoid name clashes
* Eliminate the hack to inject synthetic `BreakStatement` in label rule switch blocks. Enhance the code generator to handle these explicitly.
* Simplify the implementation of SwitchStatement.getFallThroughState correcting various issues.
* Eliminate the unnecessary vector `SwitchStatement.constMapping` that is passed to `org.eclipse.jdt.internal.compiler.codegen.CodeStream.tableswitch` - constanMapping[i] == i always!
* Enable various tests in `PrimitiveInPatternsTest.java` that were disabled, fixing broken tests; Remove their disabled cousins from `PrimitiveInPatternsTestSH.java`
* Add a disabled regression test for #3319 (not yet understood/fixed)
* Numerous defect fixes as listed below

* Fixes #3336
* Fixes #3318
* Fixes #3320
* Fixes #3321
* Fixes #3334
* Fixes #3335
* Fixes #3265
* Fixes #3169
* Fixes #3339
* Fixes #3337
* Fixes #3344
srikanth-sankaran added a commit to srikanth-sankaran/eclipse.jdt.core that referenced this issue Dec 4, 2024
@srikanth-sankaran
Copy link
Contributor Author

The first commit from #3382 also this issue - basically the isNullHostile method's wrong result in the presence of total patterns was inhibiting a diagnostic from being reported.

@srikanth-sankaran srikanth-sankaran added this to the 4.35 M1 milestone Dec 4, 2024
srikanth-sankaran added a commit that referenced this issue Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant