Skip to content

Commit

Permalink
Internal build change.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 587885671
Change-Id: I426b7349412df400b0d2c149da5ff2dd47050e39
  • Loading branch information
cpovirk authored and copybara-github committed Dec 5, 2023
1 parent 96b3612 commit aa70121
Show file tree
Hide file tree
Showing 2 changed files with 210 additions and 46 deletions.
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -41,13 +46,15 @@
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;
import java.lang.reflect.Method;
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;
Expand All @@ -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
Expand Down Expand Up @@ -133,9 +142,14 @@ public void init(
dependencyModule.getPlatformJars());
checkingTreeScanner = context.get(CheckingTreeScanner.class);
if (checkingTreeScanner == null) {
Set<Path> 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);
}
}
Expand Down Expand Up @@ -235,24 +249,41 @@ private static class CheckingTreeScanner extends TreeScanner {

private final Set<JarOwner> seenTargets = new HashSet<>();

private final Set<Path> seenStrictDepsViolatingJars = new HashSet<>();
private final Set<NonPlatformJar> seenStrictDepsViolatingJars = new HashSet<>();

/** The set of jars on the compilation bootclasspath. */
private final Set<Path> 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<ClassSymbol, Optional<Path>> 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<SjdDiagnostic> diagnostics,
Set<JarOwner> missingTargets,
Set<Path> platformJars) {
Set<Path> 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"));

This comment has been minimized.

Copy link
@fmeum

fmeum Dec 5, 2023

Collaborator

@cushon @cpovirk I haven't tested it, but this seems potentially unsafe for Bazel as a default. Could it result in unexpected strict deps behavior for jspecify with fixups recommending a google3 target?

This comment has been minimized.

Copy link
@cpovirk

cpovirk Dec 5, 2023

Author Contributor

Hi, thanks for checking on this. I think it should be safe, but @cushon will have better judgment, and either of you may know of concerns that I don't.

The only case in which this commit affects behavior is if the org.jspecify annotations are part of the bootclasspath or system modules. This isn't the case for a normal JVM or Android build: Anyone who uses the JSpecify annotations would put them on the normal classpath. We're just arranging for the bootclasspath/system-module approach for some of our builds inside Google.

I wouldn't expect other people to do this. (We might dream that someday we could make it practical for others to do so, but it's not coming anytime soon :)) In fact, we had hoped to not need to do it, but it became necessary to support some other nullness work that we're doing.

All that said, it would not be hard for us to push another commit to flip the default value of the flag and then to keep the flag in place for Google to opt in indefinitely. (Nor would it be hard to add the flag back if we were to remove it after all :)) And I don't think that one more flag to JavaBuilder internally would cause us any trouble.

This comment has been minimized.

Copy link
@fmeum

fmeum Dec 5, 2023

Collaborator

Thanks for the quick reply and the work on JSpecify! If this is really limited to jspecify in the bootclasspath/system modules, then I don't see a realistic risk for Bazel users using JSpecify.

}

Set<ClassSymbol> getSeenClasses() {
Expand All @@ -264,26 +295,26 @@ 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);
}
}

/**
* 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 <Aspect name>" where <Aspect name> is
// optional.
Expand All @@ -309,22 +340,28 @@ 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()
// Path.toString uses the platform separator (`\` on Windows) which may not
// 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) {
Expand Down Expand Up @@ -423,6 +460,110 @@ public void visitPackageDef(JCTree.JCPackageDecl tree) {
scan(tree.annotations);
checkTypeLiteral(tree, tree.packge.package_info);
}

/**
* Returns the name of the <i>classpath</i> jar from which the given class symbol was loaded
* (with an exception for the JSpecify annotations) or else {@code null}.
*
* <p>If the symbol came from the platform (i.e., system modules/bootclasspath), rather than
* from the classpath, this method <i>usually</i> 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 <i>classpath</i> 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 <i>aren't</i> part of the platform. That's important because in
* fact they aren't part of it <i>at runtime</i> and so we want to force users of those classes
* to declare a dependency on them.
*
* <p>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 <i>would</i> 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.
*
* <p>OK, arguably it's unfortunate that <i>ever</i> 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<Path> 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);
}
}

/**
Expand Down Expand Up @@ -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<Path> 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) {
Expand All @@ -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.
*
* <p>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 <i>not</i> present
* in the <i>runtime</i> platform.
*
* <p>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());
}

0 comments on commit aa70121

Please sign in to comment.