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

Handle yield statement case in ASTHelpers#targetType #3737

Conversation

hisener
Copy link
Contributor

@hisener hisener commented Jan 27, 2023

7504736 fixes the switch expression case, but ImmutableChecker can still throw NullPointerException for the yield statement + method reference case. This PR handles that case, too.

Comment on lines +2997 to +3033
@Test
public void switchExpressionsYieldStatement_doesNotThrow() {
assumeTrue(RuntimeVersion.isAtLeast14());
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.function.Supplier;",
"class Test {",
" String test(String mode) {",
" return switch (mode) {",
" case \"random\" -> {",
" yield \"foo\";",
" }",
" default -> throw new IllegalArgumentException();",
" };",
" }",
"}")
.doTest();
}

@Test
public void switchExpressionsMethodReference_doesNotThrow() {
assumeTrue(RuntimeVersion.isAtLeast14());
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.function.Supplier;",
"class Test {",
" Supplier<Double> test(String mode) {",
" return switch (mode) {",
" case \"random\" -> Math::random;",
" default -> throw new IllegalArgumentException();",
" };",
" }",
"}")
.doTest();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two cases are irrelevant for this change, but I added to make sure NPE is only related to yield AClass::methodReference.

Comment on lines +1727 to 1731
Tree actualTree = null;
if (YIELD_TREE != null && YIELD_TREE.isAssignableFrom(parent.getLeaf().getClass())) {
actualTree = parent.getParentPath().getParentPath().getParentPath().getLeaf();
} else if (CONSTANT_CASE_LABEL_TREE != null
&& CONSTANT_CASE_LABEL_TREE.isAssignableFrom(parent.getLeaf().getClass())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a better way of handling these. 😬

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but I think reflection is the best option we have right now.

I've been wondering if we should try to use mrjars to avoid some of the reflection, I filed an issue to track that idea: #3756

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea! Thanks for filing the issue. 🙇

Copy link
Collaborator

@cushon cushon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

Comment on lines +1727 to 1731
Tree actualTree = null;
if (YIELD_TREE != null && YIELD_TREE.isAssignableFrom(parent.getLeaf().getClass())) {
actualTree = parent.getParentPath().getParentPath().getParentPath().getLeaf();
} else if (CONSTANT_CASE_LABEL_TREE != null
&& CONSTANT_CASE_LABEL_TREE.isAssignableFrom(parent.getLeaf().getClass())) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but I think reflection is the best option we have right now.

I've been wondering if we should try to use mrjars to avoid some of the reflection, I filed an issue to track that idea: #3756

copybara-service bot pushed a commit that referenced this pull request Feb 4, 2023
7504736 fixes the switch expression case, but `ImmutableChecker` can still throw `NullPointerException` for the `yield` statement + method reference case. This PR handles that case, too.

Fixes #3737

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3737 from DataDog:halil.sener/immutable-checker-yield-case 8fd83b5
PiperOrigin-RevId: 507164093
copybara-service bot pushed a commit that referenced this pull request Feb 13, 2023
7504736 fixes the switch expression case, but `ImmutableChecker` can still throw `NullPointerException` for the `yield` statement + method reference case. This PR handles that case, too.

Fixes #3737

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3737 from DataDog:halil.sener/immutable-checker-yield-case 8fd83b5
PiperOrigin-RevId: 507165001
@hisener hisener deleted the halil.sener/immutable-checker-yield-case branch February 13, 2023 19:06
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.

2 participants