ImmutableChecker handles null types

[schlosna]

ASTHelpers#getType(ClassTree) and ASTHelpers#targetType(VisitorState) both may return null, which ImmutableChecker did not previously handle safely, which could lead to NullPointerException in cases such as switch expressions. This PR handles these potential null results more safely.

Fixes #3220 & #3225

[schlosna]

I wanted to check in to see if there is anything I can help with to move this PR along. Thanks for error-prone, it is very useful!

[cushon]

Thanks for the fix!

Does the included test case reproduce the crash for you? I'm having trouble seeing the failure when running that test on JDK 17 without the fix applied.

Adding the defensive null-checks and returning is definitely better than crashing, but I was curious if there's another problem here: e.g. maybe ASTHelpers.targetType should be updated to handle new-style switches, or there's an AST node that javac should be attaching type information to but isn't.

[cushon]

Does the included test case reproduce the crash for you?

Sorry, I just saw the CI failure you demonstrated in #3268, I'll take a closer look.

[cushon]

maybe ASTHelpers.targetType should be updated to handle new-style switches

I think this is the culprit:

error-prone/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

Lines 1764 to 1765 in 0da434e

	// TODO(b/176098078): When the ErrorProne project switches to JDK 12, we should check
	// for SwitchExpressionTree.

What do you think about something like this?

diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
index d56da359a..0cdd625f7 100644
--- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
+++ b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
@@ -1755,14 +1755,29 @@ public class ASTHelpers {
     @Nullable
     @Override
     public Type visitCase(CaseTree tree, Void unused) {
-      Tree t = parent.getParentPath().getLeaf();
-      // JDK 12+, t can be SwitchExpressionTree
-      if (t instanceof SwitchTree) {
-        SwitchTree switchTree = (SwitchTree) t;
-        return getType(switchTree.getExpression());
-      }
-      // TODO(b/176098078): When the ErrorProne project switches to JDK 12, we should check
-      // for SwitchExpressionTree.
+      Tree switchTree = parent.getParentPath().getLeaf();
+      return getType(getSwitchExpression(switchTree));
+    }
+
+    private static ExpressionTree getSwitchExpression(Tree tree) {
+      if (tree instanceof SwitchTree) {
+        return ((SwitchTree) tree).getExpression();
+      }
+      // Reflection is required for JDK < 12
+      try {
+        Class<?> switchExpression = Class.forName("com.sun.source.tree.SwitchExpressionTree");
+        Class<?> clazz = tree.getClass();
+        if (switchExpression.isAssignableFrom(clazz)) {
+          try {
+            Method method = clazz.getMethod("getExpression");
+            return (ExpressionTree) method.invoke(tree);
+          } catch (ReflectiveOperationException e) {
+            throw new LinkageError(e.getMessage(), e);
+          }
+        }
+      } catch (ClassNotFoundException e) {
+        // continue below
+      }
       return null;
     }
 

[schlosna]

Hey @cushon thanks for the review and suggestion -- that looks correct to me and I added that commit to the PR.

[schlosna]

Please let me know if there's anything else I can do to help get this merged. Thanks!

[cushon]

Are the null-checks on the result of getTarget still necessary? The attached test case passes for me with just with change to ASTHelpers, without the other null checks.

[schlosna]

@cushon I can revert the changes to ImmutableChecker, as I believe the ASTHelper changes fix the issue at hand.

Would you prefer any asserts or just let things NPE for any future changes (e.g. if something like switch records pattern matching https://openjdk.org/jeps/405 were to break current assumptions)?

[schlosna]

Updated to revert 84aabb5

[cushon]

Would you prefer any asserts or just let things NPE for any future changes

I'm learning towards just letting it crash on unsupported features. It's a bit of a trade-off, and crashing is not great, but the alternative of defensive null-checks and NO_MATCH means it might just silently not check new features, which is potentially worse.

Anyway, thanks for the PR! I'll look at getting this merged.

[schlosna]

Excellent, thanks!