Optimize ServiceLoader.load() calls when no services are present
And adds checks for wrong subtype, non-public constructor, and missing
constructor.
The optimization is enabled only when the following rule is present:
-assumenosideeffects class java.util.ServiceLoader {
*** load(...);
}
Bug: b/291923475
Change-Id: I3a3409a8b833f1c088e6c0ca7a52efb4b63e9bb5diff --git a/src/main/java/com/android/tools/r8/graph/AccessFlags.java b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
index d52e9ae..4151107 100644
--- a/src/main/java/com/android/tools/r8/graph/AccessFlags.java
+++ b/src/main/java/com/android/tools/r8/graph/AccessFlags.java
@@ -158,6 +158,10 @@
return isSet(Constants.ACC_PUBLIC);
}
+ public boolean wasPublic() {
+ return wasSet(Constants.ACC_PUBLIC);
+ }
+
public void setPublic() {
assert !isPrivate() && !isProtected();
set(Constants.ACC_PUBLIC);
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 ea50b5a..da91743 100644
--- a/src/main/java/com/android/tools/r8/graph/AppServices.java
+++ b/src/main/java/com/android/tools/r8/graph/AppServices.java
@@ -76,10 +76,6 @@
assert verifyRewrittenWithLens();
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();
}
ImmutableList.Builder<DexType> builder = ImmutableList.builder();
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 2ca09f2..8cc1360 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
@@ -129,10 +129,11 @@
}
ConstClass constClass = argument.getDefinition().asConstClass();
+ DexType serviceType = constClass.getType();
if (invokedMethod.isNotIdenticalTo(serviceLoaderMethods.loadWithClassLoader)) {
report(
code.context(),
- constClass.getType(),
+ serviceType,
"Inlining is only supported for `java.util.ServiceLoader.load(java.lang.Class,"
+ " java.lang.ClassLoader)`");
continue;
@@ -143,7 +144,7 @@
+ " java.lang.ServiceLoader.iterator()`";
Value serviceLoaderLoadOut = serviceLoaderLoad.outValue();
if (!serviceLoaderLoadOut.hasSingleUniqueUser() || serviceLoaderLoadOut.hasPhiUsers()) {
- report(code.context(), constClass.getType(), invalidUserMessage);
+ report(code.context(), serviceType, invalidUserMessage);
continue;
}
@@ -151,14 +152,13 @@
InvokeVirtual singleUniqueUser = serviceLoaderLoadOut.singleUniqueUser().asInvokeVirtual();
if (singleUniqueUser == null
|| singleUniqueUser.getInvokedMethod().isNotIdenticalTo(serviceLoaderMethods.iterator)) {
- report(
- code.context(), constClass.getType(), invalidUserMessage + ", but found other usages");
+ report(code.context(), serviceType, invalidUserMessage + ", but found other usages");
continue;
}
// Check that the service is not kept.
if (appView().appInfo().isPinnedWithDefinitionLookup(constClass.getValue())) {
- report(code.context(), constClass.getType(), "The service loader type is kept");
+ report(code.context(), serviceType, "The service loader type is kept");
continue;
}
@@ -168,9 +168,9 @@
if (classLoaderValue.isPhi()) {
report(
code.context(),
- constClass.getType(),
+ serviceType,
"The java.lang.ClassLoader argument must be defined locally as null or "
- + constClass.getType()
+ + serviceType
+ ".class.getClassLoader()");
continue;
}
@@ -186,45 +186,62 @@
.getDefinition()
.asConstClass()
.getValue()
- .isIdenticalTo(constClass.getType());
+ .isIdenticalTo(serviceType);
if (!isNullClassLoader && !isGetClassLoaderOnConstClass) {
report(
code.context(),
- constClass.getType(),
+ serviceType,
"The java.lang.ClassLoader argument must be defined locally as null or "
- + constClass.getType()
+ + serviceType
+ ".class.getClassLoader()");
continue;
}
// Check that the service is configured in the META-INF/services.
AppServices appServices = appView.appServices();
- if (!appServices.allServiceTypes().contains(constClass.getValue())) {
- report(code.context(), constClass.getType(), "No META-INF/services file found.");
- continue;
- }
-
- // Check that we are not service loading anything from a feature into base.
- if (hasServiceImplementationInDifferentFeature(
- code, constClass.getType(), isNullClassLoader)) {
- continue;
- }
-
List<DexType> dexTypes = appServices.serviceImplementationsFor(constClass.getValue());
List<DexClass> classes = new ArrayList<>(dexTypes.size());
- boolean seenNull = false;
for (DexType serviceImpl : dexTypes) {
DexClass serviceImplementation = appView.definitionFor(serviceImpl);
if (serviceImplementation == null) {
report(
code.context(),
- constClass.getType(),
+ serviceType,
"Unable to find definition for service implementation " + serviceImpl.getTypeName());
- seenNull = true;
+ break;
+ }
+ if (!appView().appInfo().isSubtype(serviceImpl, serviceType)) {
+ report(
+ code.context(),
+ serviceType,
+ "Implementation is not a subtype of the service: " + serviceImpl.getTypeName());
+ break;
+ }
+ DexEncodedMethod method = serviceImplementation.getDefaultInitializer();
+ if (method == null) {
+ report(
+ code.context(),
+ serviceType,
+ "Implementation has no default constructor: " + serviceImpl.getTypeName());
+ break;
+ }
+ if (!method.getAccessFlags().wasPublic()) {
+ // A non-public constructor causes a ServiceConfigurationError on APIs 24 & 25 (Nougat).
+ // Check the original access flag to not change app behavior if R8 publicized the method.
+ report(
+ code.context(),
+ serviceType,
+ "Implementation's default constructor is not public: " + serviceImpl.getTypeName());
+ break;
}
classes.add(serviceImplementation);
}
- if (seenNull) {
+ if (dexTypes.size() != classes.size()) {
+ continue;
+ }
+
+ // Check that we are not service loading anything from a feature into base.
+ if (hasServiceImplementationInDifferentFeature(code, serviceType, isNullClassLoader)) {
continue;
}
diff --git a/src/test/java/com/android/tools/r8/optimize/serviceloader/ServiceLoaderRewritingTest.java b/src/test/java/com/android/tools/r8/optimize/serviceloader/ServiceLoaderRewritingTest.java
index 09025e9..939e90b5 100644
--- a/src/test/java/com/android/tools/r8/optimize/serviceloader/ServiceLoaderRewritingTest.java
+++ b/src/test/java/com/android/tools/r8/optimize/serviceloader/ServiceLoaderRewritingTest.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.optimize.serviceloader;
+import static com.android.tools.r8.ToolHelper.DexVm.Version.V7_0_0;
import static org.junit.Assert.assertEquals;
import static org.junit.Assume.assumeTrue;
@@ -11,11 +12,11 @@
import com.android.tools.r8.NeverInline;
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.utils.StringUtils;
import java.io.IOException;
import java.util.List;
import java.util.NoSuchElementException;
+import java.util.ServiceConfigurationError;
import java.util.ServiceLoader;
import java.util.concurrent.ExecutionException;
import org.junit.Test;
@@ -49,6 +50,14 @@
}
}
+ public static class ServiceImplNoDefaultConstructor extends ServiceImpl {
+ public ServiceImplNoDefaultConstructor(int unused) {}
+ }
+
+ public static class ServiceImplNonPublicConstructor extends ServiceImpl {
+ ServiceImplNonPublicConstructor() {}
+ }
+
public static class MainRunner {
public static void main(String[] args) {
@@ -153,11 +162,10 @@
throws IOException, CompilationFailedException, ExecutionException {
serviceLoaderTest(null)
.addKeepMainRule(MainRunner.class)
- .allowDiagnosticInfoMessages()
- .compileWithExpectedDiagnostics(REWRITER_DIAGNOSTICS)
+ .compile()
.run(parameters.getRuntime(), MainRunner.class)
.assertFailureWithErrorThatThrows(NoSuchElementException.class)
- .inspectFailure(inspector -> assertEquals(1, getServiceLoaderLoads(inspector)));
+ .inspectFailure(inspector -> assertEquals(0, getServiceLoaderLoads(inspector)));
}
@Test
@@ -221,6 +229,44 @@
}
@Test
+ public void testDoNoRewriteNoDefaultConstructor()
+ throws IOException, CompilationFailedException, ExecutionException {
+ serviceLoaderTest(Service.class, ServiceImplNoDefaultConstructor.class)
+ .addKeepMainRule(MainRunner.class)
+ .allowDiagnosticInfoMessages()
+ .compileWithExpectedDiagnostics(REWRITER_DIAGNOSTICS)
+ .run(parameters.getRuntime(), MainRunner.class)
+ .assertFailureWithErrorThatThrows(ServiceConfigurationError.class);
+ }
+
+ @Test
+ public void testDoNoRewriteNonSubclass()
+ throws IOException, CompilationFailedException, ExecutionException {
+ serviceLoaderTest(Service.class, MainRunner.class)
+ .addKeepMainRule(MainRunner.class)
+ .allowDiagnosticInfoMessages()
+ .compileWithExpectedDiagnostics(REWRITER_DIAGNOSTICS)
+ .run(parameters.getRuntime(), MainRunner.class)
+ .assertFailureWithErrorThatThrows(ServiceConfigurationError.class);
+ }
+
+ @Test
+ public void testDoNoRewriteNonPublicConstructor()
+ throws IOException, CompilationFailedException, ExecutionException {
+ // This throws a ServiceConfigurationError only on Android 7.
+ serviceLoaderTest(Service.class, ServiceImplNonPublicConstructor.class)
+ .addKeepMainRule(MainRunner.class)
+ .allowDiagnosticInfoMessages()
+ .compileWithExpectedDiagnostics(REWRITER_DIAGNOSTICS)
+ .run(parameters.getRuntime(), MainRunner.class)
+ .applyIf(
+ parameters.isCfRuntime() || parameters.getDexRuntimeVersion() != V7_0_0,
+ runResult -> runResult.assertSuccessWithOutput(EXPECTED_OUTPUT),
+ runResult ->
+ runResult.assertFailureWithErrorThatThrows(ServiceConfigurationError.class));
+ }
+
+ @Test
public void testDoNoRewriteWhenEscaping()
throws IOException, CompilationFailedException, ExecutionException {
serviceLoaderTest(Service.class, ServiceImpl.class)
@@ -262,7 +308,7 @@
// https://android-review.googlesource.com/c/platform/libcore/+/273135
assumeTrue(
parameters.getRuntime().isCf()
- || !parameters.getRuntime().asDex().getVm().getVersion().equals(Version.V7_0_0));
+ || !parameters.getRuntime().asDex().getVm().getVersion().equals(V7_0_0));
serviceLoaderTest(Service.class, ServiceImpl.class)
.addKeepMainRule(MainRunner.class)
.addKeepClassRules(Service.class)
