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

DLS_DEAD_LOCAL_STORE false-positive in switch #2736

Open
boris-petrov opened this issue Dec 2, 2023 · 9 comments
Open

DLS_DEAD_LOCAL_STORE false-positive in switch #2736

boris-petrov opened this issue Dec 2, 2023 · 9 comments

Comments

@boris-petrov
Copy link

switch (x) {
    case Map<?, ?> m -> return 1;
    default -> return 2;
}

This leads to a DLS_DEAD_LOCAL_STORE error for the m variable. That's a false-positive because Java doesn't allow the variable name to be omitted.

@gtoison
Copy link
Contributor

gtoison commented Dec 17, 2023

This might be solved if this JEP 456: "Unnamed Variables & Patterns" is implemented: https://openjdk.org/jeps/456

@boris-petrov
Copy link
Author

@gtoison yes, that's true, once Java 22 is available, the current behavior will be correct. But until then it isn't. 😄

@gtoison
Copy link
Contributor

gtoison commented Dec 18, 2023

@gtoison yes, that's true, once Java 22 is available, the current behavior will be correct. But until then it isn't. 😄

... and once everybody has switched to Java 22 😄
It also remains to be seen how SpotBugs will analyse the bytecode produced with unamed variables

gtoison added a commit to gtoison/spotbugs that referenced this issue Jan 21, 2024
hazendaz pushed a commit that referenced this issue Feb 3, 2024
#2828)

* test: reproducer for issue #2736

* fix: detect astore instructions corresponding to java 21 type switches

* doc: added changelog entry

* fix: reverted start import change

* fix: ordered imports and removed duplicate
@boris-petrov
Copy link
Author

I tried SpotBugs 4.8.4 which I thought contains the fix for this but it didn't fix any of my cases. Am I missing something or is this still not released?

@gtoison
Copy link
Contributor

gtoison commented Apr 9, 2024

Hum, I thought this was fixed by #2828 ...
The test cases in https://github.com/gtoison/spotbugs/blob/b5893ac7a825413dd6085135318ff7a5a7c49e49/spotbugsTestCases/src/java21/Issue2782.java seem to be handled correctly so maybe your code is a bit different?
What compiler (and version) are you using? I suppose that different compiler might be outputting different bytecode here.

The change in #2828 looks for a CHECKCAST followed by an ASTORE at the beginning of a switch case

@boris-petrov
Copy link
Author

% java --version
openjdk 21.0.2 2024-01-16
OpenJDK Runtime Environment (build 21.0.2+13)
OpenJDK 64-Bit Server VM (build 21.0.2+13, mixed mode, sharing)

As for the code that doesn't work - as I said, none of my cases (around 10 or so) have been fixed... but the original one I've given here seems to work which is strange. 😄 Here are a couple more:

return switch (value) {
	case Map<?, ?> map -> map.size();
	case Set<?> set -> set.size();
	case List<?> list -> list.size();
	case Collection<?> collection ->
		throw new UnsupportedOperationException("Error " + value.getClass());
	case null, default -> 0;
};

Gives the warning on collection.

private static String getValueType(Object value) {
	return switch (value) {
		case List<?> list -> "list";
		case Map<?, ?> map -> "map";
		case Number number -> "number";
		case null, default -> "other";
	};
}

Gives the warning on all list, map and number.

@gtoison
Copy link
Contributor

gtoison commented Apr 10, 2024

Thanks for the samples, I have added them in #2937 and they are flagged indeed
I'd have to investigate why they are not when running the test on my computer

@maxl2287
Copy link

Updating on 4.8.5.0 is removing this DLS for such switches.

@gtoison
Copy link
Contributor

gtoison commented May 29, 2024

You might run into false positives with 4.8.5 because I had not realized that the bytecode instruction for a switch is either LOOKUPSWITCH or TABLESWITCH. Your compiler might use TABLESWITCH in some cases.
In 4.8.5 only LOOKUPSWITCH is handled, once released the fix in #2937 should handle both.

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

No branches or pull requests

3 participants