Revert "Ensure no service loader rewriting if it crosses a feature split"
This reverts commit 8e258a371de31374fd3da8f79291dfb49f6df6cf.
Reason for revert: NPE
Change-Id: I3810807e815e3e59eaaeb3e870fccdf2207bd096
diff --git a/src/main/java/com/android/tools/r8/FeatureSplit.java b/src/main/java/com/android/tools/r8/FeatureSplit.java
index 9bb637e..5947786 100644
--- a/src/main/java/com/android/tools/r8/FeatureSplit.java
+++ b/src/main/java/com/android/tools/r8/FeatureSplit.java
@@ -29,9 +29,6 @@
*/
@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 f010b85..5c1c7ad 100644
--- a/src/main/java/com/android/tools/r8/graph/AppServices.java
+++ b/src/main/java/com/android/tools/r8/graph/AppServices.java
@@ -8,8 +8,6 @@
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;
@@ -23,7 +21,7 @@
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Collection;
-import java.util.LinkedHashMap;
+import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
@@ -38,9 +36,9 @@
private final AppView<?> appView;
// Mapping from service types to service implementation types.
- private final Map<DexType, Map<FeatureSplit, List<DexType>>> services;
+ private final Map<DexType, List<DexType>> services;
- private AppServices(AppView<?> appView, Map<DexType, Map<FeatureSplit, List<DexType>>> services) {
+ private AppServices(AppView<?> appView, Map<DexType, List<DexType>> services) {
this.appView = appView;
this.services = services;
}
@@ -56,124 +54,66 @@
public List<DexType> serviceImplementationsFor(DexType serviceType) {
assert verifyRewrittenWithLens();
- Map<FeatureSplit, List<DexType>> featureSplitListMap = services.get(serviceType);
- if (featureSplitListMap == null) {
+ assert services.containsKey(serviceType);
+ List<DexType> serviceImplementationTypes = services.get(serviceType);
+ if (serviceImplementationTypes == null) {
assert false
: "Unexpected attempt to get service implementations for non-service type `"
+ serviceType.toSourceString()
+ "`";
return ImmutableList.of();
}
- 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;
+ return serviceImplementationTypes;
}
public AppServices rewrittenWithLens(GraphLense graphLens) {
- ImmutableMap.Builder<DexType, Map<FeatureSplit, List<DexType>>> rewrittenFeatureMappings =
- ImmutableMap.builder();
- for (Entry<DexType, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) {
+ ImmutableMap.Builder<DexType, List<DexType>> rewrittenServices = ImmutableMap.builder();
+ for (Entry<DexType, List<DexType>> entry : services.entrySet()) {
DexType rewrittenServiceType = graphLens.lookupType(entry.getKey());
- 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());
+ ImmutableList.Builder<DexType> rewrittenServiceImplementationTypes = ImmutableList.builder();
+ for (DexType serviceImplementationType : entry.getValue()) {
+ rewrittenServiceImplementationTypes.add(graphLens.lookupType(serviceImplementationType));
}
- rewrittenFeatureMappings.put(rewrittenServiceType, rewrittenFeatureImplementations.build());
+ rewrittenServices.put(rewrittenServiceType, rewrittenServiceImplementationTypes.build());
}
- return new AppServices(appView, rewrittenFeatureMappings.build());
+ return new AppServices(appView, rewrittenServices.build());
}
public AppServices prunedCopy(Collection<DexType> removedClasses) {
- 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()) {
+ 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();
ImmutableList.Builder<DexType> rewrittenServiceImplementationTypesBuilder =
ImmutableList.builder();
- for (DexType serviceImplementationType : featureSplitEntry.getValue()) {
+ for (DexType serviceImplementationType : entry.getValue()) {
if (!removedClasses.contains(serviceImplementationType)) {
rewrittenServiceImplementationTypesBuilder.add(serviceImplementationType);
}
}
- List<DexType> prunedFeatureSplitImplementations =
+ List<DexType> rewrittenServiceImplementationTypes =
rewrittenServiceImplementationTypesBuilder.build();
- if (prunedFeatureSplitImplementations.size() > 0) {
- prunedFeatureSplitImpls.put(
- featureSplitEntry.getKey(), rewrittenServiceImplementationTypesBuilder.build());
+ if (rewrittenServiceImplementationTypes.size() > 0) {
+ rewrittenServicesBuilder.put(
+ serviceType, 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, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) {
+ for (Entry<DexType, List<DexType>> entry : services.entrySet()) {
assert entry.getKey() == appView.graphLense().lookupType(entry.getKey());
- for (Entry<FeatureSplit, List<DexType>> featureEntry : entry.getValue().entrySet()) {
- for (DexType type : featureEntry.getValue()) {
- assert type == appView.graphLense().lookupType(type);
- }
+ for (DexType type : entry.getValue()) {
+ assert type == appView.graphLense().lookupType(type);
}
}
return true;
}
public void visit(BiConsumer<DexType, List<DexType>> consumer) {
- services.forEach(
- (type, featureImpls) -> {
- ImmutableList.Builder<DexType> builder = ImmutableList.builder();
- featureImpls.values().forEach(builder::addAll);
- consumer.accept(type, builder.build());
- });
+ services.forEach(consumer);
}
public static Builder builder(AppView<?> appView) {
@@ -183,7 +123,7 @@
public static class Builder {
private final AppView<?> appView;
- private final Map<DexType, Map<FeatureSplit, List<DexType>>> services = new LinkedHashMap<>();
+ private final Map<DexType, List<DexType>> services = new IdentityHashMap<>();
private Builder(AppView<?> appView) {
this.appView = appView;
@@ -191,22 +131,14 @@
public AppServices build() {
for (DataResourceProvider provider : appView.appInfo().app().dataResourceProviders) {
- readServices(provider, FeatureSplit.BASE);
- }
- List<FeatureSplit> featureSplits =
- appView.options().featureSplitConfiguration.getFeatureSplits();
- for (FeatureSplit featureSplit : featureSplits) {
- for (ProgramResourceProvider provider : featureSplit.getProgramResourceProviders()) {
- readServices(provider.getDataResourceProvider(), featureSplit);
- }
+ readServices(provider);
}
return new AppServices(appView, services);
}
- private void readServices(
- DataResourceProvider dataResourceProvider, FeatureSplit featureSplit) {
+ private void readServices(DataResourceProvider dataResourceProvider) {
try {
- dataResourceProvider.accept(new DataResourceProviderVisitor(featureSplit));
+ dataResourceProvider.accept(new DataResourceProviderVisitor());
} catch (ResourceException e) {
throw new CompilationError(e.getMessage(), e);
}
@@ -214,12 +146,6 @@
private class DataResourceProviderVisitor implements Visitor {
- private final FeatureSplit featureSplit;
-
- public DataResourceProviderVisitor(FeatureSplit featureSplit) {
- this.featureSplit = featureSplit;
- }
-
@Override
public void visit(DataDirectoryResource directory) {
// Ignore.
@@ -236,10 +162,8 @@
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 =
- featureSplitImplementations.computeIfAbsent(featureSplit, f -> new ArrayList<>());
+ services.computeIfAbsent(serviceType, (key) -> 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 71a2a18..263cadd 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,11 +132,6 @@
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 c290f26..39a1b03 100644
--- a/src/test/java/com/android/tools/r8/dexsplitter/R8FeatureSplitServiceLoaderTest.java
+++ b/src/test/java/com/android/tools/r8/dexsplitter/R8FeatureSplitServiceLoaderTest.java
@@ -5,14 +5,11 @@
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;
@@ -39,15 +36,18 @@
}
@Test
- public void testR8AllServiceConfigurationInBaseAndNoTypesInFeatures() throws Exception {
+ 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();
testForR8(parameters.getBackend())
- .addProgramClasses(Base.class, I.class, Feature1I.class, Feature2I.class)
+ .addProgramClasses(Base.class, I.class)
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Base.class)
.addFeatureSplit(
- builder -> simpleSplitProvider(builder, feature1Path, temp, Feature3Dummy.class))
+ 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())
@@ -57,99 +57,59 @@
.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();
- 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()");
- }
+ 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("");
}
@Test
@@ -158,47 +118,42 @@
Path feature1Path = temp.newFile("feature1.zip").toPath();
Path feature2Path = temp.newFile("feature2.zip").toPath();
Path feature3Path = temp.newFile("feature3.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))
- .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()");
- }
+ 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("");
}
@Test
- public void testR8OnlyFeature2() throws Exception {
+ public void testR8NotFound() throws Exception {
Path base = temp.newFile("base.zip").toPath();
Path feature1Path = temp.newFile("feature1.zip").toPath();
Path feature2Path = temp.newFile("feature2.zip").toPath();
@@ -238,8 +193,9 @@
.writeToZip(base)
.addRunClasspathFiles(feature2Path)
.run(parameters.getRuntime(), Base.class)
- // TODO(b/160889305): This should work.
- .assertFailureWithErrorThatMatches(containsString("java.lang.ClassNotFoundException"));
+ // TODO(b/157426812): Should output
+ // .assertSuccessWithOutputLines("Feature2I.foo()");
+ .assertSuccessWithOutput("");
}
public interface I {
