From 40488e9b0723d58f5e2235bae07ebc33acb61d8a Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Thu, 26 Dec 2024 12:47:20 -0800 Subject: [PATCH] Fix LazyClassKey proguard file issues. * Fixes the name of the proguard file to contain an underscore between the package and simple names (e.g. `test.FooModule` was `testFooModule_LazyClassKeys.pro` and is now `test_FooModule_LazyClassKeys.pro`). * Fixes incremental processing by adding the originating element to the `writeResource` call (https://github.com/google/dagger/issues/4549). * Fixes bug where `StringBuilder` was declared outside module for-loop, which could lead to duplicate entries across proguard rules for different modules. * Fixed `ClassName#toString()` usage to `ClassName#canonicalName()`, since `toString()` is ambiguous and will silently break things when we migrate to `XClassName`. * Added test coverage for the proguard file name and contents. Fixes #4549 RELNOTES=Fixes #4549: Fixes incremental processing for LazyClassKey proguard files by adding the originating element to the `writeResource` call. PiperOrigin-RevId: 709861817 --- .../LazyClassKeyProcessingStep.java | 67 ++++-- ...ssKeyMapBindingComponentProcessorTest.java | 215 ++++++++++++++++++ 2 files changed, 258 insertions(+), 24 deletions(-) diff --git a/java/dagger/internal/codegen/processingstep/LazyClassKeyProcessingStep.java b/java/dagger/internal/codegen/processingstep/LazyClassKeyProcessingStep.java index fa57dc3e944..1843c190d30 100644 --- a/java/dagger/internal/codegen/processingstep/LazyClassKeyProcessingStep.java +++ b/java/dagger/internal/codegen/processingstep/LazyClassKeyProcessingStep.java @@ -18,6 +18,7 @@ import static androidx.room.compiler.processing.XElementKt.isTypeElement; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.stream.Collectors.joining; import androidx.room.compiler.processing.XElement; import androidx.room.compiler.processing.XFiler; @@ -38,7 +39,6 @@ import java.io.OutputStream; import java.io.OutputStreamWriter; import java.nio.file.Path; -import java.util.Collection; import java.util.Map; import java.util.Set; import javax.inject.Inject; @@ -46,7 +46,10 @@ /** Generate keep rules for LazyClassKey referenced classes to prevent class merging. */ final class LazyClassKeyProcessingStep extends TypeCheckingProcessingStep { private static final String PROGUARD_KEEP_RULE = "-keep,allowobfuscation,allowshrinking class "; - private final SetMultimap processedElements = LinkedHashMultimap.create(); + + // Note: We aggregate @LazyClassKey usages across processing rounds, so we use ClassName instead + // of XElement as the map key to avoid storing XElement instances across processing rounds. + private final SetMultimap lazyMapKeysByModule = LinkedHashMultimap.create(); private final LazyMapKeyProxyGenerator lazyMapKeyProxyGenerator; @Inject @@ -73,7 +76,7 @@ protected void process(XElement element, ImmutableSet annotations) { return; } XTypeElement moduleElement = XElements.asTypeElement(element.getEnclosingElement()); - processedElements.put(moduleElement.getClassName(), lazyClassKey); + lazyMapKeysByModule.put(moduleElement.getClassName(), lazyClassKey); XMethodElement method = XElements.asMethod(element); lazyMapKeyProxyGenerator.generate(method); } @@ -91,30 +94,45 @@ private static boolean isModuleOrProducerModule(XElement element) { || element.hasAnnotation(TypeNames.PRODUCER_MODULE)); } + // TODO(b/386393062): Avoid generating proguard files in processOver. @Override public void processOver( XProcessingEnv env, Map> elementsByAnnotation) { super.processOver(env, elementsByAnnotation); - StringBuilder proguardRules = new StringBuilder(); - for (Map.Entry> moduleToLazyClassKeys : - processedElements.asMap().entrySet()) { - String bindingGraphProguardName = - getFullyQualifiedEnclosedClassName(moduleToLazyClassKeys.getKey()) + "_LazyClassKeys.pro"; - for (ClassName lazyClassKey : moduleToLazyClassKeys.getValue()) { - proguardRules.append(PROGUARD_KEEP_RULE).append(lazyClassKey).append("\n"); - } - writeProguardFile(bindingGraphProguardName, proguardRules.toString(), env.getFiler()); - } + lazyMapKeysByModule + .asMap() + .forEach( + (moduleClassName, lazyClassKeys) -> { + // Note: we could probably get better incremental performance by using the method + // element instead of the module element as the originating element. However, that + // would require appending the method name to each proguard file, which would probably + // cause issues with the filename length limit (256 characters) given it already must + // include the module's fully qualified name. + XTypeElement originatingElement = + env.requireTypeElement(moduleClassName.canonicalName()); + + Path proguardFile = + Path.of( + "META-INF/proguard", + getFullyQualifiedEnclosedClassName(moduleClassName) + "_LazyClassKeys.pro"); + + String proguardFileContents = + lazyClassKeys.stream() + .map(lazyClassKey -> PROGUARD_KEEP_RULE + lazyClassKey.canonicalName()) + .collect(joining("\n")); + + writeResource(env.getFiler(), originatingElement, proguardFile, proguardFileContents); + }); + // Processing is over so this shouldn't matter, but clear the map just incase. + lazyMapKeysByModule.clear(); } - private void writeProguardFile(String proguardFileName, String proguardRules, XFiler filer) { + private void writeResource( + XFiler filer, XElement originatingElement, Path path, String contents) { try (OutputStream outputStream = - filer.writeResource( - Path.of("META-INF/proguard/" + proguardFileName), - ImmutableList.of(), - XFiler.Mode.Isolating); + filer.writeResource(path, ImmutableList.of(originatingElement), XFiler.Mode.Isolating); BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(outputStream, UTF_8))) { - writer.write(proguardRules); + writer.write(contents); } catch (IOException e) { throw new IllegalStateException(e); } @@ -122,10 +140,11 @@ private void writeProguardFile(String proguardFileName, String proguardRules, XF /** Returns the fully qualified class name, with _ instead of . */ private static String getFullyQualifiedEnclosedClassName(ClassName className) { - return className.packageName().replace('.', '_') + getEnclosedName(className); - } - - public static String getEnclosedName(ClassName name) { - return Joiner.on('_').join(name.simpleNames()); + return Joiner.on('_') + .join( + ImmutableList.builder() + .add(className.packageName().replace('.', '_')) + .addAll(className.simpleNames()) + .build()); } } diff --git a/javatests/dagger/internal/codegen/LazyClassKeyMapBindingComponentProcessorTest.java b/javatests/dagger/internal/codegen/LazyClassKeyMapBindingComponentProcessorTest.java index 6cdeb3f395b..444040abe44 100644 --- a/javatests/dagger/internal/codegen/LazyClassKeyMapBindingComponentProcessorTest.java +++ b/javatests/dagger/internal/codegen/LazyClassKeyMapBindingComponentProcessorTest.java @@ -16,13 +16,19 @@ package dagger.internal.codegen; +import static com.google.common.truth.Truth.assertThat; import static com.google.testing.compile.CompilationSubject.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; import androidx.room.compiler.processing.util.Source; +import com.google.common.truth.PrimitiveByteArraySubject; +import com.google.common.truth.StringSubject; +import com.google.common.truth.Subject; import com.google.testing.compile.Compilation; import com.google.testing.compile.JavaFileObjects; import dagger.testing.compile.CompilerTests; import dagger.testing.golden.GoldenFileRule; +import java.lang.reflect.Method; import java.util.Collection; import javax.tools.JavaFileObject; import org.junit.Rule; @@ -276,4 +282,213 @@ public void scopedLazyClassKeyProvider_compilesSuccessfully() throws Exception { .withProcessingOptions(compilerMode.processorOptions()) .compile(subject -> subject.hasErrorCount(0)); } + + @Test + public void testProguardFile() throws Exception { + Source fooKey = + CompilerTests.javaSource( + "test.FooKey", + "package test;", + "", + "interface FooKey {}"); + Source fooKeyModule = + CompilerTests.javaSource( + "test.FooKeyModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.LazyClassKey;", + "import dagger.multibindings.IntoMap;", + "", + "@Module", + "public interface FooKeyModule {", + " @Provides", + " @IntoMap", + " @LazyClassKey(FooKey.class)", + " static String provideString() { return \"\"; }", + "}"); + CompilerTests.daggerCompiler(fooKey, fooKeyModule) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(0); + PrimitiveByteArraySubject proguardFile = + subject.generatedResourceFileWithPath( + "META-INF/proguard/test_FooKeyModule_LazyClassKeys.pro"); + assertThatContentAsUtf8String(proguardFile) + .isEqualTo("-keep,allowobfuscation,allowshrinking class test.FooKey"); + }); + } + + @Test + public void testProguardFile_nestedModule() throws Exception { + Source fooKey = + CompilerTests.javaSource( + "test.FooKey", + "package test;", + "", + "interface FooKey {}"); + Source outerClass = + CompilerTests.javaSource( + "test.OuterClass", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.LazyClassKey;", + "import dagger.multibindings.IntoMap;", + "", + "public interface OuterClass {", + " @Module", + " public interface FooKeyModule {", + " @Provides", + " @IntoMap", + " @LazyClassKey(FooKey.class)", + " static String provideString() { return \"\"; }", + " }", + "}"); + CompilerTests.daggerCompiler(fooKey, outerClass) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(0); + PrimitiveByteArraySubject proguardFile = + subject.generatedResourceFileWithPath( + "META-INF/proguard/test_OuterClass_FooKeyModule_LazyClassKeys.pro"); + assertThatContentAsUtf8String(proguardFile) + .isEqualTo("-keep,allowobfuscation,allowshrinking class test.FooKey"); + }); + } + + @Test + public void testProguardFile_multipleModules() throws Exception { + Source fooKey = + CompilerTests.javaSource( + "test.FooKey", + "package test;", + "", + "interface FooKey {}"); + Source barKey = + CompilerTests.javaSource( + "test.BarKey", + "package test;", + "", + "interface BarKey {}"); + Source fooKeyModule = + CompilerTests.javaSource( + "test.FooKeyModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.LazyClassKey;", + "import dagger.multibindings.IntoMap;", + "", + "@Module", + "public interface FooKeyModule {", + " @Provides", + " @IntoMap", + " @LazyClassKey(FooKey.class)", + " static String provideString() { return \"\"; }", + "}"); + Source barKeyModule = + CompilerTests.javaSource( + "test.BarKeyModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.LazyClassKey;", + "import dagger.multibindings.IntoMap;", + "", + "@Module", + "public interface BarKeyModule {", + " @Provides", + " @IntoMap", + " @LazyClassKey(BarKey.class)", + " static String provideString() { return \"\"; }", + "}"); + CompilerTests.daggerCompiler(fooKey, fooKeyModule, barKey, barKeyModule) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(0); + PrimitiveByteArraySubject fooKeyModuleProguardFile = + subject.generatedResourceFileWithPath( + "META-INF/proguard/test_FooKeyModule_LazyClassKeys.pro"); + assertThatContentAsUtf8String(fooKeyModuleProguardFile) + .isEqualTo("-keep,allowobfuscation,allowshrinking class test.FooKey"); + + PrimitiveByteArraySubject barKeyModuleProguardFile = + subject.generatedResourceFileWithPath( + "META-INF/proguard/test_BarKeyModule_LazyClassKeys.pro"); + assertThatContentAsUtf8String(barKeyModuleProguardFile) + .isEqualTo("-keep,allowobfuscation,allowshrinking class test.BarKey"); + }); + } + + @Test + public void testProguardFile_multipleKeys() throws Exception { + Source fooKey = + CompilerTests.javaSource( + "test.FooKey", + "package test;", + "", + "interface FooKey {}"); + Source barKey = + CompilerTests.javaSource( + "test.BarKey", + "package test;", + "", + "interface BarKey {}"); + Source fooKeyAndBarKeyModule = + CompilerTests.javaSource( + "test.FooKeyAndBarKeyModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.LazyClassKey;", + "import dagger.multibindings.IntoMap;", + "", + "@Module", + "public interface FooKeyAndBarKeyModule {", + " @Provides", + " @IntoMap", + " @LazyClassKey(FooKey.class)", + " static String provideFooKeyString() { return \"\"; }", + "", + " @Provides", + " @IntoMap", + " @LazyClassKey(BarKey.class)", + " static String provideBarKeyString() { return \"\"; }", + "}"); + CompilerTests.daggerCompiler(fooKey, barKey, fooKeyAndBarKeyModule) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(0); + PrimitiveByteArraySubject proguardFile = + subject.generatedResourceFileWithPath( + "META-INF/proguard/test_FooKeyAndBarKeyModule_LazyClassKeys.pro"); + assertThatContentAsUtf8String(proguardFile) + .isEqualTo( + "-keep,allowobfuscation,allowshrinking class test.FooKey\n" + + "-keep,allowobfuscation,allowshrinking class test.BarKey"); + }); + } + + // TODO(b/386213524): Add support for getting a resource file as a StringSubject. + // Use reflection to get the subject's byte array and then convert it to a StringSubject. + private static StringSubject assertThatContentAsUtf8String(PrimitiveByteArraySubject subject) { + try { + Method protectedActualMethod = Subject.class.getDeclaredMethod("actual"); + protectedActualMethod.setAccessible(true); + byte[] actualBytes = (byte[]) protectedActualMethod.invoke(subject); + return assertThat(new String(actualBytes, UTF_8)); + } catch (Exception e) { + throw new RuntimeException(e); + } + } }