Ensure no service loader rewriting if it crosses a feature split
This CL ensures that we will not perform service-loader rewriting of
types, where the service type is defined in a feature, a
META-INF/services files is declared for a service in a feature or if
any of the implementations defined is in a feature.
Bug: 157426812
Change-Id: I086c569b88f48d9fc037e4d1ac5b8fb64c61a84a
diff --git a/src/main/java/com/android/tools/r8/FeatureSplit.java b/src/main/java/com/android/tools/r8/FeatureSplit.java
index 5947786..9bb637e 100644
--- a/src/main/java/com/android/tools/r8/FeatureSplit.java
+++ b/src/main/java/com/android/tools/r8/FeatureSplit.java
@@ -29,6 +29,9 @@
*/
@Keep
public final class FeatureSplit {
+
+ public static final FeatureSplit BASE = new FeatureSplit(null, null);
+
private final ProgramConsumer programConsumer;
private final List<ProgramResourceProvider> programResourceProviders;
diff --git a/src/main/java/com/android/tools/r8/graph/AppServices.java b/src/main/java/com/android/tools/r8/graph/AppServices.java
index 5c1c7ad..f010b85 100644
--- a/src/main/java/com/android/tools/r8/graph/AppServices.java
+++ b/src/main/java/com/android/tools/r8/graph/AppServices.java
@@ -8,6 +8,8 @@
import com.android.tools.r8.DataEntryResource;
import com.android.tools.r8.DataResourceProvider;
import com.android.tools.r8.DataResourceProvider.Visitor;
+import com.android.tools.r8.FeatureSplit;
+import com.android.tools.r8.ProgramResourceProvider;
import com.android.tools.r8.ResourceException;
import com.android.tools.r8.errors.CompilationError;
import com.android.tools.r8.origin.Origin;
@@ -21,7 +23,7 @@
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Collection;
-import java.util.IdentityHashMap;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
@@ -36,9 +38,9 @@
private final AppView<?> appView;
// Mapping from service types to service implementation types.
- private final Map<DexType, List<DexType>> services;
+ private final Map<DexType, Map<FeatureSplit, List<DexType>>> services;
- private AppServices(AppView<?> appView, Map<DexType, List<DexType>> services) {
+ private AppServices(AppView<?> appView, Map<DexType, Map<FeatureSplit, List<DexType>>> services) {
this.appView = appView;
this.services = services;
}
@@ -54,66 +56,124 @@
public List<DexType> serviceImplementationsFor(DexType serviceType) {
assert verifyRewrittenWithLens();
- assert services.containsKey(serviceType);
- List<DexType> serviceImplementationTypes = services.get(serviceType);
- if (serviceImplementationTypes == null) {
+ Map<FeatureSplit, List<DexType>> featureSplitListMap = services.get(serviceType);
+ if (featureSplitListMap == null) {
assert false
: "Unexpected attempt to get service implementations for non-service type `"
+ serviceType.toSourceString()
+ "`";
return ImmutableList.of();
}
- return serviceImplementationTypes;
+ ImmutableList.Builder<DexType> builder = ImmutableList.builder();
+ for (List<DexType> implementations : featureSplitListMap.values()) {
+ builder.addAll(implementations);
+ }
+ return builder.build();
+ }
+
+ public boolean hasServiceImplementationsInFeature(DexType serviceType) {
+ if (appView.options().featureSplitConfiguration == null) {
+ return false;
+ }
+ Map<FeatureSplit, List<DexType>> featureImplementations = services.get(serviceType);
+ if (featureImplementations == null || featureImplementations.isEmpty()) {
+ assert false
+ : "Unexpected attempt to get service implementations for non-service type `"
+ + serviceType.toSourceString()
+ + "`";
+ return true;
+ }
+ if (featureImplementations.size() > 1
+ || !featureImplementations.containsKey(FeatureSplit.BASE)) {
+ return true;
+ }
+ // Check if service is defined feature
+ DexProgramClass serviceClass = appView.definitionForProgramType(serviceType);
+ if (appView.options().featureSplitConfiguration.isInFeature(serviceClass)) {
+ return true;
+ }
+ for (DexType dexType : featureImplementations.get(FeatureSplit.BASE)) {
+ DexProgramClass implementationClass = appView.definitionForProgramType(dexType);
+ if (appView.options().featureSplitConfiguration.isInFeature(implementationClass)) {
+ return true;
+ }
+ }
+ return false;
}
public AppServices rewrittenWithLens(GraphLense graphLens) {
- ImmutableMap.Builder<DexType, List<DexType>> rewrittenServices = ImmutableMap.builder();
- for (Entry<DexType, List<DexType>> entry : services.entrySet()) {
+ ImmutableMap.Builder<DexType, Map<FeatureSplit, List<DexType>>> rewrittenFeatureMappings =
+ ImmutableMap.builder();
+ for (Entry<DexType, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) {
DexType rewrittenServiceType = graphLens.lookupType(entry.getKey());
- ImmutableList.Builder<DexType> rewrittenServiceImplementationTypes = ImmutableList.builder();
- for (DexType serviceImplementationType : entry.getValue()) {
- rewrittenServiceImplementationTypes.add(graphLens.lookupType(serviceImplementationType));
+ ImmutableMap.Builder<FeatureSplit, List<DexType>> rewrittenFeatureImplementations =
+ ImmutableMap.builder();
+ for (Entry<FeatureSplit, List<DexType>> featureSplitImpls : entry.getValue().entrySet()) {
+ ImmutableList.Builder<DexType> rewrittenServiceImplementationTypes =
+ ImmutableList.builder();
+ for (DexType serviceImplementationType : featureSplitImpls.getValue()) {
+ rewrittenServiceImplementationTypes.add(graphLens.lookupType(serviceImplementationType));
+ }
+ rewrittenFeatureImplementations.put(
+ featureSplitImpls.getKey(), rewrittenServiceImplementationTypes.build());
}
- rewrittenServices.put(rewrittenServiceType, rewrittenServiceImplementationTypes.build());
+ rewrittenFeatureMappings.put(rewrittenServiceType, rewrittenFeatureImplementations.build());
}
- return new AppServices(appView, rewrittenServices.build());
+ return new AppServices(appView, rewrittenFeatureMappings.build());
}
public AppServices prunedCopy(Collection<DexType> removedClasses) {
- ImmutableMap.Builder<DexType, List<DexType>> rewrittenServicesBuilder = ImmutableMap.builder();
- for (Entry<DexType, List<DexType>> entry : services.entrySet()) {
- if (!removedClasses.contains(entry.getKey())) {
- DexType serviceType = entry.getKey();
+ ImmutableMap.Builder<DexType, Map<FeatureSplit, List<DexType>>> rewrittenServicesBuilder =
+ ImmutableMap.builder();
+ for (Entry<DexType, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) {
+ if (removedClasses.contains(entry.getKey())) {
+ continue;
+ }
+ ImmutableMap.Builder<FeatureSplit, List<DexType>> prunedFeatureSplitImpls =
+ ImmutableMap.builder();
+ for (Entry<FeatureSplit, List<DexType>> featureSplitEntry : entry.getValue().entrySet()) {
ImmutableList.Builder<DexType> rewrittenServiceImplementationTypesBuilder =
ImmutableList.builder();
- for (DexType serviceImplementationType : entry.getValue()) {
+ for (DexType serviceImplementationType : featureSplitEntry.getValue()) {
if (!removedClasses.contains(serviceImplementationType)) {
rewrittenServiceImplementationTypesBuilder.add(serviceImplementationType);
}
}
- List<DexType> rewrittenServiceImplementationTypes =
+ List<DexType> prunedFeatureSplitImplementations =
rewrittenServiceImplementationTypesBuilder.build();
- if (rewrittenServiceImplementationTypes.size() > 0) {
- rewrittenServicesBuilder.put(
- serviceType, rewrittenServiceImplementationTypesBuilder.build());
+ if (prunedFeatureSplitImplementations.size() > 0) {
+ prunedFeatureSplitImpls.put(
+ featureSplitEntry.getKey(), rewrittenServiceImplementationTypesBuilder.build());
}
}
+ ImmutableMap<FeatureSplit, List<DexType>> prunedServiceImplementations =
+ prunedFeatureSplitImpls.build();
+ if (prunedServiceImplementations.size() > 0) {
+ rewrittenServicesBuilder.put(entry.getKey(), prunedServiceImplementations);
+ }
}
return new AppServices(appView, rewrittenServicesBuilder.build());
}
private boolean verifyRewrittenWithLens() {
- for (Entry<DexType, List<DexType>> entry : services.entrySet()) {
+ for (Entry<DexType, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) {
assert entry.getKey() == appView.graphLense().lookupType(entry.getKey());
- for (DexType type : entry.getValue()) {
- assert type == appView.graphLense().lookupType(type);
+ for (Entry<FeatureSplit, List<DexType>> featureEntry : entry.getValue().entrySet()) {
+ for (DexType type : featureEntry.getValue()) {
+ assert type == appView.graphLense().lookupType(type);
+ }
}
}
return true;
}
public void visit(BiConsumer<DexType, List<DexType>> consumer) {
- services.forEach(consumer);
+ services.forEach(
+ (type, featureImpls) -> {
+ ImmutableList.Builder<DexType> builder = ImmutableList.builder();
+ featureImpls.values().forEach(builder::addAll);
+ consumer.accept(type, builder.build());
+ });
}
public static Builder builder(AppView<?> appView) {
@@ -123,7 +183,7 @@
public static class Builder {
private final AppView<?> appView;
- private final Map<DexType, List<DexType>> services = new IdentityHashMap<>();
+ private final Map<DexType, Map<FeatureSplit, List<DexType>>> services = new LinkedHashMap<>();
private Builder(AppView<?> appView) {
this.appView = appView;
@@ -131,14 +191,22 @@
public AppServices build() {
for (DataResourceProvider provider : appView.appInfo().app().dataResourceProviders) {
- readServices(provider);
+ readServices(provider, FeatureSplit.BASE);
+ }
+ List<FeatureSplit> featureSplits =
+ appView.options().featureSplitConfiguration.getFeatureSplits();
+ for (FeatureSplit featureSplit : featureSplits) {
+ for (ProgramResourceProvider provider : featureSplit.getProgramResourceProviders()) {
+ readServices(provider.getDataResourceProvider(), featureSplit);
+ }
}
return new AppServices(appView, services);
}
- private void readServices(DataResourceProvider dataResourceProvider) {
+ private void readServices(
+ DataResourceProvider dataResourceProvider, FeatureSplit featureSplit) {
try {
- dataResourceProvider.accept(new DataResourceProviderVisitor());
+ dataResourceProvider.accept(new DataResourceProviderVisitor(featureSplit));
} catch (ResourceException e) {
throw new CompilationError(e.getMessage(), e);
}
@@ -146,6 +214,12 @@
private class DataResourceProviderVisitor implements Visitor {
+ private final FeatureSplit featureSplit;
+
+ public DataResourceProviderVisitor(FeatureSplit featureSplit) {
+ this.featureSplit = featureSplit;
+ }
+
@Override
public void visit(DataDirectoryResource directory) {
// Ignore.
@@ -162,8 +236,10 @@
DexType serviceType = appView.dexItemFactory().createType(serviceDescriptor);
byte[] bytes = ByteStreams.toByteArray(file.getByteStream());
String contents = new String(bytes, Charset.defaultCharset());
+ Map<FeatureSplit, List<DexType>> featureSplitImplementations =
+ services.computeIfAbsent(serviceType, k -> new LinkedHashMap<>());
List<DexType> serviceImplementations =
- services.computeIfAbsent(serviceType, (key) -> new ArrayList<>());
+ featureSplitImplementations.computeIfAbsent(featureSplit, f -> new ArrayList<>());
readServiceImplementationsForService(
contents, file.getOrigin(), serviceImplementations);
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
index 263cadd..71a2a18 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
@@ -132,6 +132,11 @@
continue;
}
+ // Check that we are not service loading anything from a feature into base.
+ if (appView.appServices().hasServiceImplementationsInFeature(constClass.getValue())) {
+ continue;
+ }
+
// Check that ClassLoader used is the ClassLoader defined for the the service configuration
// that we are instantiating or NULL.
InvokeVirtual classLoaderInvoke =
diff --git a/src/test/java/com/android/tools/r8/dexsplitter/R8FeatureSplitServiceLoaderTest.java b/src/test/java/com/android/tools/r8/dexsplitter/R8FeatureSplitServiceLoaderTest.java
index 39a1b03..c290f26 100644
--- a/src/test/java/com/android/tools/r8/dexsplitter/R8FeatureSplitServiceLoaderTest.java
+++ b/src/test/java/com/android/tools/r8/dexsplitter/R8FeatureSplitServiceLoaderTest.java
@@ -5,11 +5,14 @@
package com.android.tools.r8.dexsplitter;
import static com.android.tools.r8.rewrite.ServiceLoaderRewritingTest.getServiceLoaderLoads;
+import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.assertEquals;
import com.android.tools.r8.DataEntryResource;
+import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.ToolHelper.DexVm.Version;
import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.utils.Pair;
import com.android.tools.r8.utils.StringUtils;
@@ -36,18 +39,15 @@
}
@Test
- public void testR8AllServiceConfigurationInBase() throws Exception {
+ public void testR8AllServiceConfigurationInBaseAndNoTypesInFeatures() throws Exception {
Path base = temp.newFile("base.zip").toPath();
Path feature1Path = temp.newFile("feature1.zip").toPath();
- Path feature2Path = temp.newFile("feature2.zip").toPath();
testForR8(parameters.getBackend())
- .addProgramClasses(Base.class, I.class)
+ .addProgramClasses(Base.class, I.class, Feature1I.class, Feature2I.class)
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Base.class)
.addFeatureSplit(
- builder -> simpleSplitProvider(builder, feature1Path, temp, Feature1I.class))
- .addFeatureSplit(
- builder -> simpleSplitProvider(builder, feature2Path, temp, Feature2I.class))
+ builder -> simpleSplitProvider(builder, feature1Path, temp, Feature3Dummy.class))
.addDataEntryResources(
DataEntryResource.fromBytes(
StringUtils.lines(Feature1I.class.getTypeName(), Feature2I.class.getTypeName())
@@ -57,59 +57,99 @@
.compile()
.inspect(
inspector -> {
- // TODO(b/157426812): This should be 1.
assertEquals(0, getServiceLoaderLoads(inspector, Base.class));
})
.writeToZip(base)
- .addRunClasspathFiles(feature1Path, feature2Path)
.run(parameters.getRuntime(), Base.class)
.assertSuccessWithOutputLines("Feature1I.foo()", "Feature2I.foo()");
}
@Test
+ public void testR8AllServiceConfigurationInBase() throws Exception {
+ Path base = temp.newFile("base.zip").toPath();
+ Path feature1Path = temp.newFile("feature1.zip").toPath();
+ Path feature2Path = temp.newFile("feature2.zip").toPath();
+ R8TestRunResult runResult =
+ testForR8(parameters.getBackend())
+ .addProgramClasses(Base.class, I.class)
+ .setMinApi(parameters.getApiLevel())
+ .addKeepMainRule(Base.class)
+ .addFeatureSplit(
+ builder -> simpleSplitProvider(builder, feature1Path, temp, Feature1I.class))
+ .addFeatureSplit(
+ builder -> simpleSplitProvider(builder, feature2Path, temp, Feature2I.class))
+ .addDataEntryResources(
+ DataEntryResource.fromBytes(
+ StringUtils.lines(Feature1I.class.getTypeName(), Feature2I.class.getTypeName())
+ .getBytes(),
+ "META-INF/services/" + I.class.getTypeName(),
+ Origin.unknown()))
+ .compile()
+ .inspect(
+ inspector -> {
+ assertEquals(1, getServiceLoaderLoads(inspector, Base.class));
+ })
+ .writeToZip(base)
+ .addRunClasspathFiles(feature1Path, feature2Path)
+ .run(parameters.getRuntime(), Base.class);
+ // TODO(b/160888348): This is failing on 7.0
+ if (parameters.getRuntime().isDex()
+ && parameters.getRuntime().asDex().getVm().getVersion() == Version.V7_0_0) {
+ runResult.assertFailureWithErrorThatMatches(containsString("ServiceConfigurationError"));
+ } else {
+ runResult.assertSuccessWithOutputLines("Feature1I.foo()", "Feature2I.foo()");
+ }
+ }
+
+ @Test
public void testR8AllLoaded() throws Exception {
Path base = temp.newFile("base.zip").toPath();
Path feature1Path = temp.newFile("feature1.zip").toPath();
Path feature2Path = temp.newFile("feature2.zip").toPath();
- testForR8(parameters.getBackend())
- .addProgramClasses(Base.class, I.class)
- .setMinApi(parameters.getApiLevel())
- .addKeepMainRule(Base.class)
- .addFeatureSplit(
- builder ->
- splitWithNonJavaFile(
- builder,
- feature1Path,
- temp,
- ImmutableList.of(
- new Pair<>(
- "META-INF/services/" + I.class.getTypeName(),
- StringUtils.lines(Feature1I.class.getTypeName()))),
- true,
- Feature1I.class))
- .addFeatureSplit(
- builder ->
- splitWithNonJavaFile(
- builder,
- feature2Path,
- temp,
- ImmutableList.of(
- new Pair<>(
- "META-INF/services/" + I.class.getTypeName(),
- StringUtils.lines(Feature2I.class.getTypeName()))),
- true,
- Feature2I.class))
- .compile()
- .inspect(
- inspector -> {
- assertEquals(1, getServiceLoaderLoads(inspector, Base.class));
- })
- .writeToZip(base)
- .addRunClasspathFiles(feature1Path, feature2Path)
- .run(parameters.getRuntime(), Base.class)
- // TODO(b/157426812): Should output
- // .assertSuccessWithOutputLines("Feature1I.foo()", "Feature2I.foo()");
- .assertSuccessWithOutput("");
+ R8TestRunResult runResult =
+ testForR8(parameters.getBackend())
+ .addProgramClasses(Base.class, I.class)
+ .setMinApi(parameters.getApiLevel())
+ .addKeepMainRule(Base.class)
+ .addFeatureSplit(
+ builder ->
+ splitWithNonJavaFile(
+ builder,
+ feature1Path,
+ temp,
+ ImmutableList.of(
+ new Pair<>(
+ "META-INF/services/" + I.class.getTypeName(),
+ StringUtils.lines(Feature1I.class.getTypeName()))),
+ true,
+ Feature1I.class))
+ .addFeatureSplit(
+ builder ->
+ splitWithNonJavaFile(
+ builder,
+ feature2Path,
+ temp,
+ ImmutableList.of(
+ new Pair<>(
+ "META-INF/services/" + I.class.getTypeName(),
+ StringUtils.lines(Feature2I.class.getTypeName()))),
+ true,
+ Feature2I.class))
+ .compile()
+ .inspect(
+ inspector -> {
+ assertEquals(1, getServiceLoaderLoads(inspector, Base.class));
+ })
+ .writeToZip(base)
+ .addRunClasspathFiles(feature1Path, feature2Path)
+ .run(parameters.getRuntime(), Base.class);
+ // TODO(b/160888348): This is failing on 7.0
+ if (parameters.getRuntime().isDex()
+ && parameters.getRuntime().asDex().getVm().getVersion() == Version.V7_0_0) {
+ runResult.assertFailureWithErrorThatMatches(containsString("ServiceConfigurationError"));
+ } else {
+ runResult.assertSuccessWithOutputLines("Feature1I.foo()", "Feature2I.foo()");
+ }
}
@Test
@@ -118,42 +158,47 @@
Path feature1Path = temp.newFile("feature1.zip").toPath();
Path feature2Path = temp.newFile("feature2.zip").toPath();
Path feature3Path = temp.newFile("feature3.zip").toPath();
- testForR8(parameters.getBackend())
- .addProgramClasses(Base.class, I.class)
- .setMinApi(parameters.getApiLevel())
- .addKeepMainRule(Base.class)
- .addFeatureSplit(
- builder -> simpleSplitProvider(builder, feature1Path, temp, Feature1I.class))
- .addFeatureSplit(
- builder -> simpleSplitProvider(builder, feature2Path, temp, Feature2I.class))
- .addFeatureSplit(
- builder ->
- splitWithNonJavaFile(
- builder,
- feature3Path,
- temp,
- ImmutableList.of(
- new Pair<>(
- "META-INF/services/" + I.class.getTypeName(),
- StringUtils.lines(
- Feature1I.class.getTypeName(), Feature2I.class.getTypeName()))),
- true,
- Feature3Dummy.class))
- .compile()
- .inspect(
- inspector -> {
- assertEquals(1, getServiceLoaderLoads(inspector, Base.class));
- })
- .writeToZip(base)
- .addRunClasspathFiles(feature1Path, feature2Path, feature3Path)
- .run(parameters.getRuntime(), Base.class)
- // TODO(b/157426812): Should output
- // .assertSuccessWithOutputLines("Feature1I.foo()", "Feature2I.foo()");
- .assertSuccessWithOutput("");
+ R8TestRunResult runResult =
+ testForR8(parameters.getBackend())
+ .addProgramClasses(Base.class, I.class)
+ .setMinApi(parameters.getApiLevel())
+ .addKeepMainRule(Base.class)
+ .addFeatureSplit(
+ builder -> simpleSplitProvider(builder, feature1Path, temp, Feature1I.class))
+ .addFeatureSplit(
+ builder -> simpleSplitProvider(builder, feature2Path, temp, Feature2I.class))
+ .addFeatureSplit(
+ builder ->
+ splitWithNonJavaFile(
+ builder,
+ feature3Path,
+ temp,
+ ImmutableList.of(
+ new Pair<>(
+ "META-INF/services/" + I.class.getTypeName(),
+ StringUtils.lines(
+ Feature1I.class.getTypeName(), Feature2I.class.getTypeName()))),
+ true,
+ Feature3Dummy.class))
+ .compile()
+ .inspect(
+ inspector -> {
+ assertEquals(1, getServiceLoaderLoads(inspector, Base.class));
+ })
+ .writeToZip(base)
+ .addRunClasspathFiles(feature1Path, feature2Path, feature3Path)
+ .run(parameters.getRuntime(), Base.class);
+ // TODO(b/160888348): This is failing on 7.0
+ if (parameters.getRuntime().isDex()
+ && parameters.getRuntime().asDex().getVm().getVersion() == Version.V7_0_0) {
+ runResult.assertFailureWithErrorThatMatches(containsString("ServiceConfigurationError"));
+ } else {
+ runResult.assertSuccessWithOutputLines("Feature1I.foo()", "Feature2I.foo()");
+ }
}
@Test
- public void testR8NotFound() throws Exception {
+ public void testR8OnlyFeature2() throws Exception {
Path base = temp.newFile("base.zip").toPath();
Path feature1Path = temp.newFile("feature1.zip").toPath();
Path feature2Path = temp.newFile("feature2.zip").toPath();
@@ -193,9 +238,8 @@
.writeToZip(base)
.addRunClasspathFiles(feature2Path)
.run(parameters.getRuntime(), Base.class)
- // TODO(b/157426812): Should output
- // .assertSuccessWithOutputLines("Feature2I.foo()");
- .assertSuccessWithOutput("");
+ // TODO(b/160889305): This should work.
+ .assertFailureWithErrorThatMatches(containsString("java.lang.ClassNotFoundException"));
}
public interface I {
