From 6086de52fc14d12f09f8803ecd256824ade9ee41 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 7 Dec 2022 15:59:32 +0100 Subject: [PATCH] Avoid ambiguous imports with `@UseImportPolicy(STATIC_IMPORT_ALWAYS)` These changes prevent Refaster rules from introducing a static import that conflicts with an existing static import. Without these changes the new test case fails with the following error: ``` com/google/errorprone/refaster/testdata/StaticImportClashTemplate.java:27: error: reference to reverseOrder is ambiguous return reverseOrder(); ^ both method reverseOrder() in java.util.Comparator and method reverseOrder() in java.util.Collections match ``` --- .../errorprone/refaster/ImportPolicy.java | 23 ++++++++--- .../refaster/TemplateIntegrationTest.java | 5 +++ .../StaticImportClashTemplateExample.java | 28 ++++++++++++++ .../StaticImportClashTemplateExample.java | 28 ++++++++++++++ .../template/StaticImportClashTemplate.java | 38 +++++++++++++++++++ 5 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 core/src/test/java/com/google/errorprone/refaster/testdata/input/StaticImportClashTemplateExample.java create mode 100644 core/src/test/java/com/google/errorprone/refaster/testdata/output/StaticImportClashTemplateExample.java create mode 100644 core/src/test/java/com/google/errorprone/refaster/testdata/template/StaticImportClashTemplate.java diff --git a/core/src/main/java/com/google/errorprone/refaster/ImportPolicy.java b/core/src/main/java/com/google/errorprone/refaster/ImportPolicy.java index 150fa1f23570..e33d2d69fa9b 100644 --- a/core/src/main/java/com/google/errorprone/refaster/ImportPolicy.java +++ b/core/src/main/java/com/google/errorprone/refaster/ImportPolicy.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static java.util.function.Predicate.not; import com.google.common.base.Joiner; import com.google.common.base.Splitter; @@ -36,6 +37,7 @@ import java.util.Iterator; import java.util.List; import java.util.Optional; +import java.util.function.Predicate; import java.util.stream.Stream; /** @@ -59,7 +61,7 @@ public JCExpression classReference( // Special handling to ensure that the pretty-printer always recognizes Refaster references return inliner.maker().Ident(inliner.asName("Refaster")); } - ImmutableSet allImports = getAllImports(inliner, WhichImports.NON_STATIC); + ImmutableSet allImports = getAllImports(inliner, WhichImports.NON_STATIC, i -> true); /* * Check if topLevelClazz or fullyQualifiedClazz is already imported. * If fullyQualifiedClazz is imported, return the class name. @@ -198,9 +200,18 @@ public JCExpression staticReference( return IMPORT_TOP_LEVEL.staticReference( inliner, topLevelClazz, fullyQualifiedClazz, member); } - // Check to see if the reference is already static-imported. - String importableName = fullyQualifiedClazz + "." + member; - if (!getAllImports(inliner, WhichImports.STATIC).contains(importableName)) { + // Check to see if the reference (or a conflicting reference) is already static-imported. + String importSuffix = "." + member; + ImmutableSet relatedImports = + getAllImports(inliner, WhichImports.STATIC, i -> i.endsWith(importSuffix)); + String importableName = fullyQualifiedClazz + importSuffix; + if (!relatedImports.stream().allMatch(importableName::equals)) { + // A conflicting identifier is already statically imported. + return IMPORT_TOP_LEVEL.staticReference( + inliner, topLevelClazz, fullyQualifiedClazz, member); + } + if (relatedImports.isEmpty()) { + // The reference is not yet statically imported. inliner.addStaticImport(importableName); } return inliner.maker().Ident(inliner.asName(member)); @@ -260,7 +271,8 @@ boolean existingImportMatches(JCImport jcImport) { * Returns the set of imports that already exist of the import type (both in the source file and * in the pending list of imports to add). */ - private static ImmutableSet getAllImports(Inliner inliner, WhichImports whichImports) { + private static ImmutableSet getAllImports( + Inliner inliner, WhichImports whichImports, Predicate filter) { return Streams.concat( whichImports.getExistingImports(inliner), Optional.ofNullable(inliner.getContext()) @@ -270,6 +282,7 @@ private static ImmutableSet getAllImports(Inliner inliner, WhichImports .orElse(Stream.of()) .filter(whichImports::existingImportMatches) .map(imp -> imp.getQualifiedIdentifier().toString())) + .filter(filter) .collect(toImmutableSet()); } } diff --git a/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java b/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java index c928231983e1..71fbb5f24158 100644 --- a/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java +++ b/core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java @@ -369,4 +369,9 @@ public void staticImportClassToken() throws IOException { public void suppressWarnings() throws IOException { runTest("SuppressWarningsTemplate"); } + + @Test + public void staticImportClash() throws IOException { + runTest("StaticImportClashTemplate"); + } } diff --git a/core/src/test/java/com/google/errorprone/refaster/testdata/input/StaticImportClashTemplateExample.java b/core/src/test/java/com/google/errorprone/refaster/testdata/input/StaticImportClashTemplateExample.java new file mode 100644 index 000000000000..854731ea609b --- /dev/null +++ b/core/src/test/java/com/google/errorprone/refaster/testdata/input/StaticImportClashTemplateExample.java @@ -0,0 +1,28 @@ +/* + * Copyright 2022 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.refaster.testdata; + +import static java.util.Collections.reverseOrder; + +import java.util.Comparator; + +/** Test data for {@code StaticImportClashTemplate}. */ +public class StaticImportClashTemplate { + Comparator example() { + return reverseOrder(); + } +} diff --git a/core/src/test/java/com/google/errorprone/refaster/testdata/output/StaticImportClashTemplateExample.java b/core/src/test/java/com/google/errorprone/refaster/testdata/output/StaticImportClashTemplateExample.java new file mode 100644 index 000000000000..e23b8d5c0ef8 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/refaster/testdata/output/StaticImportClashTemplateExample.java @@ -0,0 +1,28 @@ +/* + * Copyright 2022 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.refaster.testdata; + +import static java.util.Collections.reverseOrder; + +import java.util.Comparator; + +/** Test data for {@code StaticImportClashTemplate}. */ +public class StaticImportClashTemplate { + Comparator example() { + return Comparator.reverseOrder(); + } +} diff --git a/core/src/test/java/com/google/errorprone/refaster/testdata/template/StaticImportClashTemplate.java b/core/src/test/java/com/google/errorprone/refaster/testdata/template/StaticImportClashTemplate.java new file mode 100644 index 000000000000..300710816158 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/refaster/testdata/template/StaticImportClashTemplate.java @@ -0,0 +1,38 @@ +/* + * Copyright 2022 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.refaster.testdata.template; + +import static java.util.Comparator.reverseOrder; + +import com.google.errorprone.refaster.ImportPolicy; +import com.google.errorprone.refaster.annotation.AfterTemplate; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.UseImportPolicy; +import java.util.Collections; +import java.util.Comparator; + +/** Example template that may cause a static import clash. */ +final class StaticImportClashTemplate> { + @BeforeTemplate + Comparator before() { + return Collections.reverseOrder(); + } + + @AfterTemplate + @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS) + Comparator after() { + return reverseOrder(); + } +}