Enforce error prone's UnnecessarilyFullyQualified rule

[trask]

Follow-up to

• Replace Xlint deprecation check with custom errorprone check #16122

As part of this, I flipped the general convention in instrumentation/opentelemetry-api/opentelemetry-api-* modules to import io.opentelemetry.* and never import application.io.opentelemetry.*

I think this is actually a bit simpler to read, since it conforms with the rest of the repo.

And it's easier to carve out exception for allowing FQCN for application.*

[trask]

diff with upstream

--- a/UnnecessarilyFullyQualified.java  (upstream Error Prone, master branch)
+++ b/OtelUnnecessarilyFullyQualified.java  (this PR)
@@ -1,5 +1,11 @@
 /*
- * Copyright 2020 The Error Prone Authors.
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+// Includes work from:
+/*
+ * Copyright 2025 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.
@@ -13,19 +19,20 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package com.google.errorprone.bugpatterns;
+
+package io.opentelemetry.javaagent.customchecks;

 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getGeneratedBy;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
-import static com.google.errorprone.util.ASTHelpers.hasExplicitSource;
 import static com.google.errorprone.util.ASTHelpers.isGeneratedConstructor;
 import static com.google.errorprone.util.FindIdentifiers.findIdent;
 import static com.sun.tools.javac.code.Kinds.KindSelector.VAL_TYP;
 import static java.util.stream.Collectors.toCollection;

+import com.google.auto.service.AutoService;
 import com.google.common.base.Ascii;
 import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.HashMultimap;
@@ -36,6 +43,7 @@
 import com.google.errorprone.BugPattern.SeverityLevel;
 import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
@@ -55,6 +63,7 @@
 import com.sun.tools.javac.code.Symbol.PackageSymbol;
 import com.sun.tools.javac.code.Symbol.TypeSymbol;
 import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.util.Position;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
@@ -68,13 +77,49 @@
 /** Flags uses of fully qualified names which are not ambiguous if imported. */
 @BugPattern(
     severity = SeverityLevel.WARNING,
-    summary = "This fully qualified name is unambiguous to the compiler if imported.")
-public final class UnnecessarilyFullyQualified extends BugChecker
+    summary = "This fully qualified name is unambiguous to the compiler if imported.",
+    // so it can be suppressed with @SuppressWarnings("UnnecessarilyFullyQualified")
+    altNames = "UnnecessarilyFullyQualified")
+@AutoService(BugChecker.class)
+public final class OtelUnnecessarilyFullyQualified extends BugChecker
     implements CompilationUnitTreeMatcher {

   private static final ImmutableSet<String> EXEMPTED_NAMES = ImmutableSet.of("Annotation");

   /**
+   * Fully qualified names that are intentionally kept as FQNs. For example, {@code
+   * io.opentelemetry.extension.annotations.WithSpan} is the deprecated predecessor of {@code
+   * io.opentelemetry.instrumentation.annotations.WithSpan}; test code that exercises the old
+   * annotation uses the FQN to make it obvious which variant is under test.
+   */
+  private static final ImmutableSet<String> EXEMPTED_FQNS =
+      ImmutableSet.of(
+          "io.opentelemetry.extension.annotations.SpanAttribute",
+          "io.opentelemetry.extension.annotations.WithSpan");
+
+  /**
+   * Class names which are bad as a direct import if they have an enclosing class.
+   *
+   * <p>The common factor for these names isn't just that they may be vague class names; there are
+   * many more examples of that. What's important is that they are vague <em>and</em> generally
+   * clarified by the name of the outer class (that is, {@code Foo.Builder} is clearer than {@code
+   * Builder}).
+   */
+  // Copied from com.google.errorprone.bugpatterns.BadImport.BAD_NESTED_CLASSES (package-private).
+  private static final ImmutableSet<String> BAD_NESTED_CLASSES =
+      ImmutableSet.of(
+          "Builder",
+          "BuilderFactory",
+          "Callback",
+          "Class",
+          "Enum",
+          "Factory",
+          "Type",
+          "Key",
+          "Id",
+          "Provider");
+
+  /**
    * Exempted types that fully qualified name usages are acceptable for their nested types when
    * importing the enclosing type is ambiguous.
    *
@@ -90,11 +135,15 @@

   private final boolean batchFindings;

+  public OtelUnnecessarilyFullyQualified() {
+    this(ErrorProneFlags.empty());
+  }
+
   @Inject
-  UnnecessarilyFullyQualified(ErrorProneFlags errorProneFlags) {
+  OtelUnnecessarilyFullyQualified(ErrorProneFlags errorProneFlags) {
     this.exemptedEnclosingTypes = errorProneFlags.getSetOrEmpty("BadImport:BadEnclosingTypes");
     this.batchFindings =
-        errorProneFlags.getBoolean("UnnecessarilyFullyQualified:BatchFindings").orElse(false);
+        errorProneFlags.getBoolean("OtelUnnecessarilyFullyQualified:BatchFindings").orElse(false);
   }

   @Override
@@ -163,6 +212,14 @@
         if (!isFullyQualified(tree)) {
           return;
         }
+        if (isApplicationClass(tree)) {
+          return;
+        }
+        Symbol sym = getSymbol(tree);
+        if (sym instanceof ClassSymbol
+            && EXEMPTED_FQNS.contains(sym.getQualifiedName().toString())) {
+          return;
+        }
         if (isBadNestedClass(tree) || isExemptedEnclosingType(tree)) {
           handle(new TreePath(path, tree.getExpression()));
           return;
@@ -171,7 +228,13 @@
         if (!(symbol instanceof ClassSymbol)) {
           return;
         }
-        if (!hasExplicitSource(tree, state)) {
+        // Classes deprecated for removal (e.g. java.security.AccessController) must stay
+        // fully qualified: importing them produces a [removal] javadoc warning that cannot
+        // be suppressed at the import site, only at usage sites via @SuppressWarnings.
+        if (isDeprecatedForRemoval(symbol)) {
+          return;
+        }
+        if (state.getEndPosition(tree) == Position.NOPOS) {
           return;
         }
         List<TreePath> treePaths = table.get(tree.getIdentifier(), symbol.type.tsym);
@@ -183,7 +246,25 @@
       }

       private boolean isBadNestedClass(MemberSelectTree tree) {
-        return BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString());
+        return BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString());
+      }
+
+      private boolean isDeprecatedForRemoval(Symbol symbol) {
+        // When not cross-compiling, the javac symbol flag works directly.
+        if (symbol.isDeprecatedForRemoval()) {
+          return true;
+        }
+        // When compiling with --release 8, javac loads JDK class metadata from ct.sym
+        // which uses Java 8's @Deprecated (no forRemoval element), so the
+        // DEPRECATED_REMOVAL flag is never set on the symbol. Fall back to loading
+        // the class from the running JVM (JDK 9+) to check the actual annotation.
+        try {
+          Class<?> clazz = Class.forName(symbol.getQualifiedName().toString());
+          Deprecated deprecated = clazz.getAnnotation(Deprecated.class);
+          return deprecated != null && deprecated.forRemoval();
+        } catch (ClassNotFoundException | NoClassDefFoundError e) {
+          return false;
+        }
       }

       private boolean isExemptedEnclosingType(MemberSelectTree tree) {
@@ -199,6 +280,17 @@
         return exemptedEnclosingTypes.contains(symbol.getQualifiedName().toString());
       }

+      private boolean isApplicationClass(MemberSelectTree tree) {
+        ExpressionTree expression = tree;
+        while (expression instanceof MemberSelectTree) {
+          expression = ((MemberSelectTree) expression).getExpression();
+        }
+        if (expression instanceof IdentifierTree) {
+          return ((IdentifierTree) expression).getName().contentEquals("application");
+        }
+        return false;
+      }
+
       private boolean isFullyQualified(MemberSelectTree tree) {
         AtomicBoolean isFullyQualified = new AtomicBoolean();
         new SimpleTreeVisitor<Void, Void>() {
@@ -208,7 +300,7 @@
           }

           @Override
-          public Void visitIdentifier(IdentifierTree identifierTree, Void aVoid) {
+          public Void visitIdentifier(IdentifierTree identifierTree, Void unused) {
             if (getSymbol(identifierTree) instanceof PackageSymbol) {
               isFullyQualified.set(true);
             }
@@ -239,7 +331,7 @@
       if (EXEMPTED_NAMES.contains(nameString)) {
         continue;
       }
-      List<TreePath> pathsToFix = getOnlyElement(types.values());
+      List<TreePath> pathsToFix = Objects.requireNonNull(getOnlyElement(types.values()));
       Set<Symbol> meaningAtUsageSites =
           pathsToFix.stream()
               .map(path -> findIdent(nameString, state.withPath(path), VAL_TYP))
@@ -251,7 +343,8 @@
       SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
       // Only add the import if any of the usage sites don't already resolve to this type.
       if (meaningAtUsageSites.stream().anyMatch(Objects::isNull)) {
-        fixBuilder.addImport(getOnlyElement(types.keySet()).getQualifiedName().toString());
+        fixBuilder.addImport(
+            Objects.requireNonNull(getOnlyElement(types.keySet())).getQualifiedName().toString());
       }
       for (TreePath path : pathsToFix) {
         fixBuilder.replace(path.getLeaf(), nameString);

[trask]

diff with upstream

--- a/UnnecessarilyFullyQualifiedTest.java (upstream Error Prone)
+++ b/OtelUnnecessarilyFullyQualifiedTest.java (this PR)
@@ -1,4 +1,10 @@
 /*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+// Includes work from:
+/*
  * Copyright 2020 The Error Prone Authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -13,25 +19,24 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package com.google.errorprone.bugpatterns;
+
+package io.opentelemetry.javaagent.customchecks;

 import com.google.errorprone.BugCheckerRefactoringTestHelper;
 import com.google.errorprone.CompilationTestHelper;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
-/** Tests for {@link UnnecessarilyFullyQualified}. */
-@RunWith(JUnit4.class)
-public final class UnnecessarilyFullyQualifiedTest {
+import org.junit.jupiter.api.Test;
+
+/** Tests for {@link OtelUnnecessarilyFullyQualified}. */
+class OtelUnnecessarilyFullyQualifiedTest {
   private final BugCheckerRefactoringTestHelper helper =
-      BugCheckerRefactoringTestHelper.newInstance(UnnecessarilyFullyQualified.class, getClass());
+      BugCheckerRefactoringTestHelper.newInstance(
+          OtelUnnecessarilyFullyQualified.class, getClass());

   private final CompilationTestHelper compilationHelper =
-      CompilationTestHelper.newInstance(UnnecessarilyFullyQualified.class, getClass());
+      CompilationTestHelper.newInstance(OtelUnnecessarilyFullyQualified.class, getClass());

   @Test
-  public void singleUse() {
+  void singleUse() {
     helper
         .addInputLines(
             "Test.java",
@@ -57,7 +62,7 @@
   }

   @Test
-  public void wouldBeAmbiguous() {
+  void wouldBeAmbiguous() {
     helper
         .addInputLines(
             "List.java", //
@@ -75,7 +80,7 @@
   }

   @Test
-  public void refersToMultipleTypes() {
+  void refersToMultipleTypes() {
     helper
         .addInputLines(
             "List.java",
@@ -101,7 +106,7 @@
   }

   @Test
-  public void refersToMultipleTypes_dependingOnLocation() {
+  void refersToMultipleTypes_dependingOnLocation() {
     helper
         .addInputLines(
             "Outer.java",
@@ -133,7 +138,7 @@
   }

   @Test
-  public void inconsistentImportUsage() {
+  void inconsistentImportUsage() {
     helper
         .addInputLines(
             "Test.java",
@@ -161,7 +166,7 @@
   }

   @Test
-  public void clashesWithTypeInSuperType() {
+  void clashesWithTypeInSuperType() {
     helper
         .addInputLines(
             "A.java",
@@ -191,7 +196,7 @@
   }

   @Test
-  public void builder() {
+  void builder() {
     helper
         .addInputLines(
             "Foo.java",
@@ -231,7 +236,7 @@
   }

   @Test
-  public void exemptedNames() {
+  void exemptedNames() {
     helper
         .addInputLines(
             "Annotation.java",
@@ -253,7 +258,7 @@
   }

   @Test
-  public void innerClass() {
+  void innerClass() {
     helper
         .addInputLines(
             "A.java",
@@ -273,8 +278,8 @@
   }

   @Test
-  public void packageInfo() {
-    CompilationTestHelper.newInstance(UnnecessarilyFullyQualified.class, getClass())
+  void packageInfo() {
+    compilationHelper
         .addSourceLines(
             "a/A.java",
             """
@@ -292,7 +297,7 @@
   }

   @Test
-  public void staticNestedClass() {
+  void staticNestedClass() {
     helper
         .addInputLines(
             "test/EnclosingType.java",
@@ -324,7 +329,7 @@
   }

   @Test
-  public void exemptedEnclosingTypes() {
+  void exemptedEnclosingTypes() {
     helper
         .setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value")
         .addInputLines(
@@ -362,7 +367,7 @@
   }

   @Test
-  public void exemptedEnclosingTypes_importWouldBeAmbiguous() {
+  void exemptedEnclosingTypes_importWouldBeAmbiguous() {
     helper
         .setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value")
         .addInputLines(
@@ -402,7 +407,7 @@
   }

   @Test
-  public void unbatchedFindings() {
+  void unbatchedFindings() {
     compilationHelper
         .addSourceLines(
             "Test.java",
@@ -419,7 +424,7 @@
   }

   @Test
-  public void batchedFindings() {
+  void batchedFindings() {
     compilationHelper
         .addSourceLines(
             "Test.java",
@@ -431,12 +436,12 @@
               java.util.List bar();
             }
             """)
-        .setArgs("-XepOpt:UnnecessarilyFullyQualified:BatchFindings=true")
+        .setArgs("-XepOpt:OtelUnnecessarilyFullyQualified:BatchFindings=true")
         .doTest();
   }

   @Test
-  public void lambdaParameter() {
+  void lambdaParameter() {
     compilationHelper
         .addSourceLines(
             "Lib.java",
@@ -461,7 +466,66 @@
               }
             }
             """)
-        .setArgs("-XepOpt:UnnecessarilyFullyQualified:BatchFindings=true")
+        .setArgs("-XepOpt:OtelUnnecessarilyFullyQualified:BatchFindings=true")
+        .doTest();
+  }
+
+  @Test
+  void applicationClassesNotFlagged() {
+    // application.io.opentelemetry.* classes are shaded copies that must use fully qualified
+    // names to avoid conflicts with the agent's own copy of the classes
+    compilationHelper
+        .addSourceLines(
+            "application/io/opentelemetry/api/trace/SpanKind.java",
+            """
+            package application.io.opentelemetry.api.trace;
+
+            public enum SpanKind {
+              INTERNAL,
+              SERVER,
+              CLIENT
+            }
+            """)
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              application.io.opentelemetry.api.trace.SpanKind method() {
+                return application.io.opentelemetry.api.trace.SpanKind.INTERNAL;
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  void applicationAnnotationsNotFlagged() {
+    compilationHelper
+        .addSourceLines(
+            "application/io/opentelemetry/extension/annotations/WithSpan.java",
+            """
+            package application.io.opentelemetry.extension.annotations;
+
+            import java.lang.annotation.ElementType;
+            import java.lang.annotation.Retention;
+            import java.lang.annotation.RetentionPolicy;
+            import java.lang.annotation.Target;
+
+            @Retention(RetentionPolicy.RUNTIME)
+            @Target(ElementType.METHOD)
+            public @interface WithSpan {
+              String value() default "";
+            }
+            """)
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              application.io.opentelemetry.extension.annotations.WithSpan getAnnotation() {
+                return null;
+              }
+            }
+            """)
         .doTest();
   }
 }

[Copilot]

Pull request overview

This pull request enforces error prone's UnnecessarilyFullyQualified rule by flipping the import convention in instrumentation/opentelemetry-api/opentelemetry-api-* modules. The new convention imports agent classes (io.opentelemetry.*) and uses fully qualified class names (FQCN) for application classes (application.io.opentelemetry.*). This is the reverse of the previous convention but aligns better with the rest of the repository and simplifies the error prone rule configuration.

Changes:

• Reversed import convention: now importing io.opentelemetry.* (agent) and using FQCN for application.io.opentelemetry.* (application)
• Updated error prone configuration to note the custom check handles application.* and repo-specific conventions
• Systematic refactoring across all OpenTelemetry API instrumentation modules

Reviewed changes

Copilot reviewed 144 out of 144 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
conventions/src/main/kotlin/otel.errorprone-conventions.gradle.kts	Updated comment to explain custom OtelUnnecessarilyFullyQualified check
instrumentation/opentelemetry-api/**	Reversed import convention across all API version modules
instrumentation/reactor/**	Applied convention change to reactor instrumentation
instrumentation/spring/spring-webflux/**	Applied convention change to webflux instrumentation
instrumentation-api-incubator/**	Applied convention change and added missing imports
instrumentation/java-util-logging/**	Applied convention change to logging instrumentation

[laurit]

I guess we can always give up on this if maintaining this turns out to be too painful.

[trask]

/easycla