From 229a5b56e95ee1cbd6abe92a0030555fb82e6902 Mon Sep 17 00:00:00 2001 From: Theron Wang Date: Tue, 30 Jul 2024 13:13:00 -0400 Subject: [PATCH] Fully qualify types in generic synthetic method return types (#341) Previously, when a synthetic method with a generic return type had a type parameter in the same package as the target method, it would not keep its fully qualified name in its synthetic definition, leading to compilation errors seen in na-791b. For example, `ImmutableSet.copyOf` returned an `ImmutableSet`, but in its synthetic definition, `copyOf` returned `com.google.common.collect.ImmutableSet` instead of `com.google.common.collect.ImmutableSet`. --- .../specimin/JavaParserUtil.java | 10 +++ .../specimin/MustImplementMethodsVisitor.java | 14 +--- .../specimin/UnsolvedSymbolVisitor.java | 73 ++++++++++++++++++- .../resources/min_program_compile_status.json | 2 +- ...MethodReturnFullyQualifiedGenericTest.java | 21 ++++++ .../specimin/MethodReturnGenericTest.java | 23 ++++++ .../expected/com/bar/Bar.java | 8 ++ .../expected/com/example/Simple.java | 10 +++ .../expected/com/foo/InOtherPackage2.java | 4 + .../input/com/example/Simple.java | 9 +++ .../expected/com/bar/Bar.java | 16 ++++ .../expected/com/example/InSamePackage.java | 4 + .../expected/com/example/Simple.java | 19 +++++ .../expected/com/foo/InOtherPackage.java | 4 + .../input/com/example/Simple.java | 18 +++++ typecheck_one_test.sh | 2 + typecheck_test_outputs.bat | 2 + 17 files changed, 222 insertions(+), 17 deletions(-) create mode 100644 src/test/java/org/checkerframework/specimin/MethodReturnFullyQualifiedGenericTest.java create mode 100644 src/test/java/org/checkerframework/specimin/MethodReturnGenericTest.java create mode 100644 src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/bar/Bar.java create mode 100644 src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/example/Simple.java create mode 100644 src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/foo/InOtherPackage2.java create mode 100644 src/test/resources/methodreturnfullyqualifiedgeneric/input/com/example/Simple.java create mode 100644 src/test/resources/methodreturngeneric/expected/com/bar/Bar.java create mode 100644 src/test/resources/methodreturngeneric/expected/com/example/InSamePackage.java create mode 100644 src/test/resources/methodreturngeneric/expected/com/example/Simple.java create mode 100644 src/test/resources/methodreturngeneric/expected/com/foo/InOtherPackage.java create mode 100644 src/test/resources/methodreturngeneric/input/com/example/Simple.java diff --git a/src/main/java/org/checkerframework/specimin/JavaParserUtil.java b/src/main/java/org/checkerframework/specimin/JavaParserUtil.java index b1aa5445..0962d656 100644 --- a/src/main/java/org/checkerframework/specimin/JavaParserUtil.java +++ b/src/main/java/org/checkerframework/specimin/JavaParserUtil.java @@ -85,6 +85,16 @@ public static ResolvedReferenceType classOrInterfaceTypeToResolvedReferenceType( return type.resolve().asReferenceType(); } + /** + * Erases type arguments from a method signature string. + * + * @param signature the signature + * @return the same signature without type arguments + */ + public static String erase(String signature) { + return signature.replaceAll("<.*>", ""); + } + /** * Returns the corresponding type name for an Expression within an annotation. * diff --git a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java index cfc0a59e..98ffad51 100644 --- a/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java +++ b/src/main/java/org/checkerframework/specimin/MustImplementMethodsVisitor.java @@ -277,12 +277,12 @@ private boolean overridesAnInterfaceMethodImpl( // (i.e., a raw type). Note though that this doesn't mean there are no type variables in the // signature: // add(E) is still add(E). - targetSignature = erase(targetSignature); + targetSignature = JavaParserUtil.erase(targetSignature); for (ResolvedMethodDeclaration methodInInterface : resolvedInterface.getAllMethodsVisibleToInheritors()) { try { - if (erase(methodInInterface.getSignature()).equals(targetSignature)) { + if (JavaParserUtil.erase(methodInInterface.getSignature()).equals(targetSignature)) { if (methodInInterface.isAbstract()) { // once we've found the correct method, we return to whether we // control it or not. If we don't, it must be preserved. If we do, then we only @@ -366,16 +366,6 @@ private static Collection getAllImplementations( return qualifiedNameToType.values(); } - /** - * Erases type arguments from a method signature string. - * - * @param signature the signature - * @return the same signature without type arguments - */ - private static String erase(String signature) { - return signature.replaceAll("<.*>", ""); - } - /** * Given a MethodDeclaration, this method returns the method that it overrides, if one exists in * one of its super classes. If one does not exist, it returns null. diff --git a/src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java b/src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java index fdd6919e..e4482a43 100644 --- a/src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java +++ b/src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java @@ -3598,18 +3598,83 @@ private String lookupFQNs(String javacType) { new ModifierVisitor() { @Override public Visitable visit(ClassOrInterfaceType type, Void p) { - if (classAndPackageMap.containsKey(type.asString())) { - return new ClassOrInterfaceType( - classAndPackageMap.get(type.asString()) + "." + type.asString()); - } else { + StringBuilder fullyQualifiedName = new StringBuilder(); + if (classAndPackageMap.containsKey(JavaParserUtil.erase(type.asString()))) { + fullyQualifiedName.append(classAndPackageMap.get(type.asString())); + fullyQualifiedName.append("."); + } else if (!type.getTypeArguments().isPresent()) { + // This type is in the same package and doesn't contain any type parameters return super.visit(type, p); } + + NodeList typeArguments = type.getTypeArguments().orElse(null); + + if (typeArguments != null) { + fullyQualifiedName.append( + type.asString().substring(0, type.asString().indexOf('<') + 1)); + + for (int i = 0; i < typeArguments.size(); i++) { + Type typeArgument = typeArguments.get(i); + lookupTypeArgumentFQN(fullyQualifiedName, typeArgument); + + if (i < typeArguments.size() - 1) { + fullyQualifiedName.append(", "); + } + } + fullyQualifiedName.append(">"); + } else { + fullyQualifiedName.append(type.asString()); + } + + return StaticJavaParser.parseClassOrInterfaceType(fullyQualifiedName.toString()); } }, null); return typeVarDecl + parsedJavac.toString(); } + /** + * Helper method for lookupFQNs which adds the fully qualified type argument to + * fullyQualifiedName. + * + * @param fullyQualifiedName the fully qualified name to build + * @param typeArgument the type argument to lookup + */ + private void lookupTypeArgumentFQN(StringBuilder fullyQualifiedName, Type typeArgument) { + String erased = JavaParserUtil.erase(typeArgument.asString()); + if (classAndPackageMap.containsKey(erased)) { + fullyQualifiedName + .append(classAndPackageMap.get(erased)) + .append(".") + .append(typeArgument.asString()); + } else if (JavaLangUtils.isJavaLangName(erased)) { + // Keep java.lang type arguments as is (Integer, String, etc.) + fullyQualifiedName.append(typeArgument.asString()); + } else if (typeArgument.isWildcardType()) { + WildcardType asWildcardType = typeArgument.asWildcardType(); + + if (asWildcardType.getSuperType().isPresent()) { + fullyQualifiedName.append("? super "); + lookupTypeArgumentFQN(fullyQualifiedName, asWildcardType.getSuperType().get()); + } else if (asWildcardType.getExtendedType().isPresent()) { + fullyQualifiedName.append("? extends "); + lookupTypeArgumentFQN(fullyQualifiedName, asWildcardType.getExtendedType().get()); + } + } else if (isAClassPath(erased)) { + // If it's already a fully qualified name, don't do anything + fullyQualifiedName.append(typeArgument.asString()); + } else { + // If it's not imported, it's probably in the same package + // TODO: handle already fully qualified generic type arguments. Right now JavaTypeCorrect + // only outputs the simple class name, so there is no way to get its fully qualified name + // here without ambiguity (i.e. org.example.Foo and com.example.Foo have different signatures) + // with the same simple class name + // Check MethodReturnFullyQualifiedGenericTest for more details; the current return type in + // Bar.java is com.example.InOtherPackage2 rather than com.foo.InOtherPackage2 (expected) + fullyQualifiedName.append(currentPackage).append(".").append(typeArgument.toString()); + } + } + /** * If and only if synthetic classes exist for both input type names, this method migrates all the * content of the sythetic class for the incorrect type name to the synthetic class for the diff --git a/src/main/resources/min_program_compile_status.json b/src/main/resources/min_program_compile_status.json index b0a27e89..94bd06d6 100644 --- a/src/main/resources/min_program_compile_status.json +++ b/src/main/resources/min_program_compile_status.json @@ -28,5 +28,5 @@ "na-389": "PASS", "na-705": "PASS", "na-791a": "PASS", - "na-791b": "FAIL" + "na-791b": "PASS" } diff --git a/src/test/java/org/checkerframework/specimin/MethodReturnFullyQualifiedGenericTest.java b/src/test/java/org/checkerframework/specimin/MethodReturnFullyQualifiedGenericTest.java new file mode 100644 index 00000000..0bdf967a --- /dev/null +++ b/src/test/java/org/checkerframework/specimin/MethodReturnFullyQualifiedGenericTest.java @@ -0,0 +1,21 @@ +package org.checkerframework.specimin; + +import java.io.IOException; +import org.junit.Test; + +/** + * This test checks that Specimin includes the fully qualified class name in generics when + * generating a synthetic method. For example, if a return type is Bar, the actual + * synthetic return type should be com.qualified.Bar. This test encodes Specimin's + * current behavior, which doesn't produce compilable output. If you break this test, it might + * not be a bad thing. + */ +public class MethodReturnFullyQualifiedGenericTest { + @Test + public void runTest() throws IOException { + SpeciminTestExecutor.runTestWithoutJarPaths( + "methodreturnfullyqualifiedgeneric", + new String[] {"com/example/Simple.java"}, + new String[] {"com.example.Simple#alreadyQualified()"}); + } +} diff --git a/src/test/java/org/checkerframework/specimin/MethodReturnGenericTest.java b/src/test/java/org/checkerframework/specimin/MethodReturnGenericTest.java new file mode 100644 index 00000000..fe1d448f --- /dev/null +++ b/src/test/java/org/checkerframework/specimin/MethodReturnGenericTest.java @@ -0,0 +1,23 @@ +package org.checkerframework.specimin; + +import java.io.IOException; +import org.junit.Test; + +/** + * This test checks that Specimin includes the fully qualified class name in generics when + * generating a synthetic method. For example, if a return type is Foo, Specimin should always + * generate the synthetic method return type as com.example.Foo. + */ +public class MethodReturnGenericTest { + @Test + public void runTest() throws IOException { + SpeciminTestExecutor.runTestWithoutJarPaths( + "methodreturngeneric", + new String[] {"com/example/Simple.java"}, + new String[] { + "com.example.Simple#foo()", + "com.example.Simple#bar()", + "com.example.Simple#baz()" + }); + } +} diff --git a/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/bar/Bar.java b/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/bar/Bar.java new file mode 100644 index 00000000..701edad9 --- /dev/null +++ b/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/bar/Bar.java @@ -0,0 +1,8 @@ +package com.bar; + +public class Bar { + + public static com.bar.Bar getOtherPackage2() { + throw new Error(); + } +} diff --git a/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/example/Simple.java b/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/example/Simple.java new file mode 100644 index 00000000..5d09c4d1 --- /dev/null +++ b/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/example/Simple.java @@ -0,0 +1,10 @@ +package com.example; + +import com.bar.Bar; + +public class Simple { + + public Bar alreadyQualified() { + return Bar.getOtherPackage2(); + } +} diff --git a/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/foo/InOtherPackage2.java b/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/foo/InOtherPackage2.java new file mode 100644 index 00000000..dc8c7b9e --- /dev/null +++ b/src/test/resources/methodreturnfullyqualifiedgeneric/expected/com/foo/InOtherPackage2.java @@ -0,0 +1,4 @@ +package com.foo; + +public class InOtherPackage2 { +} diff --git a/src/test/resources/methodreturnfullyqualifiedgeneric/input/com/example/Simple.java b/src/test/resources/methodreturnfullyqualifiedgeneric/input/com/example/Simple.java new file mode 100644 index 00000000..eff46a48 --- /dev/null +++ b/src/test/resources/methodreturnfullyqualifiedgeneric/input/com/example/Simple.java @@ -0,0 +1,9 @@ +package com.example; + +import com.bar.Bar; + +public class Simple { + public Bar alreadyQualified() { + return Bar.getOtherPackage2(); + } +} diff --git a/src/test/resources/methodreturngeneric/expected/com/bar/Bar.java b/src/test/resources/methodreturngeneric/expected/com/bar/Bar.java new file mode 100644 index 00000000..5762efcd --- /dev/null +++ b/src/test/resources/methodreturngeneric/expected/com/bar/Bar.java @@ -0,0 +1,16 @@ +package com.bar; + +public class Bar { + + public static com.bar.Bar getOtherPackage() { + throw new Error(); + } + + public static com.bar.Bar getSamePackage() { + throw new Error(); + } + + public static com.bar.Bar getJavaLang() { + throw new Error(); + } +} diff --git a/src/test/resources/methodreturngeneric/expected/com/example/InSamePackage.java b/src/test/resources/methodreturngeneric/expected/com/example/InSamePackage.java new file mode 100644 index 00000000..54524550 --- /dev/null +++ b/src/test/resources/methodreturngeneric/expected/com/example/InSamePackage.java @@ -0,0 +1,4 @@ +package com.example; + +public class InSamePackage { +} diff --git a/src/test/resources/methodreturngeneric/expected/com/example/Simple.java b/src/test/resources/methodreturngeneric/expected/com/example/Simple.java new file mode 100644 index 00000000..9dc11bd3 --- /dev/null +++ b/src/test/resources/methodreturngeneric/expected/com/example/Simple.java @@ -0,0 +1,19 @@ +package com.example; + +import com.bar.Bar; +import com.foo.InOtherPackage; + +public class Simple { + + public Bar foo() { + return Bar.getOtherPackage(); + } + + public Bar bar() { + return Bar.getSamePackage(); + } + + public Bar baz() { + return Bar.getJavaLang(); + } +} diff --git a/src/test/resources/methodreturngeneric/expected/com/foo/InOtherPackage.java b/src/test/resources/methodreturngeneric/expected/com/foo/InOtherPackage.java new file mode 100644 index 00000000..28c4334b --- /dev/null +++ b/src/test/resources/methodreturngeneric/expected/com/foo/InOtherPackage.java @@ -0,0 +1,4 @@ +package com.foo; + +public class InOtherPackage { +} diff --git a/src/test/resources/methodreturngeneric/input/com/example/Simple.java b/src/test/resources/methodreturngeneric/input/com/example/Simple.java new file mode 100644 index 00000000..5913800f --- /dev/null +++ b/src/test/resources/methodreturngeneric/input/com/example/Simple.java @@ -0,0 +1,18 @@ +package com.example; + +import com.bar.Bar; +import com.foo.InOtherPackage; + +public class Simple { + public Bar foo() { + return Bar.getOtherPackage(); + } + + public Bar bar() { + return Bar.getSamePackage(); + } + + public Bar baz() { + return Bar.getJavaLang(); + } +} diff --git a/typecheck_one_test.sh b/typecheck_one_test.sh index 019ea2bd..e8868514 100644 --- a/typecheck_one_test.sh +++ b/typecheck_one_test.sh @@ -14,6 +14,8 @@ if [ "${testcase}" = "superinterfaceextends" ]; then exit 0; fi # incomplete handling of method references: https://github.com/njit-jerse/specimin/issues/291 # this test exists to check that no crash occurs, not that Specimin produces the correct output if [ "${testcase}" = "methodref2" ]; then exit 0; fi +# this test will not compile right now; this is a TODO in UnsolvedSymbolVisitor#lookupTypeArgumentFQN +if [ "${testcase}" = "methodreturnfullyqualifiedgeneric" ]; then exit 0; fi cd "${testcase}/expected/" || exit 2 # javac relies on word splitting # shellcheck disable=SC2046 diff --git a/typecheck_test_outputs.bat b/typecheck_test_outputs.bat index b07d0dea..9efe890a 100644 --- a/typecheck_test_outputs.bat +++ b/typecheck_test_outputs.bat @@ -20,6 +20,8 @@ for /d %%t in (*) do ( rem incomplete handling of method references: https://github.com/njit-jerse/specimin/issues/291 rem this test exists to check that no crash occurs, not that Specimin produces the correct output if "%%t"=="methodref2" set continue=1 + rem this test will not compile right now; this is a TODO in UnsolvedSymbolVisitor#lookupTypeArgumentFQN + if "%%t"=="methodreturnfullyqualifiedgeneric" set continue=1 if !continue!==0 ( cd "%%t/expected/" || exit 1 rem javac relies on word splitting