From 84aabb566f78505828dee93483273c1c2544784d Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 15 Jun 2022 10:59:14 -0400 Subject: [PATCH 1/4] ImmutableChecker handles null types --- .../threadsafety/ImmutableChecker.java | 68 +++++++++++++------ 1 file changed, 49 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java index f33fcf1cef4..6b604e14c43 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java @@ -120,11 +120,15 @@ private ImmutableChecker(ErrorProneFlags flags, ImmutableSet immutableAn @Override public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { - TypeSymbol lambdaType = getType(tree).tsym; + Type type = getType(tree); + if (type == null) { + return NO_MATCH; + } + TypeSymbol lambdaType = type.tsym; ImmutableAnalysis analysis = createImmutableAnalysis(state); Violation info = analysis.checkInstantiation( - lambdaType.getTypeParameters(), getType(tree).getTypeArguments()); + lambdaType.getTypeParameters(), type.getTypeArguments()); if (info.isPresent()) { state.reportMatch(buildDescription(tree).setMessage(info.message()).build()); @@ -147,10 +151,18 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s // check instantiations of `@ImmutableTypeParameter`s in method references checkInvocation(tree, getSymbol(tree), ((JCMemberReference) tree).referentType, state); ImmutableAnalysis analysis = createImmutableAnalysis(state); - TypeSymbol memberReferenceType = targetType(state).type().tsym; + ASTHelpers.TargetType targetType = targetType(state); + if (targetType == null) { + return NO_MATCH; + } + TypeSymbol memberReferenceType = targetType.type().tsym; + Type type = getType(tree); + if (type == null) { + return NO_MATCH; + } Violation info = analysis.checkInstantiation( - memberReferenceType.getTypeParameters(), getType(tree).getTypeArguments()); + memberReferenceType.getTypeParameters(), type.getTypeArguments()); if (info.isPresent()) { state.reportMatch(buildDescription(tree).setMessage(info.message()).build()); @@ -190,11 +202,15 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) { // check instantiations of `@ImmutableTypeParameter`s in generic constructor invocations checkInvocation(tree, getSymbol(tree), ((JCNewClass) tree).constructorType, state); // check instantiations of `@ImmutableTypeParameter`s in class constructor invocations + Type type = getType(tree); + if (type == null) { + return NO_MATCH; + } checkInstantiation( tree, state, getSymbol(tree.getIdentifier()).getTypeParameters(), - ASTHelpers.getType(tree).getTypeArguments()); + type.getTypeArguments()); ClassTree classBody = tree.getClassBody(); @@ -210,11 +226,15 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) { @Override public Description matchMethod(MethodTree tree, VisitorState state) { + Type type = getType(tree); + if (type == null) { + return NO_MATCH; + } checkInstantiation( tree, state, getSymbol(tree).getTypeParameters(), - ASTHelpers.getType(tree).getTypeArguments()); + type.getTypeArguments()); return NO_MATCH; } @@ -336,22 +356,28 @@ public Description matchClass(ClassTree tree, VisitorState state) { private void checkClassTreeInstantiation( ClassTree tree, VisitorState state, ImmutableAnalysis analysis) { for (Tree implementTree : tree.getImplementsClause()) { - checkInstantiation( - tree, - state, - analysis, - getSymbol(implementTree).getTypeParameters(), - ASTHelpers.getType(implementTree).getTypeArguments()); + Type type = getType(implementTree); + if (type != null) { + checkInstantiation( + tree, + state, + analysis, + getSymbol(implementTree).getTypeParameters(), + type.getTypeArguments()); + } } Tree extendsClause = tree.getExtendsClause(); if (extendsClause != null) { - checkInstantiation( - tree, - state, - analysis, - getSymbol(extendsClause).getTypeParameters(), - ASTHelpers.getType(extendsClause).getTypeArguments()); + Type type = getType(extendsClause); + if (type != null) { + checkInstantiation( + tree, + state, + analysis, + getSymbol(extendsClause).getTypeParameters(), + type.getTypeArguments()); + } } } @@ -394,11 +420,15 @@ private Description handleAnonymousClass( // return new ImmutableBox<>(t); // } ImmutableSet typarams = immutableTypeParametersInScope(sym, state, analysis); + ClassType classType = getType(tree); + if (classType == null) { + return NO_MATCH; + } Violation info = analysis.areFieldsImmutable( Optional.of(tree), typarams, - ASTHelpers.getType(tree), + classType, (t, i) -> describeAnonymous(t, superType, i)); if (!info.isPresent()) { return NO_MATCH; From b46d4b89fc9af27cc5eaf85bf5eb5013971ba02e Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 15 Jun 2022 11:59:47 -0400 Subject: [PATCH 2/4] Add ImmutableChecker switch expression test --- .../threadsafety/ImmutableCheckerTest.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java index 864524e5e8e..b51dd6501ce 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java @@ -16,11 +16,14 @@ package com.google.errorprone.bugpatterns.threadsafety; +import static org.junit.Assume.assumeTrue; + import com.google.common.collect.ImmutableList; import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.Immutable; import com.google.errorprone.annotations.concurrent.LazyInit; +import com.google.errorprone.util.RuntimeVersion; import java.util.Arrays; import org.junit.Ignore; import org.junit.Test; @@ -2969,4 +2972,27 @@ public void anonymousClass_canCallSuperMethodOnNonImmutableSuperClass() { "}") .doTest(); } + + @Test + public void switchExpressionsResultingInGenericTypes_doesNotThrow() { + assumeTrue(RuntimeVersion.isAtLeast14()); + compilationHelper + .addSourceLines( + "Kind.java", + "import com.google.errorprone.annotations.Immutable;", + "@Immutable enum Kind { A, B; }") + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import java.util.function.Supplier;", + "class Test {", + " Supplier> test(Kind kind) {", + " return switch (kind) {", + " case A -> Optional::empty;", + " case B -> () -> Optional.of(\"\");", + " };", + " }", + "}") + .doTest(); + } } From b27de53223629bcabaf6ecde41d8e101c7bbd9b9 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Tue, 19 Jul 2022 09:54:37 -0400 Subject: [PATCH 3/4] Handle SwitchExpressionTree in JDK 12+ --- .../google/errorprone/util/ASTHelpers.java | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) 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 7ff84c44ac9..5de1c2b5ab1 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 @@ -1754,14 +1754,30 @@ public Type visitAnnotation(AnnotationTree tree, Void unused) { @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)); + } + + @Nullable + 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; } From dd8f38f24a8f772142fe100fd2ce5760afa732e4 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Sat, 6 Aug 2022 10:51:06 -0400 Subject: [PATCH 4/4] Revert "ImmutableChecker handles null types" This reverts commit 84aabb566f78505828dee93483273c1c2544784d. --- .../threadsafety/ImmutableChecker.java | 68 ++++++------------- 1 file changed, 19 insertions(+), 49 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java index 6b604e14c43..f33fcf1cef4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java @@ -120,15 +120,11 @@ private ImmutableChecker(ErrorProneFlags flags, ImmutableSet immutableAn @Override public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { - Type type = getType(tree); - if (type == null) { - return NO_MATCH; - } - TypeSymbol lambdaType = type.tsym; + TypeSymbol lambdaType = getType(tree).tsym; ImmutableAnalysis analysis = createImmutableAnalysis(state); Violation info = analysis.checkInstantiation( - lambdaType.getTypeParameters(), type.getTypeArguments()); + lambdaType.getTypeParameters(), getType(tree).getTypeArguments()); if (info.isPresent()) { state.reportMatch(buildDescription(tree).setMessage(info.message()).build()); @@ -151,18 +147,10 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s // check instantiations of `@ImmutableTypeParameter`s in method references checkInvocation(tree, getSymbol(tree), ((JCMemberReference) tree).referentType, state); ImmutableAnalysis analysis = createImmutableAnalysis(state); - ASTHelpers.TargetType targetType = targetType(state); - if (targetType == null) { - return NO_MATCH; - } - TypeSymbol memberReferenceType = targetType.type().tsym; - Type type = getType(tree); - if (type == null) { - return NO_MATCH; - } + TypeSymbol memberReferenceType = targetType(state).type().tsym; Violation info = analysis.checkInstantiation( - memberReferenceType.getTypeParameters(), type.getTypeArguments()); + memberReferenceType.getTypeParameters(), getType(tree).getTypeArguments()); if (info.isPresent()) { state.reportMatch(buildDescription(tree).setMessage(info.message()).build()); @@ -202,15 +190,11 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) { // check instantiations of `@ImmutableTypeParameter`s in generic constructor invocations checkInvocation(tree, getSymbol(tree), ((JCNewClass) tree).constructorType, state); // check instantiations of `@ImmutableTypeParameter`s in class constructor invocations - Type type = getType(tree); - if (type == null) { - return NO_MATCH; - } checkInstantiation( tree, state, getSymbol(tree.getIdentifier()).getTypeParameters(), - type.getTypeArguments()); + ASTHelpers.getType(tree).getTypeArguments()); ClassTree classBody = tree.getClassBody(); @@ -226,15 +210,11 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) { @Override public Description matchMethod(MethodTree tree, VisitorState state) { - Type type = getType(tree); - if (type == null) { - return NO_MATCH; - } checkInstantiation( tree, state, getSymbol(tree).getTypeParameters(), - type.getTypeArguments()); + ASTHelpers.getType(tree).getTypeArguments()); return NO_MATCH; } @@ -356,28 +336,22 @@ public Description matchClass(ClassTree tree, VisitorState state) { private void checkClassTreeInstantiation( ClassTree tree, VisitorState state, ImmutableAnalysis analysis) { for (Tree implementTree : tree.getImplementsClause()) { - Type type = getType(implementTree); - if (type != null) { - checkInstantiation( - tree, - state, - analysis, - getSymbol(implementTree).getTypeParameters(), - type.getTypeArguments()); - } + checkInstantiation( + tree, + state, + analysis, + getSymbol(implementTree).getTypeParameters(), + ASTHelpers.getType(implementTree).getTypeArguments()); } Tree extendsClause = tree.getExtendsClause(); if (extendsClause != null) { - Type type = getType(extendsClause); - if (type != null) { - checkInstantiation( - tree, - state, - analysis, - getSymbol(extendsClause).getTypeParameters(), - type.getTypeArguments()); - } + checkInstantiation( + tree, + state, + analysis, + getSymbol(extendsClause).getTypeParameters(), + ASTHelpers.getType(extendsClause).getTypeArguments()); } } @@ -420,15 +394,11 @@ private Description handleAnonymousClass( // return new ImmutableBox<>(t); // } ImmutableSet typarams = immutableTypeParametersInScope(sym, state, analysis); - ClassType classType = getType(tree); - if (classType == null) { - return NO_MATCH; - } Violation info = analysis.areFieldsImmutable( Optional.of(tree), typarams, - classType, + ASTHelpers.getType(tree), (t, i) -> describeAnonymous(t, superType, i)); if (!info.isPresent()) { return NO_MATCH;