Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag to disable cross-compilation root validation. #2578

Merged
merged 1 commit into from
Apr 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package dagger.hilt.android.processor.internal.androidentrypoint;

import static dagger.hilt.processor.internal.HiltCompilerOptions.BooleanOption.DISABLE_ANDROID_SUPERCLASS_VALIDATION;
import static dagger.hilt.processor.internal.HiltCompilerOptions.isAndroidSuperclassValidationDisabled;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet;

import com.google.auto.common.MoreElements;
Expand Down Expand Up @@ -245,7 +245,7 @@ private static AndroidEntryPointMetadata of(
final TypeElement baseElement;
final ClassName generatedClassName;
boolean requiresBytecodeInjection =
DISABLE_ANDROID_SUPERCLASS_VALIDATION.get(env)
isAndroidSuperclassValidationDisabled(androidEntryPointElement, env)
&& MoreTypes.isTypeOf(Void.class, androidEntryPointClassValue.asType());
if (requiresBytecodeInjection) {
baseElement = MoreElements.asType(env.getTypeUtils().asElement(androidEntryPointElement.getSuperclass()));
Expand Down
5 changes: 5 additions & 0 deletions java/dagger/hilt/processor/internal/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,11 @@ java_library(
java_library(
name = "compiler_options",
srcs = ["HiltCompilerOptions.java"],
deps = [
":processor_errors",
"//java/dagger/internal/guava:collect",
"@google_bazel_common//third_party/java/javapoet",
],
)

filegroup(
Expand Down
74 changes: 55 additions & 19 deletions java/dagger/hilt/processor/internal/HiltCompilerOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,36 +16,74 @@

package dagger.hilt.processor.internal;

import com.google.common.collect.ImmutableSet;
import java.util.Arrays;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.processing.ProcessingEnvironment;
import javax.lang.model.element.TypeElement;

/** Hilt annotation processor options. */
// TODO(danysantiago): Consider consolidating with Dagger compiler options logic.
public final class HiltCompilerOptions {

/**
* Returns {@code true} if the superclass validation is disabled for
* {@link dagger.hilt.android.AndroidEntryPoint}-annotated classes.
*
* This flag is for internal use only! The superclass validation checks that the super class is a
* generated {@code Hilt_} class. This flag is disabled by the Hilt Gradle plugin to enable
* bytecode transformation to change the superclass.
*/
public static boolean isAndroidSuperclassValidationDisabled(
TypeElement element, ProcessingEnvironment env) {
BooleanOption option = BooleanOption.DISABLE_ANDROID_SUPERCLASS_VALIDATION;
return option.get(env);
}

/**
* Returns {@code true} if cross-compilation root validation is disabled.
*
* <p>This flag should rarely be needed, but may be used for legacy/migration purposes if
* tests require the use of {@link dagger.hilt.android.HiltAndroidApp} rather than
* {@link dagger.hilt.android.testing.HiltAndroidTest}.
*
* <p>Note that Hilt still does validation within a single compilation unit. In particular,
* a compilation unit that contains a {@code HiltAndroidApp} usage cannot have other
* {@code HiltAndroidApp} or {@code HiltAndroidTest} usages in the same compilation unit.
*/
public static boolean isCrossCompilationRootValidationDisabled(
ImmutableSet<TypeElement> rootElements, ProcessingEnvironment env) {
BooleanOption option = BooleanOption.DISABLE_CROSS_COMPILATION_ROOT_VALIDATION;
return option.get(env);
}

/** Returns {@code true} if the check for {@link dagger.hilt.InstallIn} is disabled. */
public static boolean isModuleInstallInCheckDisabled(ProcessingEnvironment env) {
return BooleanOption.DISABLE_MODULES_HAVE_INSTALL_IN_CHECK.get(env);
}

/**
* Returns {@code true} of unit tests should try to share generated components, rather than using
* separate generated components per Hilt test root.
*
* <p>Tests that provide their own test bindings (e.g. using {@link
* dagger.hilt.android.testing.BindValue} or a test {@link dagger.Module}) cannot use the shared
* component. In these cases, a component will be generated for the test.
*/
public static boolean isSharedTestComponentsEnabled(ProcessingEnvironment env) {
return BooleanOption.SHARE_TEST_COMPONENTS.get(env);
}

/** Processor options which can have true or false values. */
public enum BooleanOption {
/**
* Flag that disables validating the superclass of @AndroidEntryPoint are Hilt_ generated,
* classes. This flag is to be used internally by the Gradle plugin, enabling the bytecode
* transformation to change the superclass.
*/
private enum BooleanOption {
DISABLE_ANDROID_SUPERCLASS_VALIDATION(
"android.internal.disableAndroidSuperclassValidation", false),

/** Flag that disables check on modules to be annotated with @InstallIn. */
DISABLE_CROSS_COMPILATION_ROOT_VALIDATION("disableCrossCompilationRootValidation", false),

DISABLE_MODULES_HAVE_INSTALL_IN_CHECK("disableModulesHaveInstallInCheck", false),

/**
* Flag that enables unit tests to share a single generated Component, rather than using a
* separate generated Component per Hilt test root.
*
* <p>Tests that provide their own test bindings (e.g. using {@link
* dagger.hilt.android.testing.BindValue} or a test {@link dagger.Module}) cannot use the shared
* component. In these cases, a component will be generated for the test.
*/
SHARE_TEST_COMPONENTS("shareTestComponents", false);

private final String name;
Expand All @@ -56,7 +94,7 @@ public enum BooleanOption {
this.defaultValue = defaultValue;
}

public boolean get(ProcessingEnvironment env) {
boolean get(ProcessingEnvironment env) {
String value = env.getOptions().get(getQualifiedName());
if (value == null) {
return defaultValue;
Expand All @@ -65,7 +103,7 @@ public boolean get(ProcessingEnvironment env) {
return Boolean.parseBoolean(value);
}

public String getQualifiedName() {
String getQualifiedName() {
return "dagger.hilt." + name;
}
}
Expand All @@ -75,6 +113,4 @@ public static Set<String> getProcessorOptions() {
.map(BooleanOption::getQualifiedName)
.collect(Collectors.toSet());
}

private HiltCompilerOptions() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import static com.google.auto.common.MoreElements.asType;
import static com.google.auto.common.MoreElements.getPackage;
import static com.google.common.collect.Iterables.getOnlyElement;
import static dagger.hilt.processor.internal.HiltCompilerOptions.BooleanOption.DISABLE_MODULES_HAVE_INSTALL_IN_CHECK;
import static dagger.hilt.processor.internal.HiltCompilerOptions.isModuleInstallInCheckDisabled;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableList;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet;
import static javax.lang.model.element.ElementKind.CLASS;
Expand Down Expand Up @@ -398,7 +398,7 @@ private static boolean isValidKind(Element element) {
}

private boolean installInCheckDisabled(Element element) {
return DISABLE_MODULES_HAVE_INSTALL_IN_CHECK.get(getProcessingEnv())
return isModuleInstallInCheckDisabled(getProcessingEnv())
|| Processors.hasAnnotation(element, ClassNames.DISABLE_INSTALL_IN_CHECK);
}

Expand Down
4 changes: 2 additions & 2 deletions java/dagger/hilt/processor/internal/root/RootMetadata.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Suppliers.memoize;
import static dagger.hilt.processor.internal.HiltCompilerOptions.BooleanOption.SHARE_TEST_COMPONENTS;
import static dagger.hilt.processor.internal.HiltCompilerOptions.isSharedTestComponentsEnabled;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet;
import static javax.lang.model.element.Modifier.ABSTRACT;
import static javax.lang.model.element.Modifier.PRIVATE;
Expand Down Expand Up @@ -133,7 +133,7 @@ public ImmutableSet<TypeElement> modules(ClassName componentName) {
// TODO(groakley): Allow more tests to share modules, e.g. tests that uninstall the same module.
// In that case, this might instead return which shared dep grouping should be used.
public boolean canShareTestComponents() {
return SHARE_TEST_COMPONENTS.get(env)
return isSharedTestComponentsEnabled(env)
&& root.isTestRoot()
&& !deps.includesTestDeps(root.classname());
}
Expand Down
55 changes: 30 additions & 25 deletions java/dagger/hilt/processor/internal/root/RootProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
package dagger.hilt.processor.internal.root;

import static com.google.common.base.Preconditions.checkState;
import static dagger.hilt.processor.internal.HiltCompilerOptions.BooleanOption.SHARE_TEST_COMPONENTS;
import static dagger.hilt.processor.internal.HiltCompilerOptions.isCrossCompilationRootValidationDisabled;
import static dagger.hilt.processor.internal.HiltCompilerOptions.isSharedTestComponentsEnabled;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableList;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet;
import static java.util.Comparator.comparing;
Expand Down Expand Up @@ -136,7 +137,7 @@ public void postRoundProcess(RoundEnvironment roundEnv) throws Exception {

boolean isTestEnv = rootsToProcess.stream().anyMatch(Root::isTestRoot);
ComponentNames componentNames =
isTestEnv && SHARE_TEST_COMPONENTS.get(getProcessingEnv())
isTestEnv && isSharedTestComponentsEnabled(getProcessingEnv())
? ComponentNames.withRenamingIntoPackage(
ClassNames.DEFAULT_ROOT.packageName(),
rootsToProcess.stream().map(Root::element).collect(toImmutableList()))
Expand Down Expand Up @@ -202,30 +203,14 @@ private void validateRoots(ImmutableSet<Root> allRoots, ImmutableSet<Root> roots
.sorted(QUALIFIED_NAME_COMPARATOR)
.collect(toImmutableSet());

ImmutableSet<TypeElement> processedTestRootElements =
allRoots.stream()
.filter(Root::isTestRoot)
.filter(root -> !rootsToProcess.contains(root))
.map(Root::element)
.sorted(QUALIFIED_NAME_COMPARATOR)
.collect(toImmutableSet());

// TODO(b/185742783): Add an explanation or link to docs to explain why we're forbidding this.
ProcessorErrors.checkState(
processedTestRootElements.isEmpty(),
"Cannot process new roots when there are test roots from a previous compilation unit:"
+ "\n\tTest roots from previous compilation unit: %s"
+ "\n\tAll roots from this compilation unit: %s",
processedTestRootElements,
rootElementsToProcess);

ImmutableSet<TypeElement> appRootElementsToProcess =
rootsToProcess.stream()
.filter(root -> !root.isTestRoot())
.map(Root::element)
.sorted(QUALIFIED_NAME_COMPARATOR)
.collect(toImmutableSet());

// Perform validation between roots in this compilation unit.
if (!appRootElementsToProcess.isEmpty()) {
ImmutableSet<TypeElement> testRootElementsToProcess =
rootsToProcess.stream()
Expand All @@ -242,6 +227,31 @@ private void validateRoots(ImmutableSet<Root> allRoots, ImmutableSet<Root> roots
appRootElementsToProcess,
testRootElementsToProcess);

ProcessorErrors.checkState(
appRootElementsToProcess.size() == 1,
"Cannot process multiple app roots in the same compilation unit: %s",
appRootElementsToProcess);
}

// Perform validation across roots previous compilation units.
if (!isCrossCompilationRootValidationDisabled(rootElementsToProcess, getProcessingEnv())) {
ImmutableSet<TypeElement> processedTestRootElements =
allRoots.stream()
.filter(Root::isTestRoot)
.filter(root -> !rootsToProcess.contains(root))
.map(Root::element)
.sorted(QUALIFIED_NAME_COMPARATOR)
.collect(toImmutableSet());

// TODO(b/185742783): Add an explanation or link to docs to explain why we're forbidding this.
ProcessorErrors.checkState(
processedTestRootElements.isEmpty(),
"Cannot process new roots when there are test roots from a previous compilation unit:"
+ "\n\tTest roots from previous compilation unit: %s"
+ "\n\tAll roots from this compilation unit: %s",
processedTestRootElements,
rootElementsToProcess);

ImmutableSet<TypeElement> processedAppRootElements =
allRoots.stream()
.filter(root -> !root.isTestRoot())
Expand All @@ -251,18 +261,13 @@ private void validateRoots(ImmutableSet<Root> allRoots, ImmutableSet<Root> roots
.collect(toImmutableSet());

ProcessorErrors.checkState(
processedAppRootElements.isEmpty(),
processedAppRootElements.isEmpty() || appRootElementsToProcess.isEmpty(),
"Cannot process app roots in this compilation unit since there are app roots in a "
+ "previous compilation unit:"
+ "\n\tApp roots in previous compilation unit: %s"
+ "\n\tApp roots in this compilation unit: %s",
processedAppRootElements,
appRootElementsToProcess);

ProcessorErrors.checkState(
appRootElementsToProcess.size() == 1,
"Cannot process multiple app roots in the same compilation unit: %s",
appRootElementsToProcess);
}
}

Expand Down
3 changes: 3 additions & 0 deletions javatests/dagger/hilt/processor/internal/root/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ compiler_test(
"@maven//:androidx_test_core",
],
deps = [
"//java/dagger/internal/guava:collect",
"//javatests/dagger/hilt/android/processor:android_compilers",
"@google_bazel_common//third_party/java/compile_testing",
"@google_bazel_common//third_party/java/junit",
Expand Down Expand Up @@ -70,6 +71,7 @@ compiler_test(
"@maven//:androidx_test_core",
],
deps = [
"//java/dagger/internal/guava:collect",
"//javatests/dagger/hilt/android/processor:android_compilers",
"@google_bazel_common//third_party/java/compile_testing",
"@google_bazel_common//third_party/java/junit",
Expand All @@ -90,6 +92,7 @@ compiler_test(
"@maven//:androidx_test_core",
],
deps = [
"//java/dagger/internal/guava:collect",
"//javatests/dagger/hilt/android/processor:android_compilers",
"@google_bazel_common//third_party/java/compile_testing",
"@google_bazel_common//third_party/java/junit",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,41 @@
package dagger.hilt.processor.internal.root;

import static com.google.testing.compile.CompilationSubject.assertThat;
import static dagger.hilt.android.processor.AndroidCompilers.compiler;

import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.testing.compile.Compilation;
import com.google.testing.compile.Compiler;
import com.google.testing.compile.JavaFileObjects;
import dagger.hilt.android.processor.AndroidCompilers;
import javax.tools.JavaFileObject;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

@RunWith(JUnit4.class)
@RunWith(Parameterized.class)
public final class MyAppPreviousCompilationTest {

@Parameters(name = "{0}")
public static ImmutableCollection<Object[]> parameters() {
return ImmutableList.copyOf(new Object[][] {{true}, {false}});
}

private final boolean disableCrossCompilationRootValidation;

public MyAppPreviousCompilationTest(boolean disableCrossCompilationRootValidation) {
this.disableCrossCompilationRootValidation = disableCrossCompilationRootValidation;
}

private Compiler compiler() {
return AndroidCompilers.compiler()
.withOptions(
String.format(
"-Adagger.hilt.disableCrossCompilationRootValidation=%s",
disableCrossCompilationRootValidation));
}

@Test
public void testRootTest() {
JavaFileObject testRoot =
Expand All @@ -40,6 +64,7 @@ public void testRootTest() {
"@HiltAndroidTest",
"public class TestRoot {}");

// This test case should succeed independent of disableCrossCompilationRootValidation.
Compilation compilation = compiler().compile(testRoot);
assertThat(compilation).succeeded();
}
Expand All @@ -58,14 +83,18 @@ public void appRootTest() {
"public class AppRoot extends Hilt_AppRoot {}");

Compilation compilation = compiler().compile(appRoot);
assertThat(compilation).failed();
assertThat(compilation).hadErrorCount(1);
assertThat(compilation)
.hadErrorContaining(
"Cannot process app roots in this compilation unit since there are app roots in a "
+ "previous compilation unit:"
+ "\n \tApp roots in previous compilation unit: ["
+ "dagger.hilt.processor.internal.root.MyAppPreviousCompilation.MyApp]"
+ "\n \tApp roots in this compilation unit: [test.AppRoot]");
if (disableCrossCompilationRootValidation) {
assertThat(compilation).succeeded();
} else {
assertThat(compilation).failed();
assertThat(compilation).hadErrorCount(1);
assertThat(compilation)
.hadErrorContaining(
"Cannot process app roots in this compilation unit since there are app roots in a "
+ "previous compilation unit:"
+ "\n \tApp roots in previous compilation unit: ["
+ "dagger.hilt.processor.internal.root.MyAppPreviousCompilation.MyApp]"
+ "\n \tApp roots in this compilation unit: [test.AppRoot]");
}
}
}
Loading