diff --git a/lib/src/javaParser/java/com/diffplug/spotless/glue/javaparser/ShortenQualifiedTypesFormatterFunc.java b/lib/src/javaParser/java/com/diffplug/spotless/glue/javaparser/ShortenQualifiedTypesFormatterFunc.java index e0052e9353..a62fcf2876 100644 --- a/lib/src/javaParser/java/com/diffplug/spotless/glue/javaparser/ShortenQualifiedTypesFormatterFunc.java +++ b/lib/src/javaParser/java/com/diffplug/spotless/glue/javaparser/ShortenQualifiedTypesFormatterFunc.java @@ -32,18 +32,66 @@ import com.github.javaparser.Position; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.ImportDeclaration; +import com.github.javaparser.ast.Node; import com.github.javaparser.ast.PackageDeclaration; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; import com.github.javaparser.ast.body.EnumDeclaration; import com.github.javaparser.ast.body.RecordDeclaration; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.FieldAccessExpr; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.NameExpr; import com.github.javaparser.ast.type.ClassOrInterfaceType; import com.github.javaparser.ast.visitor.VoidVisitorAdapter; import com.diffplug.spotless.FormatterFunc; /** - * Uses JavaParser to identify fully qualified type references in the AST, - * then performs text-level replacement to shorten them and add imports. + * Shortens fully-qualified type references and adds the corresponding imports. + * + *

Goal: never introduce a compile error

+ * + *

Spotless runs without a full classpath or type-resolution, so the formatter + * cannot know every type visible to the compiler. The guiding principle is: + * "when in doubt, leave it alone." An un-shortened reference is merely + * verbose; a wrongly-shortened one breaks the build. + * + *

What we can check exactly

+ * + * + *

Expression-context heuristics

+ * + *

FQTs also appear in expression context — static method calls + * ({@code java.lang.management.ManagementFactory.getPlatformMXBeans(…)}), + * static field / enum-constant access + * ({@code java.util.concurrent.TimeUnit.SECONDS}), and nested-type member + * access ({@code pkg.models.CustomTypeProperty.TypeEnum.STRING}). + * JavaParser sees these as {@code FieldAccessExpr}/{@code MethodCallExpr}, + * not type nodes, so we apply two heuristics to avoid false positives: + * + *

    + *
  1. Known-package check. If the candidate FQN's package already + * appears in the file's imports, own package declaration, or is + * {@code java.lang}, we trust it.
    2. + *
  2. Minimum-depth fallback. Otherwise we require at least two + * lowercase (package) segments before the first uppercase (type) segment. + * This filters out {@code variable.Field} patterns that look superficially + * like a FQT but are really field access on a local variable. + *
    In theory this could skip a legitimate single-segment package + * (e.g. {@code a.MyType.method()}) that has no import in the file yet, + * but single-segment packages are virtually non-existent in practice.
    3. + *
* *

The parser gives us accurate type-context identification (no false positives * from strings, comments, or non-type contexts). Text-level replacement preserves @@ -86,17 +134,36 @@ public String apply(String rawUnix) throws Exception { cu.findAll(EnumDeclaration.class).forEach(c -> declaredTypeNames.add(c.getNameAsString())); cu.findAll(RecordDeclaration.class).forEach(c -> declaredTypeNames.add(c.getNameAsString())); - // 4. Walk the AST to find outermost fully-qualified type nodes + // 4. Collect unqualified type references, which may resolve to types in the same package + Set unqualifiedTypeNames = new LinkedHashSet<>(); + cu.findAll(ClassOrInterfaceType.class).stream() + .filter(type -> type.getScope().isEmpty()) + .forEach(type -> unqualifiedTypeNames.add(type.getNameAsString())); + + // 5. Build set of known packages from existing imports, own package, and java.lang + Set knownPackages = new LinkedHashSet<>(); + knownPackages.add("java.lang"); + if (!packageName.isEmpty()) { + knownPackages.add(packageName); + } + for (String fqn : existingImportFqns) { + int lastDot = fqn.lastIndexOf('.'); + if (lastDot > 0) { + knownPackages.add(fqn.substring(0, lastDot)); + } + } + + // 6. Walk the AST to find outermost fully-qualified type nodes Map> simpleToFqns = new LinkedHashMap<>(); List qualifiedRefs = new ArrayList<>(); - cu.accept(new CollectQualifiedTypesVisitor(simpleToFqns, qualifiedRefs), null); + cu.accept(new CollectQualifiedTypesVisitor(simpleToFqns, qualifiedRefs, knownPackages), null); if (qualifiedRefs.isEmpty()) { return rawUnix; } - // 5. Determine which FQNs are safe to shorten + // 7. Determine which FQNs are safe to shorten Set safeToShorten = new LinkedHashSet<>(); for (Map.Entry> entry : simpleToFqns.entrySet()) { String simple = entry.getKey(); @@ -109,8 +176,9 @@ public String apply(String rawUnix) throws Exception { if (existing != null && !existing.equals(fqn)) { continue; } - // Skip if simple name clashes with a type declared in this file - if (declaredTypeNames.contains(simple)) { + // Skip if simple name clashes with a type declared or already referenced in this file + if (declaredTypeNames.contains(simple) + || (existing == null && unqualifiedTypeNames.contains(simple) && !isImplicitlyImported(fqn, packageName))) { continue; } safeToShorten.add(fqn); @@ -120,7 +188,7 @@ public String apply(String rawUnix) throws Exception { return rawUnix; } - // 6. Convert line/column positions to string offsets and replace + // 8. Convert line/column positions to string offsets and replace // Build line-start offset table int[] lineOffsets = buildLineOffsets(rawUnix); @@ -147,14 +215,10 @@ public String apply(String rawUnix) throws Exception { sb.delete(removal[0], removal[1]); } - // 7. Add missing imports + // 9. Add missing imports Set newImports = new TreeSet<>(); for (String fqn : safeToShorten) { - if (fqn.startsWith("java.lang.") && fqn.indexOf('.', 10) == -1) { - continue; - } - if (!packageName.isEmpty() && fqn.startsWith(packageName + ".") - && fqn.indexOf('.', packageName.length() + 1) == -1) { + if (isImplicitlyImported(fqn, packageName)) { continue; } if (existingImportFqns.contains(fqn)) { @@ -188,10 +252,13 @@ private record QualifiedTypeRef(String fqn, String simpleName, Position scopeSta private static final class CollectQualifiedTypesVisitor extends VoidVisitorAdapter { private final Map> simpleToFqns; private final List qualifiedRefs; + private final Set knownPackages; - CollectQualifiedTypesVisitor(Map> simpleToFqns, List qualifiedRefs) { + CollectQualifiedTypesVisitor(Map> simpleToFqns, List qualifiedRefs, + Set knownPackages) { this.simpleToFqns = simpleToFqns; this.qualifiedRefs = qualifiedRefs; + this.knownPackages = knownPackages; } @Override @@ -222,6 +289,73 @@ public void visit(ClassOrInterfaceType type, Void arg) { qualifiedRefs.add(new QualifiedTypeRef(rawName, simple, scopeStart, nameStart)); } } + + @Override + public void visit(MethodCallExpr expr, Void arg) { + super.visit(expr, arg); + expr.getScope().ifPresent(this::processExpressionScope); + } + + @Override + public void visit(FieldAccessExpr expr, Void arg) { + super.visit(expr, arg); + if (expr.getParentNode().isPresent()) { + Node parent = expr.getParentNode().get(); + if (parent instanceof FieldAccessExpr fa && fa.getScope() == expr) { + return; + } + if (parent instanceof MethodCallExpr mc && mc.getScope().isPresent() && mc.getScope().get() == expr) { + return; + } + } + processExpressionScope(expr); + } + + /** Extracts a fully-qualified type from an expression chain of FieldAccessExpr/NameExpr nodes. */ + private void processExpressionScope(Expression expr) { + List chain = new ArrayList<>(); + Expression current = expr; + while (current instanceof FieldAccessExpr fa) { + chain.add(0, fa); + current = fa.getScope(); + } + if (!(current instanceof NameExpr ne)) { + return; + } + String rootName = ne.getNameAsString(); + if (rootName.isEmpty() || !Character.isLowerCase(rootName.charAt(0))) { + return; + } + int typeIdx = -1; + for (int i = 0; i < chain.size(); i++) { + String name = chain.get(i).getNameAsString(); + if (!name.isEmpty() && Character.isUpperCase(name.charAt(0))) { + typeIdx = i; + break; + } + } + if (typeIdx < 0) { + return; + } + StringBuilder fqn = new StringBuilder(rootName); + for (int i = 0; i <= typeIdx; i++) { + fqn.append('.').append(chain.get(i).getNameAsString()); + } + String fqnStr = fqn.toString(); + String candidatePackage = fqnStr.substring(0, fqnStr.lastIndexOf('.')); + // Trust if package is known from imports; otherwise require ≥2 package segments + if (!knownPackages.contains(candidatePackage) && (typeIdx + 1) < 2) { + return; + } + String simple = chain.get(typeIdx).getNameAsString(); + FieldAccessExpr typeNode = chain.get(typeIdx); + Expression typeScope = typeNode.getScope(); + if (typeScope.getBegin().isPresent() && typeNode.getName().getBegin().isPresent()) { + simpleToFqns.computeIfAbsent(simple, k -> new LinkedHashSet<>()).add(fqnStr); + qualifiedRefs.add(new QualifiedTypeRef(fqnStr, simple, + typeScope.getBegin().get(), typeNode.getName().getBegin().get())); + } + } } private static String buildRawName(ClassOrInterfaceType type) { @@ -242,6 +376,12 @@ private static boolean startsWithPackage(String rawName) { return !rawName.isEmpty() && Character.isLowerCase(rawName.charAt(0)); } + private static boolean isImplicitlyImported(String fqn, String packageName) { + return fqn.startsWith("java.lang.") && fqn.indexOf('.', 10) == -1 + || !packageName.isEmpty() && fqn.startsWith(packageName + ".") + && fqn.indexOf('.', packageName.length() + 1) == -1; + } + /** Builds an array where lineOffsets[line] is the char offset of the start of that line (1-indexed). */ private static int[] buildLineOffsets(String text) { List offsets = new ArrayList<>(); diff --git a/testlib/src/test/java/com/diffplug/spotless/java/ShortenFullyQualifiedTypesStepTest.java b/testlib/src/test/java/com/diffplug/spotless/java/ShortenFullyQualifiedTypesStepTest.java index 7e32b6d288..61933dba30 100644 --- a/testlib/src/test/java/com/diffplug/spotless/java/ShortenFullyQualifiedTypesStepTest.java +++ b/testlib/src/test/java/com/diffplug/spotless/java/ShortenFullyQualifiedTypesStepTest.java @@ -27,6 +27,47 @@ import com.diffplug.spotless.LineEnding; import com.diffplug.spotless.TestProvisioner; +/** + * Tests for {@code shortenFullyQualifiedTypes}. + * + *

Safety contract: never introduce a compile error

+ * + *

The formatter operates without a classpath, so it cannot resolve types the way + * the compiler does. The rule is: when in doubt, leave the reference qualified. + * A verbose FQN is harmless; a wrong shortening breaks the build. + * + *

Tests are organised into three groups: + * + *

    + *
  1. Exact checks — situations where we have enough information to + * decide with certainty. Examples: the simple name already appears in a + * different import, or clashes with a type declared in the same file. + * In these cases the formatter must leave the FQN alone.
    2. + * + *
  2. Expression-context heuristics — FQTs used as static method call + * targets ({@code java.lang.management.ManagementFactory.getPlatformMXBeans(…)}), + * static field or enum-constant access, or nested-type member access. + * JavaParser sees these as {@code FieldAccessExpr}/{@code MethodCallExpr}, + * not type nodes, so two heuristics guard against false positives: + *
    3. + * + *
  3. Known ambiguous / leave-as-is — cases where shortening could + * change semantics and the formatter cannot prove safety. For example, + * a FQN whose simple name matches a same-package type used unqualified + * elsewhere in the file.
    4. + *
+ */ class ShortenFullyQualifiedTypesStepTest { private FormatterStep step() { @@ -139,6 +180,7 @@ void alreadyImportedNotDuplicated() throws Exception { "", "public class Foo {", " java.util.List a;", + " List b;", "}", ""); String result = apply(before); @@ -329,6 +371,20 @@ void fqnCollisionWithInnerClassName() throws Exception { assertEquals(code, apply(code)); } + @Test + void fqnCollisionWithUnqualifiedSamePackageType() throws Exception { + // RandomAccessFile is declared in another file in this package; importing java.io.RandomAccessFile + // would silently change which type the unqualified superclass name resolves to + String code = String.join("\n", + "package test.reprod1;", + "", + "public class ClassA extends RandomAccessFile {", + " final java.io.RandomAccessFile file;", + "}", + ""); + assertEquals(code, apply(code)); + } + @Test void fqnNoCollisionWithDifferentSimpleName() throws Exception { // FQN whose simple name does NOT match the enclosing class — should still shorten @@ -344,6 +400,167 @@ void fqnNoCollisionWithDifferentSimpleName() throws Exception { assertTrue(result.contains("import java.util.List;"), "should import List"); } + @Test + void issue3039_fqtInStaticMethodCall() throws Exception { + String before = String.join("\n", + "import java.lang.management.BufferPoolMXBean;", + "import java.util.List;", + "", + "public class ClassA {", + " public void methodA() {", + " final List pools = java.lang.management.ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class);", + " }", + "}", + ""); + String result = apply(before); + assertFalse(codeBody(result).contains("java.lang.management.ManagementFactory"), + "FQT in static method call should be shortened"); + assertTrue(result.contains("import java.lang.management.ManagementFactory;"), + "should add ManagementFactory import"); + } + + @Test + void issue3039_fqtNestedTypeFieldAccess() throws Exception { + String before = String.join("\n", + "import java.util.List;", + "import pkg.models.CustomTypeProperty;", + "", + "public class ClassB {", + " final List connectionProps = List.of(new CustomTypeProperty().name(\"host\")", + " .type(pkg.models.CustomTypeProperty.TypeEnum.STRING));", + "}", + ""); + String result = apply(before); + assertFalse(codeBody(result).contains("pkg.models.CustomTypeProperty"), + "FQT in nested type field access should be shortened"); + } + + @Test + void exprStaticFieldAccess() throws Exception { + String before = String.join("\n", + "import java.util.concurrent.TimeUnit;", + "", + "public class Foo {", + " long millis = java.util.concurrent.TimeUnit.SECONDS.toMillis(5);", + "}", + ""); + String result = apply(before); + assertFalse(codeBody(result).contains("java.util.concurrent.TimeUnit"), + "FQT for static field access should be shortened"); + } + + @Test + void exprJavaLangImplicitPackage() throws Exception { + // java.lang is always known — should shorten even without explicit imports + String before = String.join("\n", + "public class Foo {", + " void test() {", + " java.lang.System.exit(0);", + " }", + "}", + ""); + String result = apply(before); + assertFalse(codeBody(result).contains("java.lang.System"), + "java.lang.System should be shortened (java.lang is implicit)"); + assertFalse(result.contains("import java.lang.System"), + "java.lang types should not be imported"); + } + + @Test + void exprChainedAfterStaticMethod() throws Exception { + String before = String.join("\n", + "import java.util.List;", + "", + "public class Foo {", + " List items = java.util.Collections.unmodifiableList(new java.util.ArrayList<>());", + "}", + ""); + String result = apply(before); + assertFalse(codeBody(result).contains("java.util.Collections"), + "FQT in chained static method call should be shortened"); + assertTrue(result.contains("import java.util.Collections;"), "should import Collections"); + } + + @Test + void exprEnumConstantAccess() throws Exception { + String before = String.join("\n", + "import java.time.LocalDate;", + "", + "public class Foo {", + " Object day = java.time.DayOfWeek.MONDAY;", + "}", + ""); + String result = apply(before); + assertFalse(codeBody(result).contains("java.time.DayOfWeek"), + "FQT for enum constant should be shortened"); + assertTrue(result.contains("import java.time.DayOfWeek;"), "should import DayOfWeek"); + } + + // ── Expression-context: should NOT shorten (ambiguous) ─────────────── + + @Test + void exprSingleSegmentUnknownPackageNotShortened() throws Exception { + // 'config' could be a local variable; only 1 lowercase segment, no matching import + String code = String.join("\n", + "public class Foo {", + " Object v = config.Default.VALUE;", + "}", + ""); + assertEquals(code, apply(code)); + } + + @Test + void exprSingleSegmentUnknownPackageMethodNotShortened() throws Exception { + // 'builder' could be a local variable; only 1 lowercase segment, no matching import + String code = String.join("\n", + "public class Foo {", + " Object v = builder.Type.create();", + "}", + ""); + assertEquals(code, apply(code)); + } + + @Test + void exprSingleSegmentWithKnownImportDoesShorten() throws Exception { + // 'config' has 1 lowercase segment but IS a known package (import exists from config.*) + String before = String.join("\n", + "import config.Other;", + "", + "public class Foo {", + " Object v = config.Default.VALUE;", + "}", + ""); + String result = apply(before); + assertFalse(codeBody(result).contains("config.Default"), + "known single-segment package should be shortened"); + } + + @Test + void exprCollisionWithExistingImportNotShortened() throws Exception { + // java.util.List is already imported; java.awt.List in expression context must not shorten + String code = String.join("\n", + "import java.util.List;", + "", + "public class Foo {", + " int n = java.awt.List.COLUMN_HEADERS;", + "}", + ""); + assertEquals(code, apply(code)); + } + + @Test + void exprCollisionWithDeclaredTypeNotShortened() throws Exception { + // File declares class named 'Entry'; expression FQT with same simple name must not shorten + String code = String.join("\n", + "import java.util.Map;", + "", + "public class Entry {", + " Object e = java.util.Map.Entry.class;", + "}", + ""); + assertEquals(code, apply(code)); + } + @Test void multipleAnnotationsWithFqn() throws Exception { // FQNs used as annotation types should NOT be treated as type references