From 748b2d829a5329ca2d87c8ddcdaf7bfc86325fb9 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 19 Nov 2020 13:52:06 -0800 Subject: [PATCH] Support extra checks for ErrorProne in a dedicated property Previous extra checks for ErrorProne were added using the plugins proeprty to get them into the -processorpath argument. This works fine for java-only modules, but fails for mixed java+kotlin modules because the processorpath is given to kapt and not javac. Add a dedicated errorprone.extra_check_modules property (mirroring the lint.extra_check_modules property), and add that to a separate processorpath that is used only for errorprone rules and not cleared when kotlin is used. Test: TestKapt/errorprone Change-Id: Id6ef02ce758532d1df8b8d969fad83bb44fe93ab --- java/java.go | 45 ++++++++----- java/kotlin_test.go | 158 ++++++++++++++++++++++++++++++-------------- 2 files changed, 137 insertions(+), 66 deletions(-) diff --git a/java/java.go b/java/java.go index 0739eab04..1298b9e47 100644 --- a/java/java.go +++ b/java/java.go @@ -248,6 +248,9 @@ type CompilerProperties struct { Errorprone struct { // List of javac flags that should only be used when running errorprone. Javacflags []string + + // List of java_plugin modules that provide extra errorprone checks. + Extra_check_modules []string } Proto struct { @@ -569,6 +572,7 @@ var ( libTag = dependencyTag{name: "javalib"} java9LibTag = dependencyTag{name: "java9lib"} pluginTag = dependencyTag{name: "plugin"} + errorpronePluginTag = dependencyTag{name: "errorprone-plugin"} exportedPluginTag = dependencyTag{name: "exported-plugin"} bootClasspathTag = dependencyTag{name: "bootclasspath"} systemModulesTag = dependencyTag{name: "system modules"} @@ -765,6 +769,7 @@ func (j *Module) deps(ctx android.BottomUpMutatorContext) { } ctx.AddFarVariationDependencies(ctx.Config().BuildOSCommonTarget.Variations(), pluginTag, j.properties.Plugins...) + ctx.AddFarVariationDependencies(ctx.Config().BuildOSCommonTarget.Variations(), errorpronePluginTag, j.properties.Errorprone.Extra_check_modules...) ctx.AddFarVariationDependencies(ctx.Config().BuildOSCommonTarget.Variations(), exportedPluginTag, j.properties.Exported_plugins...) android.ProtoDeps(ctx, &j.protoProperties) @@ -852,21 +857,22 @@ func (j *Module) aidlFlags(ctx android.ModuleContext, aidlPreprocess android.Opt } type deps struct { - classpath classpath - java9Classpath classpath - bootClasspath classpath - processorPath classpath - processorClasses []string - staticJars android.Paths - staticHeaderJars android.Paths - staticResourceJars android.Paths - aidlIncludeDirs android.Paths - srcs android.Paths - srcJars android.Paths - systemModules *systemModules - aidlPreprocess android.OptionalPath - kotlinStdlib android.Paths - kotlinAnnotations android.Paths + classpath classpath + java9Classpath classpath + bootClasspath classpath + processorPath classpath + errorProneProcessorPath classpath + processorClasses []string + staticJars android.Paths + staticHeaderJars android.Paths + staticResourceJars android.Paths + aidlIncludeDirs android.Paths + srcs android.Paths + srcJars android.Paths + systemModules *systemModules + aidlPreprocess android.OptionalPath + kotlinStdlib android.Paths + kotlinAnnotations android.Paths disableTurbine bool } @@ -1068,6 +1074,12 @@ func (j *Module) collectDeps(ctx android.ModuleContext) deps { } else { ctx.PropertyErrorf("plugins", "%q is not a java_plugin module", otherName) } + case errorpronePluginTag: + if plugin, ok := dep.(*Plugin); ok { + deps.errorProneProcessorPath = append(deps.errorProneProcessorPath, plugin.ImplementationAndResourcesJars()...) + } else { + ctx.PropertyErrorf("plugins", "%q is not a java_plugin module", otherName) + } case exportedPluginTag: if plugin, ok := dep.(*Plugin); ok { if plugin.pluginProperties.Generates_api != nil && *plugin.pluginProperties.Generates_api { @@ -1191,7 +1203,7 @@ func (j *Module) collectBuilderFlags(ctx android.ModuleContext, deps deps) javaB flags.javaVersion = getJavaVersion(ctx, String(j.properties.Java_version), sdkContext(j)) if ctx.Config().RunErrorProne() { - if config.ErrorProneClasspath == nil { + if config.ErrorProneClasspath == nil && ctx.Config().TestProductVariables == nil { ctx.ModuleErrorf("cannot build with Error Prone, missing external/error_prone?") } @@ -1211,6 +1223,7 @@ func (j *Module) collectBuilderFlags(ctx android.ModuleContext, deps deps) javaB flags.classpath = append(flags.classpath, deps.classpath...) flags.java9Classpath = append(flags.java9Classpath, deps.java9Classpath...) flags.processorPath = append(flags.processorPath, deps.processorPath...) + flags.errorProneProcessorPath = append(flags.errorProneProcessorPath, deps.errorProneProcessorPath...) flags.processors = append(flags.processors, deps.processorClasses...) flags.processors = android.FirstUniqueStrings(flags.processors) diff --git a/java/kotlin_test.go b/java/kotlin_test.go index 60ca1c476..77ef29456 100644 --- a/java/kotlin_test.go +++ b/java/kotlin_test.go @@ -84,11 +84,14 @@ func TestKotlin(t *testing.T) { } func TestKapt(t *testing.T) { - ctx, _ := testJava(t, ` + bp := ` java_library { name: "foo", srcs: ["a.java", "b.kt"], plugins: ["bar", "baz"], + errorprone: { + extra_check_modules: ["my_check"], + }, } java_plugin { @@ -102,64 +105,119 @@ func TestKapt(t *testing.T) { processor_class: "com.baz", srcs: ["b.java"], } - `) - buildOS := android.BuildOs.String() + java_plugin { + name: "my_check", + srcs: ["b.java"], + } + ` + t.Run("", func(t *testing.T) { + ctx, _ := testJava(t, bp) - kapt := ctx.ModuleForTests("foo", "android_common").Rule("kapt") - kotlinc := ctx.ModuleForTests("foo", "android_common").Rule("kotlinc") - javac := ctx.ModuleForTests("foo", "android_common").Rule("javac") + buildOS := android.BuildOs.String() - bar := ctx.ModuleForTests("bar", buildOS+"_common").Rule("javac").Output.String() - baz := ctx.ModuleForTests("baz", buildOS+"_common").Rule("javac").Output.String() + kapt := ctx.ModuleForTests("foo", "android_common").Rule("kapt") + kotlinc := ctx.ModuleForTests("foo", "android_common").Rule("kotlinc") + javac := ctx.ModuleForTests("foo", "android_common").Rule("javac") - // Test that the kotlin and java sources are passed to kapt and kotlinc - if len(kapt.Inputs) != 2 || kapt.Inputs[0].String() != "a.java" || kapt.Inputs[1].String() != "b.kt" { - t.Errorf(`foo kapt inputs %v != ["a.java", "b.kt"]`, kapt.Inputs) - } - if len(kotlinc.Inputs) != 2 || kotlinc.Inputs[0].String() != "a.java" || kotlinc.Inputs[1].String() != "b.kt" { - t.Errorf(`foo kotlinc inputs %v != ["a.java", "b.kt"]`, kotlinc.Inputs) - } + bar := ctx.ModuleForTests("bar", buildOS+"_common").Rule("javac").Output.String() + baz := ctx.ModuleForTests("baz", buildOS+"_common").Rule("javac").Output.String() - // Test that only the java sources are passed to javac - if len(javac.Inputs) != 1 || javac.Inputs[0].String() != "a.java" { - t.Errorf(`foo inputs %v != ["a.java"]`, javac.Inputs) - } + // Test that the kotlin and java sources are passed to kapt and kotlinc + if len(kapt.Inputs) != 2 || kapt.Inputs[0].String() != "a.java" || kapt.Inputs[1].String() != "b.kt" { + t.Errorf(`foo kapt inputs %v != ["a.java", "b.kt"]`, kapt.Inputs) + } + if len(kotlinc.Inputs) != 2 || kotlinc.Inputs[0].String() != "a.java" || kotlinc.Inputs[1].String() != "b.kt" { + t.Errorf(`foo kotlinc inputs %v != ["a.java", "b.kt"]`, kotlinc.Inputs) + } - // Test that the kapt srcjar is a dependency of kotlinc and javac rules - if !inList(kapt.Output.String(), kotlinc.Implicits.Strings()) { - t.Errorf("expected %q in kotlinc implicits %v", kapt.Output.String(), kotlinc.Implicits.Strings()) - } - if !inList(kapt.Output.String(), javac.Implicits.Strings()) { - t.Errorf("expected %q in javac implicits %v", kapt.Output.String(), javac.Implicits.Strings()) - } + // Test that only the java sources are passed to javac + if len(javac.Inputs) != 1 || javac.Inputs[0].String() != "a.java" { + t.Errorf(`foo inputs %v != ["a.java"]`, javac.Inputs) + } - // Test that the kapt srcjar is extracted by the kotlinc and javac rules - if kotlinc.Args["srcJars"] != kapt.Output.String() { - t.Errorf("expected %q in kotlinc srcjars %v", kapt.Output.String(), kotlinc.Args["srcJars"]) - } - if javac.Args["srcJars"] != kapt.Output.String() { - t.Errorf("expected %q in javac srcjars %v", kapt.Output.String(), kotlinc.Args["srcJars"]) - } + // Test that the kapt srcjar is a dependency of kotlinc and javac rules + if !inList(kapt.Output.String(), kotlinc.Implicits.Strings()) { + t.Errorf("expected %q in kotlinc implicits %v", kapt.Output.String(), kotlinc.Implicits.Strings()) + } + if !inList(kapt.Output.String(), javac.Implicits.Strings()) { + t.Errorf("expected %q in javac implicits %v", kapt.Output.String(), javac.Implicits.Strings()) + } - // Test that the processors are passed to kapt - expectedProcessorPath := "-P plugin:org.jetbrains.kotlin.kapt3:apclasspath=" + bar + - " -P plugin:org.jetbrains.kotlin.kapt3:apclasspath=" + baz - if kapt.Args["kaptProcessorPath"] != expectedProcessorPath { - t.Errorf("expected kaptProcessorPath %q, got %q", expectedProcessorPath, kapt.Args["kaptProcessorPath"]) - } - expectedProcessor := "-P plugin:org.jetbrains.kotlin.kapt3:processors=com.bar -P plugin:org.jetbrains.kotlin.kapt3:processors=com.baz" - if kapt.Args["kaptProcessor"] != expectedProcessor { - t.Errorf("expected kaptProcessor %q, got %q", expectedProcessor, kapt.Args["kaptProcessor"]) - } + // Test that the kapt srcjar is extracted by the kotlinc and javac rules + if kotlinc.Args["srcJars"] != kapt.Output.String() { + t.Errorf("expected %q in kotlinc srcjars %v", kapt.Output.String(), kotlinc.Args["srcJars"]) + } + if javac.Args["srcJars"] != kapt.Output.String() { + t.Errorf("expected %q in javac srcjars %v", kapt.Output.String(), kotlinc.Args["srcJars"]) + } - // Test that the processors are not passed to javac - if javac.Args["processorPath"] != "" { - t.Errorf("expected processorPath '', got %q", javac.Args["processorPath"]) - } - if javac.Args["processor"] != "-proc:none" { - t.Errorf("expected processor '-proc:none', got %q", javac.Args["processor"]) - } + // Test that the processors are passed to kapt + expectedProcessorPath := "-P plugin:org.jetbrains.kotlin.kapt3:apclasspath=" + bar + + " -P plugin:org.jetbrains.kotlin.kapt3:apclasspath=" + baz + if kapt.Args["kaptProcessorPath"] != expectedProcessorPath { + t.Errorf("expected kaptProcessorPath %q, got %q", expectedProcessorPath, kapt.Args["kaptProcessorPath"]) + } + expectedProcessor := "-P plugin:org.jetbrains.kotlin.kapt3:processors=com.bar -P plugin:org.jetbrains.kotlin.kapt3:processors=com.baz" + if kapt.Args["kaptProcessor"] != expectedProcessor { + t.Errorf("expected kaptProcessor %q, got %q", expectedProcessor, kapt.Args["kaptProcessor"]) + } + + // Test that the processors are not passed to javac + if javac.Args["processorpath"] != "" { + t.Errorf("expected processorPath '', got %q", javac.Args["processorpath"]) + } + if javac.Args["processor"] != "-proc:none" { + t.Errorf("expected processor '-proc:none', got %q", javac.Args["processor"]) + } + }) + + t.Run("errorprone", func(t *testing.T) { + env := map[string]string{ + "RUN_ERROR_PRONE": "true", + } + config := testConfig(env, bp, nil) + ctx, _ := testJavaWithConfig(t, config) + + buildOS := android.BuildOs.String() + + kapt := ctx.ModuleForTests("foo", "android_common").Rule("kapt") + //kotlinc := ctx.ModuleForTests("foo", "android_common").Rule("kotlinc") + javac := ctx.ModuleForTests("foo", "android_common").Description("javac") + errorprone := ctx.ModuleForTests("foo", "android_common").Description("errorprone") + + bar := ctx.ModuleForTests("bar", buildOS+"_common").Description("javac").Output.String() + baz := ctx.ModuleForTests("baz", buildOS+"_common").Description("javac").Output.String() + myCheck := ctx.ModuleForTests("my_check", buildOS+"_common").Description("javac").Output.String() + + // Test that the errorprone plugins are not passed to kapt + expectedProcessorPath := "-P plugin:org.jetbrains.kotlin.kapt3:apclasspath=" + bar + + " -P plugin:org.jetbrains.kotlin.kapt3:apclasspath=" + baz + if kapt.Args["kaptProcessorPath"] != expectedProcessorPath { + t.Errorf("expected kaptProcessorPath %q, got %q", expectedProcessorPath, kapt.Args["kaptProcessorPath"]) + } + expectedProcessor := "-P plugin:org.jetbrains.kotlin.kapt3:processors=com.bar -P plugin:org.jetbrains.kotlin.kapt3:processors=com.baz" + if kapt.Args["kaptProcessor"] != expectedProcessor { + t.Errorf("expected kaptProcessor %q, got %q", expectedProcessor, kapt.Args["kaptProcessor"]) + } + + // Test that the errorprone plugins are not passed to javac + if javac.Args["processorpath"] != "" { + t.Errorf("expected processorPath '', got %q", javac.Args["processorpath"]) + } + if javac.Args["processor"] != "-proc:none" { + t.Errorf("expected processor '-proc:none', got %q", javac.Args["processor"]) + } + + // Test that the errorprone plugins are passed to errorprone + expectedProcessorPath = "-processorpath " + myCheck + if errorprone.Args["processorpath"] != expectedProcessorPath { + t.Errorf("expected processorpath %q, got %q", expectedProcessorPath, errorprone.Args["processorpath"]) + } + if errorprone.Args["processor"] != "-proc:none" { + t.Errorf("expected processor '-proc:none', got %q", errorprone.Args["processor"]) + } + }) } func TestKaptEncodeFlags(t *testing.T) {