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

Fix spurious dead code warning after catching NPE (561565) #1134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

btj
Copy link
Contributor

@btj btj commented Jun 8, 2023

See https://bugs.eclipse.org/bugs/show_bug.cgi?id=561565 .

Author checklist

@btj btj marked this pull request as ready for review June 8, 2023 11:42
@jukzi
Copy link
Contributor

jukzi commented Jan 31, 2024

"Bug 561565 - Non-null analysis incorrectly reports dead code; ignores catching NullPointerException" @stephan-herrmann looks like this is something you could review
@btj is it possible that you add a junit test for it?

@stephan-herrmann
Copy link
Contributor

Hi @btj long time no see! 😄

@stephan-herrmann stephan-herrmann self-assigned this Jan 31, 2024
@stephan-herrmann stephan-herrmann removed their assignment Jan 31, 2024
@btj
Copy link
Contributor Author

btj commented Feb 1, 2024

Hi @stephan-herrmann , good to see you!

@stephan-herrmann
Copy link
Contributor

@btj can you tell me, which test case this is supposed to fix? When I create a unit test from https://bugs.eclipse.org/bugs/show_bug.cgi?id=561565#c0 I get the following unexpected problems:

----------\n
1. ERROR in Foo.java (at line 9)\n
	if (foo == null)\n
	    ^^^\n
Null comparison always yields false: The variable foo cannot be null at this location\n
----------\n
2. WARNING in Foo.java (at line 10)\n
	throw new IllegalArgumentException();\n
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n
Dead code\n
----------\n

Perhaps some other change in JDT broke your fix, or did you look at a different example?

@stephan-herrmann
Copy link
Contributor

To ensure we're on the same page, this is the test I added to NullReferenceTest:

public void testGH1134() {
	runConformTest(
		new String[] {
			"Foo.java",
			"""
			class Foo {

				int x;

				static void foo(Foo foo) {
					try {
						if (foo.x == 0);
					} catch (Throwable t) {}
					if (foo == null)
						throw new IllegalArgumentException();
				}

			}
			"""
		});

@btj
Copy link
Contributor Author

btj commented Feb 1, 2024

Hmm. Yes, this test case was supposed to be fixed by this PR. I guess something else broke it again. Never mind, then! I guess I should close the PR.

@jukzi
Copy link
Contributor

jukzi commented Feb 2, 2024

Hmm. Yes, this test case was supposed to be fixed by this PR. I guess something else broke it again. Never mind, then! I guess I should close the PR.

or better add the test to the PR and fix it :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants