From aa7012154cafa833d48c4c4ced5a51a18c629335 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 4 Dec 2023 17:08:09 -0800 Subject: [PATCH] Internal build change. PiperOrigin-RevId: 587885671 Change-Id: I426b7349412df400b0d2c149da5ff2dd47050e39 --- .../build/buildjar/javac/plugins/BUILD | 3 +- .../dependency/StrictJavaDepsPlugin.java | 253 ++++++++++++++---- 2 files changed, 210 insertions(+), 46 deletions(-) diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD index ad488fed4466bc..be5c27fb8b5e01 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD @@ -1,5 +1,5 @@ -load("@rules_java//java:defs.bzl", "java_library") load("//tools/build_rules:java_rules_skylark.bzl", "bootstrap_java_library") +load("@rules_java//java:defs.bzl", "java_library") # Description: # Plugins for the Java library builders, which are used by Bazel to @@ -30,6 +30,7 @@ java_library( javacopts = [ "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", "--add-exports=jdk.compiler/com.sun.tools.javac.resources=ALL-UNNAMED", "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java index de1ad14a8acb49..829c80ef908117 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java @@ -16,7 +16,12 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.devtools.build.buildjar.javac.plugins.dependency.StrictJavaDepsPlugin.NonPlatformJar.Kind.FOR_JSPECIFY_FROM_PLATFORM; +import static com.google.devtools.build.buildjar.javac.plugins.dependency.StrictJavaDepsPlugin.NonPlatformJar.Kind.IN_CLASSPATH; +import static java.lang.Boolean.parseBoolean; +import static javax.tools.StandardLocation.CLASS_PATH; +import com.google.auto.value.AutoOneOf; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.buildjar.JarOwner; @@ -41,6 +46,7 @@ import com.sun.tools.javac.util.Log; import com.sun.tools.javac.util.Log.WriterKind; import com.sun.tools.javac.util.Name; +import com.sun.tools.javac.util.Names; import java.io.IOException; import java.io.PrintWriter; import java.io.UncheckedIOException; @@ -48,6 +54,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -59,7 +66,9 @@ import java.util.jar.Manifest; import javax.lang.model.element.AnnotationValue; import javax.lang.model.util.SimpleAnnotationValueVisitor8; +import javax.tools.JavaFileManager; import javax.tools.JavaFileObject; +import javax.tools.JavaFileObject.Kind; /** * A plugin for BlazeJavaCompiler that checks for types referenced directly in the source, but @@ -133,9 +142,14 @@ public void init( dependencyModule.getPlatformJars()); checkingTreeScanner = context.get(CheckingTreeScanner.class); if (checkingTreeScanner == null) { - Set platformJars = dependencyModule.getPlatformJars(); checkingTreeScanner = - new CheckingTreeScanner(dependencyModule, diagnostics, missingTargets, platformJars); + new CheckingTreeScanner( + dependencyModule, + diagnostics, + missingTargets, + dependencyModule.getPlatformJars(), + context.get(JavaFileManager.class), + Names.instance(context)); context.put(CheckingTreeScanner.class, checkingTreeScanner); } } @@ -235,24 +249,41 @@ private static class CheckingTreeScanner extends TreeScanner { private final Set seenTargets = new HashSet<>(); - private final Set seenStrictDepsViolatingJars = new HashSet<>(); + private final Set seenStrictDepsViolatingJars = new HashSet<>(); /** The set of jars on the compilation bootclasspath. */ private final Set platformJars; + private final JavaFileManager fileManager; + /** The current source, for diagnostics. */ private JavaFileObject source = null; + /** Cache of classpath (not platform) jars in which given symbols can be found. */ + private final Map> classpathOnlyDepPaths = new HashMap<>(); + + private final Name jspecifyAnnotationsPackage; + private final Name jspecifyNullnessPackage; + + /* TODO(b/297254214): Remove this flag after the depot is clean. */ + private final boolean hidePlatformJspecify; + public CheckingTreeScanner( DependencyModule dependencyModule, List diagnostics, Set missingTargets, - Set platformJars) { + Set platformJars, + JavaFileManager fileManager, + Names names) { this.directJars = dependencyModule.directJars(); this.diagnostics = diagnostics; this.missingTargets = missingTargets; this.directDependenciesMap = dependencyModule.getExplicitDependenciesMap(); this.platformJars = platformJars; + this.fileManager = fileManager; + jspecifyAnnotationsPackage = names.fromString("org.jspecify.annotations"); + jspecifyNullnessPackage = names.fromString("org.jspecify.nullness"); + hidePlatformJspecify = parseBoolean(System.getProperty("hidePlatformJspecify", "true")); } Set getSeenClasses() { @@ -264,12 +295,12 @@ private void checkTypeLiteral(JCTree node, Symbol sym) { if (sym == null || sym.kind != Kinds.Kind.TYP) { return; } - Path jarPath = getJarPath(sym.enclClass(), platformJars); + NonPlatformJar jar = getNonPlatformJar(sym.enclClass(), platformJars); // If this type symbol comes from a class file loaded from a jar, check // whether that jar was a direct dependency and error out otherwise. - if (jarPath != null && seenClasses.add(sym.enclClass())) { - collectExplicitDependency(jarPath, node, sym); + if (jar != null && seenClasses.add(sym.enclClass())) { + collectExplicitDependency(jar, node, sym); } } @@ -277,13 +308,13 @@ private void checkTypeLiteral(JCTree node, Symbol sym) { * Marks the provided dependency as a direct/explicit dependency. Additionally, if * strict_java_deps is enabled, it emits a [strict] compiler warning/error. */ - private void collectExplicitDependency(Path jarPath, JCTree node, Symbol sym) { + private void collectExplicitDependency(NonPlatformJar jar, JCTree node, Symbol sym) { // Does it make sense to emit a warning/error for this pair of (type, owner)? // We want to emit only one error/warning per owner. - if (!directJars.contains(jarPath) && seenStrictDepsViolatingJars.add(jarPath)) { + if (!directJars.contains(jar.pathOrEmpty()) && seenStrictDepsViolatingJars.add(jar)) { // IO cost here is fine because we only hit this path for an explicit dependency // _not_ in the direct jars, i.e. an error - JarOwner owner = readJarOwnerFromManifest(jarPath); + JarOwner owner = readJarOwnerFromManifest(jar); if (seenTargets.add(owner)) { // owner is of the form "//label/of:rule " where is // optional. @@ -309,7 +340,7 @@ private void collectExplicitDependency(Path jarPath, JCTree node, Symbol sym) { } } - if (!directDependenciesMap.containsKey(jarPath)) { + if (!directDependenciesMap.containsKey(jar.pathOrEmpty())) { // Also update the dependency proto Dependency dep = Dependency.newBuilder() @@ -317,14 +348,20 @@ private void collectExplicitDependency(Path jarPath, JCTree node, Symbol sym) { // match the format in params files (which currently always use `/`, see // bazelbuild/bazel#4108). JavaBuilder should always parse Path strings into // java.nio.file.Paths before comparing them. - .setPath(jarPath.toString()) + // + // An empty path is OK in the cases we produce it. See readJarOwnerFromManifest. + .setPath(jar.pathOrEmpty().toString()) .setKind(Dependency.Kind.EXPLICIT) .build(); - directDependenciesMap.put(jarPath, dep); + directDependenciesMap.put(jar.pathOrEmpty(), dep); } } - private static JarOwner readJarOwnerFromManifest(Path jarPath) { + private JarOwner readJarOwnerFromManifest(NonPlatformJar jar) { + if (jar.getKind() == FOR_JSPECIFY_FROM_PLATFORM) { + return JSPECIFY_JAR_OWNER; + } + Path jarPath = jar.inClasspath(); try (JarFile jarFile = new JarFile(jarPath.toFile())) { Manifest manifest = jarFile.getManifest(); if (manifest == null) { @@ -423,6 +460,110 @@ public void visitPackageDef(JCTree.JCPackageDecl tree) { scan(tree.annotations); checkTypeLiteral(tree, tree.packge.package_info); } + + /** + * Returns the name of the classpath jar from which the given class symbol was loaded + * (with an exception for the JSpecify annotations) or else {@code null}. + * + *

If the symbol came from the platform (i.e., system modules/bootclasspath), rather than + * from the classpath, this method usually returns {@code null}. The exception is for + * JSpecify-annotation symbols that are read from the platform: For such a symbol, this method + * still returns the first classpath jar that contains the symbol, or, if no classpath + * jar contains the symbol, it returns {@code FOR_JSPECIFY_FROM_PLATFORM}. (The calling code + * later converts that to {@code JSPECIFY_JAR_OWNER}, which will lead to a strict-deps error, + * since that jar clearly isn't a direct dependency.) In this way, we pretend that the + * JSpecify-annotation symbols aren't part of the platform. That's important because in + * fact they aren't part of it at runtime and so we want to force users of those classes + * to declare a dependency on them. + * + *

This behavior is mildly unfortunate in the unusual situation that a project normally reads + * a JSpecify-annotations class from an uber-jar, rather than from the normal JSpecify target. + * In that case, we claim that the class is being loaded from the normal target. That is not the + * target that the project's developers are likely to want. It's even possible that the class + * isn't present on the reduced classpath but would be present (via the uber-jar) if only + * we compiled with the full classpath. The full-classpath compilation would still produce a + * strict-deps error, but it would produce one that recommends the correct jar/dependency. But + * as this code is, we fail with a suggestion of the normal JSpecify target, and we may or may + * not fall back to the full classpath. + * + *

OK, arguably it's unfortunate that ever we suggest that the normal JSpecify target + * is on the classpath when it isn't really. However, the most common result of that is going to + * be that we produce a more convenient error message. That convenience helps to offset any + * confusion that we produce. Still, we won't introduce similar behavior for other classes + * lightly. + * + * @param platformJars jars on javac's bootclasspath + */ + private NonPlatformJar getNonPlatformJar(ClassSymbol classSymbol, Set platformJars) { + if (classSymbol == null) { + return null; + } + + // Ignore symbols that appear in the sourcepath: + if (haveSourceForSymbol(classSymbol)) { + return null; + } + + JavaFileObject classfile = classSymbol.classfile; + + Path path = ImplicitDependencyExtractor.getJarPath(classfile); + // Filter out classes from the system modules and bootclasspath + if (path == null || platformJars.contains(path)) { + // ...except the JSpecify annotations, which we treat specially. + if (hidePlatformJspecify + && (classSymbol.packge().fullname.equals(jspecifyAnnotationsPackage) + || classSymbol.packge().fullname.equals(jspecifyNullnessPackage))) { + Path classpathJar = findLookingOnlyInClasspath(classSymbol); + return classpathJar != null + ? NonPlatformJar.forClasspathJar(classpathJar) + : NonPlatformJar.FOR_JSPECIFY_FROM_PLATFORM; + } + return null; + } + + return NonPlatformJar.forClasspathJar(path); + } + + /** + * Returns the first jar file in the classpath (not system modules, not bootclasspath) that + * contains the given class or {@code null} if no such jar is available. + */ + private Path findLookingOnlyInClasspath(ClassSymbol sym) { + /* + * computeIfAbsent doesn't cache null results, so we store Optional instances instead. + * + * In practice, that won't normally matter much: The only case in which our computation + * function runs once per usage of a JSpecify-annotation class is the failing-build case—that + * is, when the class is not on the classpath. + */ + return classpathOnlyDepPaths + .computeIfAbsent( + sym, + (unused) -> { + try { + for (JavaFileObject file : + fileManager.list( + CLASS_PATH, + sym.packge().fullname.toString(), + ImmutableSet.of(Kind.CLASS), + false /* do not return classes in subpackages */)) { + /* + * The query above returns all classpath classes from the given package. We can + * imagine situations in which only *some* JSpecify annotations are present in a + * given classpath jar (an uber-jar with unused classes removed?), so we want to + * make sure that we found the class we want. + */ + if (file.isNameCompatible(sym.getSimpleName().toString(), Kind.CLASS)) { + return Optional.of(ImplicitDependencyExtractor.getJarPath(file)); + } + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } + return Optional.empty(); + }) + .orElse(null); + } } /** @@ -507,37 +648,6 @@ static String canonicalizeTarget(String target) { return target; } - /** - * Returns the name of the jar file from which the given class symbol was loaded, if available, - * and null otherwise. Implicitly filters out jars from the compilation bootclasspath. - * - * @param platformJars jars on javac's bootclasspath - */ - public static Path getJarPath(ClassSymbol classSymbol, Set platformJars) { - if (classSymbol == null) { - return null; - } - - // Ignore symbols that appear in the sourcepath: - if (haveSourceForSymbol(classSymbol)) { - return null; - } - - JavaFileObject classfile = classSymbol.classfile; - - Path path = ImplicitDependencyExtractor.getJarPath(classfile); - if (path == null) { - return null; - } - - // Filter out classes on bootclasspath - if (platformJars.contains(path)) { - return null; - } - - return path; - } - /** Returns true if the given classSymbol corresponds to one of the sources being compiled. */ private static boolean haveSourceForSymbol(ClassSymbol classSymbol) { if (classSymbol.sourcefile == null) { @@ -559,4 +669,57 @@ private static boolean haveSourceForSymbol(ClassSymbol classSymbol) { public boolean runOnAttributionErrors() { return true; } + + /** + * Either a jar in the classpath or the well-known jar that contains the classes that are present + * in the platform at compile time but not runtime. + */ + @AutoOneOf(NonPlatformJar.Kind.class) + abstract static class NonPlatformJar { + enum Kind { + IN_CLASSPATH, + FOR_JSPECIFY_FROM_PLATFORM, + } + + abstract Kind getKind(); + + abstract Path inClasspath(); + + abstract Placeholder forJspecifyFromPlatform(); + + final Path pathOrEmpty() { + return getKind() == IN_CLASSPATH ? inClasspath() : EMPTY_PATH; + } + + static NonPlatformJar forClasspathJar(Path s) { + return AutoOneOf_StrictJavaDepsPlugin_NonPlatformJar.inClasspath(s); + } + + static final NonPlatformJar FOR_JSPECIFY_FROM_PLATFORM = + AutoOneOf_StrictJavaDepsPlugin_NonPlatformJar.forJspecifyFromPlatform(Placeholder.INSTANCE); + } + + enum Placeholder { + INSTANCE + } + + private static final Path EMPTY_PATH = Path.of(""); + + /** + * A special-purpose {@link JarOwner} instance that points to the main JSpecify target but is used + * when the JSpecify annotations are instead read from the platform. + * + *

We use this instance to force users to add the explicit JSpecify dependency—again, even + * though the annotations are present in the compile-time platform (i.e., bootclasspath or system + * modules). We require users to add the dependency because the annotations are not present + * in the runtime platform. + * + *

The {@link Path} argument that we pass to this instance doesn't matter because the build is + * usually going to fail. (Or, if a strict-deps enforcement is disabled, the user can't reasonably + * expect fully accurate dependency information, and our tools should be mostly resilient to an + * empty path.) + */ + private static final JarOwner JSPECIFY_JAR_OWNER = + JarOwner.create( + EMPTY_PATH, "//third_party/java/jspecify_annotations", /* aspect= */ Optional.empty()); }