From a6ab21a1ad985820462d3b631ac369415c9630b3 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 16 Apr 2024 09:36:48 -0700 Subject: [PATCH] Fix a crash in JUnitIncompatibleType Fixes https://github.com/google/error-prone/issues/4291 PiperOrigin-RevId: 625359890 --- .../IgnoringCasts.java | 62 +++++++++++++++++ .../JUnitIncompatibleType.java | 37 ++-------- .../TruthIncompatibleType.java | 69 +++++-------------- .../JUnitIncompatibleTypeTest.java | 18 ++++- 4 files changed, 101 insertions(+), 85 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IgnoringCasts.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IgnoringCasts.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IgnoringCasts.java new file mode 100644 index 00000000000..8e808f7d278 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/IgnoringCasts.java @@ -0,0 +1,62 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.collectionincompatibletype; + +import static com.google.errorprone.util.ASTHelpers.getType; + +import com.google.errorprone.VisitorState; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.TypeCastTree; +import com.sun.source.util.SimpleTreeVisitor; +import com.sun.tools.javac.code.Type; + +/** + * A utility for handling types in cast expressions, shraed by {@link JUnitIncompatibleType} and + * {@link TruthIncompatibleType}. + */ +final class IgnoringCasts { + + /** + * Returns the most specific type of a cast or its operand, for example returns {@code byte[]} for + * both {@code (byte[]) someObject} and {@code (Object) someByteArray}. + */ + static Type ignoringCasts(Tree tree, VisitorState state) { + return new SimpleTreeVisitor() { + @Override + protected Type defaultAction(Tree node, Void unused) { + return getType(node); + } + + @Override + public Type visitTypeCast(TypeCastTree node, Void unused) { + Type castType = getType(node); + Type expressionType = node.getExpression().accept(this, null); + return (castType.isPrimitive() || state.getTypes().isSubtype(castType, expressionType)) + ? castType + : expressionType; + } + + @Override + public Type visitParenthesized(ParenthesizedTree node, Void unused) { + return node.getExpression().accept(this, null); + } + }.visit(tree, null); + } + + private IgnoringCasts() {} +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java index b5d11620085..144af235ed1 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java @@ -17,11 +17,11 @@ package com.google.errorprone.bugpatterns.collectionincompatibletype; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.bugpatterns.collectionincompatibletype.IgnoringCasts.ignoringCasts; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; -import static com.google.errorprone.util.ASTHelpers.getType; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -34,10 +34,6 @@ import com.google.errorprone.util.Signatures; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.ParenthesizedTree; -import com.sun.source.tree.Tree; -import com.sun.source.tree.TypeCastTree; -import com.sun.source.util.SimpleTreeVisitor; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Type.ArrayType; import javax.inject.Inject; @@ -76,14 +72,12 @@ public final class JUnitIncompatibleType extends BugChecker implements MethodInv public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { var arguments = tree.getArguments(); if (ASSERT_EQUALS.matches(tree, state)) { - var typeA = getType(ignoringCasts(arguments.get(arguments.size() - 2))); - var typeB = getType(ignoringCasts(arguments.get(arguments.size() - 1))); + var typeA = ignoringCasts(arguments.get(arguments.size() - 2), state); + var typeB = ignoringCasts(arguments.get(arguments.size() - 1), state); return checkCompatibility(tree, typeA, typeB, state); } else if (ASSERT_ARRAY_EQUALS.matches(tree, state)) { - var typeA = - ((ArrayType) getType(ignoringCasts(arguments.get(arguments.size() - 2)))).elemtype; - var typeB = - ((ArrayType) getType(ignoringCasts(arguments.get(arguments.size() - 1)))).elemtype; + var typeA = ((ArrayType) ignoringCasts(arguments.get(arguments.size() - 2), state)).elemtype; + var typeB = ((ArrayType) ignoringCasts(arguments.get(arguments.size() - 1), state)).elemtype; return checkCompatibility(tree, typeA, typeB, state); } return NO_MATCH; @@ -113,25 +107,4 @@ private Description checkCompatibility( targetTypeName)) .build(); } - - private Tree ignoringCasts(Tree tree) { - return tree.accept( - new SimpleTreeVisitor() { - @Override - protected Tree defaultAction(Tree node, Void unused) { - return node; - } - - @Override - public Tree visitTypeCast(TypeCastTree node, Void unused) { - return getType(node).isPrimitive() ? node : node.getExpression().accept(this, null); - } - - @Override - public Tree visitParenthesized(ParenthesizedTree node, Void unused) { - return node.getExpression().accept(this, null); - } - }, - null); - } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java index 28bc94aa577..109eb1ac61e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/TruthIncompatibleType.java @@ -19,6 +19,7 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.bugpatterns.collectionincompatibletype.AbstractCollectionIncompatibleTypeMatcher.extractTypeArgAsMemberOfSupertype; +import static com.google.errorprone.bugpatterns.collectionincompatibletype.IgnoringCasts.ignoringCasts; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.anyOf; @@ -46,10 +47,7 @@ import com.google.errorprone.util.Signatures; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.ParenthesizedTree; import com.sun.source.tree.Tree; -import com.sun.source.tree.TypeCastTree; -import com.sun.source.util.SimpleTreeVisitor; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.TypeSymbol; import com.sun.tools.javac.code.Type; @@ -202,7 +200,7 @@ private Stream matchEquality(MethodInvocationTree tree, VisitorStat } Type targetType = - getType(ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()))); + ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()), state); Type sourceType = getType(getOnlyElement(tree.getArguments())); if (isNumericType(sourceType, state) && isNumericType(targetType, state)) { return Stream.of(); @@ -219,7 +217,7 @@ private Stream matchIsAnyOf(MethodInvocationTree tree, VisitorState return Stream.empty(); } Type targetType = - getType(ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()))); + ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()), state); return matchScalarContains(tree, targetType, state); } @@ -233,7 +231,7 @@ private Stream matchIsIn(MethodInvocationTree tree, VisitorState st } Type targetType = - getType(ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()))); + ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()), state); Type sourceType = getIterableTypeArg( getType(getOnlyElement(tree.getArguments())), @@ -254,7 +252,7 @@ private Stream matchVectorContains(MethodInvocationTree tree, Visit Type targetType = getIterableTypeArg( getOnlyElement(getSymbol((MethodInvocationTree) receiver).getParameters()).type, - ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments())), + ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()), state), state); Type sourceType = getIterableTypeArg( @@ -276,7 +274,7 @@ private Stream matchArrayContains(MethodInvocationTree tree, Visito Type targetType = getIterableTypeArg( getOnlyElement(getSymbol((MethodInvocationTree) receiver).getParameters()).type, - ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments())), + ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()), state), state); Type sourceType = ((ArrayType) getType(getOnlyElement(tree.getArguments()))).elemtype; return checkCompatibility(getOnlyElement(tree.getArguments()), targetType, sourceType, state); @@ -293,7 +291,7 @@ private Stream matchScalarContains(MethodInvocationTree tree, Visit Type targetType = getIterableTypeArg( getOnlyElement(getSymbol((MethodInvocationTree) receiver).getParameters()).type, - ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments())), + ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()), state), state); return matchScalarContains(tree, targetType, state); } @@ -324,7 +322,7 @@ private Stream matchCorrespondence(MethodInvocationTree tree, Visit Type targetType = getIterableTypeArg( getOnlyElement(getSymbol((MethodInvocationTree) receiver).getParameters()).type, - ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments())), + ignoringCasts(getOnlyElement(((MethodInvocationTree) receiver).getArguments()), state), state); if (targetType == null) { // The target collection may be raw. @@ -368,16 +366,10 @@ private Stream matchMapVectorContains( getOnlyElement(getSymbol((MethodInvocationTree) receiver).getParameters()).type.tsym; Type targetKeyType = extractTypeArgAsMemberOfSupertype( - getType(ignoringCasts(assertee)), - assertionType, - /* typeArgIndex= */ 0, - state.getTypes()); + ignoringCasts(assertee, state), assertionType, /* typeArgIndex= */ 0, state.getTypes()); Type targetValueType = extractTypeArgAsMemberOfSupertype( - getType(ignoringCasts(assertee)), - assertionType, - /* typeArgIndex= */ 1, - state.getTypes()); + ignoringCasts(assertee, state), assertionType, /* typeArgIndex= */ 1, state.getTypes()); Type sourceKeyType = extractTypeArgAsMemberOfSupertype( getType(getOnlyElement(tree.getArguments())), @@ -411,10 +403,7 @@ private Stream matchMapContainsKey(MethodInvocationTree tree, Visit getOnlyElement(getSymbol((MethodInvocationTree) receiver).getParameters()).type.tsym; Type targetKeyType = extractTypeArgAsMemberOfSupertype( - getType(ignoringCasts(assertee)), - assertionType, - /* typeArgIndex= */ 0, - state.getTypes()); + ignoringCasts(assertee, state), assertionType, /* typeArgIndex= */ 0, state.getTypes()); return checkCompatibility( getOnlyElement(tree.getArguments()), targetKeyType, @@ -437,16 +426,10 @@ private Stream matchMapScalarContains( getOnlyElement(getSymbol((MethodInvocationTree) receiver).getParameters()).type.tsym; Type targetKeyType = extractTypeArgAsMemberOfSupertype( - getType(ignoringCasts(assertee)), - assertionType, - /* typeArgIndex= */ 0, - state.getTypes()); + ignoringCasts(assertee, state), assertionType, /* typeArgIndex= */ 0, state.getTypes()); Type targetValueType = extractTypeArgAsMemberOfSupertype( - getType(ignoringCasts(assertee)), - assertionType, - /* typeArgIndex= */ 1, - state.getTypes()); + ignoringCasts(assertee, state), assertionType, /* typeArgIndex= */ 1, state.getTypes()); MethodSymbol methodSymbol = getSymbol(tree); return Streams.mapWithIndex( tree.getArguments().stream(), @@ -494,32 +477,16 @@ private Stream checkCompatibility( .build()); } - private Tree ignoringCasts(Tree tree) { - return tree.accept( - new SimpleTreeVisitor() { - @Override - protected Tree defaultAction(Tree node, Void unused) { - return node; - } - - @Override - public Tree visitTypeCast(TypeCastTree node, Void unused) { - return getType(node).isPrimitive() ? node : node.getExpression().accept(this, null); - } - - @Override - public Tree visitParenthesized(ParenthesizedTree node, Void unused) { - return node.getExpression().accept(this, null); - } - }, - null); - } - private static Type getIterableTypeArg(Type type, Tree onlyElement, VisitorState state) { return extractTypeArgAsMemberOfSupertype( getType(onlyElement), type.tsym, /* typeArgIndex= */ 0, state.getTypes()); } + private static Type getIterableTypeArg(Type type, Type onlyElement, VisitorState state) { + return extractTypeArgAsMemberOfSupertype( + onlyElement, type.tsym, /* typeArgIndex= */ 0, state.getTypes()); + } + private static Type getCorrespondenceTypeArg(Tree onlyElement, VisitorState state) { return extractTypeArgAsMemberOfSupertype( getType(onlyElement), diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java index 93a70246000..df2c0704db3 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java @@ -17,7 +17,6 @@ package com.google.errorprone.bugpatterns.collectionincompatibletype; import com.google.errorprone.CompilationTestHelper; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -100,7 +99,6 @@ public void assertArrayEquals_primitiveOverloadsFine() { .doTest(); } - @Ignore("https://github.com/google/error-prone/issues/4291") @Test public void assertArrayEquals_cast() { compilationHelper @@ -114,4 +112,20 @@ public void assertArrayEquals_cast() { "}") .doTest(); } + + @Test + public void seesThroughCasts() { + compilationHelper + .addSourceLines( + "Test.java", + "import static org.junit.Assert.assertEquals;", + "import static org.junit.Assert.assertNotEquals;", + "class Test {", + " public void test() {", + " // BUG: Diagnostic contains:", + " assertEquals((Object) 1, (Object) 2L);", + " }", + "}") + .doTest(); + } }