8000 fix for null analysis issue for conditional expressions by mpalat · Pull Request #4028 · eclipse-jdt/eclipse.jdt.core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix for null analysis issue for conditional expressions #4028

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
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix for null analysis issue for conditional expressions #4028

wants to merge 4 commits into from
+549 −5

Conversation

mpalat
Copy link
Contributor
@mpalat mpalat commented May 21, 2025 •
edited
Loading

What it does

fixes #3990

Null in Conditional Expressions are now reported with this fix.
General algorithm:
At EqualExpression, check for conditional expression in checkNullComparison and then the logic followed is similar to the flow analysis of checkVariableComparison. Additional error code of 990 error code [IProblem.RedundantNullCheckNullValueExpression] is introduced - see below as well.

A few points:

  • 671 [IProblem.RedundantNullCheckOnNonNullExpression] has been reused in the fix but could not use 672 as it meant NullExpressionReference though the error message is similar to the newly introduced 990 error code [IProblem.RedundantNullCheckNullValueExpression]
  • Have not optimised the code - or done much common method refactorings - atleast for the first round of PR for the sake of clarity.

How to test

Test cases are part of the PR

Author checklist

@mpalat mpalat requested a review from stephan-herrmann May 21, 2025 09:20
@mpalat
Copy link
Contributor Author
mpalat commented May 21, 2025 •
edited
Loading

@stephan-herrmann - can you please review when you get time? TIA.

@stephan-herrmann
Copy link
Contributor

At a very brief look from a distance I was a bit surprised about the size of the change, but maybe all this is needed, so...

Two things you might want to check:

  • Does ConditionalExpression.nullStatus provide sufficient information right when analysing the EqualExpression? Would using that simplify the implementation?
  • OTOH, seeing changes in FlowContext lets me think, yes perhaps those are necessary, but to prove the relevance of FlowContext it would be great to have test cases where this expression is embedded in some control structure like a loop. Think of a loop where a statement after that if statement assigns a non-null value so the null check actually isn't redundant (in a subsequent iteration of the loop)! Does your implementation handle this correctly?

@mpalat
Copy link
Contributor Author
mpalat commented May 29, 2025
  • OTOH, seeing changes in FlowContext lets me think, yes perhaps those are necessary, but to prove the relevance of FlowContext it would be great to have test cases where this expression is embedded in some control structure like a loop. Think of a loop where a statement after that if statement assigns a non-null value so the null check actually isn't redundant (in a subsequent iteration of the loop)! Does your implementation handle this correctly?

Thanks @stephan-herrmann for the input. The following gives an erroneous message - investigating the same:

	int bar() {
		int ret = 0; 
		String s = null;

		for (int i = 0; i < 2; i++) {
			if ((test() ? s : null) == null) // wrong null warning here
				ret = 1;
			s = "hello"; 
		}
		return ret;
	}

@mpalat
Copy link
Contributor Author
mpalat commented Jun 18, 2025 •
edited
Loading

This doesn't flag the "definitely null" for the conditional inside a loop yet
`
int foo3() {
int ret = 0;
String s = null;

	for (int i = 0; i < 2; i++) {
		if (s == null) //  null error is flagged
			ret = 2;

		if ((test() ? s : null) ==  null) // Flag null error - it's not flagged now.
			ret = 1;
	}	   
	return ret;    
}

`

@mpalat mpalat self-assigned this Jun 23, 2025
@mpalat
Copy link
Contributor Author
mpalat commented Jun 23, 2025

Think of a loop where a statement after that if statement assigns a non-null value so the null check actually isn't redundant (in a subsequent iteration of the loop)! Does your implementation handle this correctly?

@stephan-herrmann - This particular case is corrected .However, the code here doesn't flag a null error in the conditional expression. Looking through the LoopingFlowContext.checkOnDeferredNullChecks(), I see that each variable is checked for nullity and at that point the null error is flagged in the case of if statement. Investigating whether its pragmatic (it is possible for sure) to propagate this nullity property upwards to the parent as appropriate (in situations similar to that of conditional expression involving variables).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephan-herrmann stephan-herrmann Awaiting requested review from stephan-herrmann

Assignees

@mpalat mpalat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug 195757 - Missing redundant null check in if condition through ternary operator
2 participants
@mpalat @stephan-herrmann
0