Merge "Create ErrorProne lint rule for RoSystemFeatures usage" into main (7983ce95) · Commits · e / os / android_frameworks_base

tools/systemfeatures/Android.bp

+69 −0

Original line number	Diff line number	Diff line
		@@ -100,3 +100,72 @@ sh_test_host {
		unit_test: true,
		},
		}

		java_library_host {
		name: "systemfeatures-errorprone-lib",
		srcs: [
		":systemfeatures-gen-metadata-srcs",
		"errorprone/java/*/.java",
		],
		static_libs: [
		"//external/error_prone:error_prone_core",
		"guava",
		"jsr305",
		],
		libs: [
		"//external/auto:auto_service_annotations",
		],
		javacflags: [
		// These exports are needed because this errorprone plugin access some private classes
		// of the java compiler.
		"--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
		"--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
		"--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
		],
		plugins: [
		"//external/auto:auto_service_plugin",
		],
		}

		java_plugin {
		name: "systemfeatures-errorprone",
		static_libs: ["systemfeatures-errorprone-lib"],
		}

		java_test_host {
		name: "systemfeatures-errorprone-tests",
		srcs: [
		"errorprone/tests/java/*/.java",
		],
		java_resource_dirs: ["tests/src"],
		java_resources: [
		":systemfeatures-errorprone-tests-data",
		],
		static_libs: [
		"compile-testing-prebuilt",
		"error_prone_test_helpers",
		"framework-annotations-lib",
		"hamcrest",
		"hamcrest-library",
		"junit",
		"systemfeatures-errorprone-lib",
		"truth",
		],
		test_options: {
		unit_test: true,
		},
		}

		java_system_features_srcs {
		name: "systemfeatures-gen-metadata-srcs",
		full_class_name: "com.android.systemfeatures.RoSystemFeaturesMetadata",
		metadata_only: true,
		visibility: ["//visibility:private"],
		}

		filegroup {
		name: "systemfeatures-errorprone-tests-data",
		path: "tests/src",
		srcs: ["tests/src/android/*/.java"],
		visibility: ["//visibility:private"],
		}

tools/systemfeatures/README.md

+105 −3

Original line number	Diff line number	Diff line
		@@ -4,8 +4,110 @@

		System features exposed from `PackageManager` are defined and aggregated as
		`<feature>` xml attributes across various partitions, and are currently queried
		at runtime through the framework. This directory contains tooling that will
		support build-time queries of select system features, enabling optimizations
		at runtime through the framework. This directory contains tooling that supports
		build-time queries of select system features, enabling optimizations
		like code stripping and conditionally dependencies when so configured.

		### TODO(b/203143243): Expand readme after landing codegen.
		### System Feature Codegen

		As not all system features can be fully specified or defined at build time (e.g.
		updatable partitisions and apex modules can change/remove such features), we
		use a conditional, build flag approach that allows a given device to customize
		the subset of build-time defined system features that are immutable and cannot
		be updated.

		#### Build Flags

		System features that can be fixed at build-time are declared in a common
		location, `build/release/flag_declarations/`. These have the form
		`RELEASE_SYSTEM_FEATURE_${X}`, where `${X}` corresponds to a feature defined in
		`PackageManager`, e.g., `TELEVISION` or `WATCH`.

		Build flag values can then be defined per device (or form factor), where such
		values either indicate the existence/version of the system feature, or that the
		feature is unavailable, e.g., for TV, we could define these build flag values:
		```
		name: "RELEASE_SYSTEM_FEATURE_TELEVISION"
		value: {
		string_value: "0" # Feature version = 0
		}
		```
		```
		name: "RELEASE_SYSTEM_FEATURE_WATCH"
		value: {
		string_value: "UNAVAILABLE"
		}
		```

		See also [SystemFeaturesGenerator](src/com/android/systemfeatures/SystemFeaturesGenerator.kt)
		for more details.

		#### Runtime Queries

		Each declared build flag system feature is routed into codegen, generating a
		getter API in the internal class, `com.android.internal.pm.RoSystemFeatures`:
		```
		class RoSystemFeatures {
		...
		public static boolean hasFeatureX(Context context);
		...
		}
		```
		By default, these queries simply fall back to the usual
		`PackageManager.hasSystemFeature(...)` runtime queries. However, if a device
		defines these features via build flags, the generated code will add annotations
		indicating fixed value for this query, and adjust the generated code to return
		the value directly. This in turn enables build-time stripping and optimization.

		> _NOTE:_ Any build-time defined system features will also be implicitly
		used to accelerate calls to `PackageManager.hasSystemFeature(...)` for the
		feature, avoiding binder calls when possible.

		#### Lint

		A new `ErrorProne` rule is introduced to assist with migration and maintenance
		of codegen APIs for build-time defined system features. This is defined in the
		`systemfeatures-errorprone` build rule, which can be added to any Java target's
		`plugins` list.

		// TODO(b/203143243): Add plugin to key system targets after initial migration.

		1) Add the plugin dependency to a given `${TARGET}`:
		```
		java_library {
		name: "${TARGET}",
		plugins: ["systemfeatures-errorprone"],
		}
		```
		2) Run locally:
		```
		RUN_ERROR_PRONE=true m ${TARGET}
		```
		3) (Optional) Update the target rule to generate in-place patch files:
		```
		java_library {
		name: "${TARGET}",
		plugins: ["systemfeatures-errorprone"],
		// DO NOT SUBMIT: GENERATE IN-PLACE PATCH FILES
		errorprone: {
		javacflags: [
		"-XepPatchChecks:RoSystemFeaturesChecker",
		"-XepPatchLocation:IN_PLACE",
		],
		}
		...
		}
		```
		```
		RUN_ERROR_PRONE=true m ${TARGET}
		```

		See also [RoSystemFeaturesChecker](errorprone/java/com/android/systemfeatures/errorprone/RoSystemFeaturesChecker.java)
		for more details.

		> _NOTE:_ Not all system feature queries or targets need or should be
		migrated. Only system features that are explicitly declared with build flags,
		and only targets that are built with the platform (i.e., not updatable), are
		candidates for this linting and migration, e.g., SystemUI, System Server, etc...

		// TODO(b/203143243): Wrap the in-place lint updates with a simple script for convenience.

tools/systemfeatures/errorprone/java/com/android/systemfeatures/errorprone/RoSystemFeaturesChecker.java

0 → 100644

+119 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright (C) 2024 The Android Open Source Project
		*
		* Licensed under the Apache License, Version 2.0 (the "License");
		* you may not use this file except in compliance with the License.
		* You may obtain a copy of the License at
		*
		* http://www.apache.org/licenses/LICENSE-2.0
		*
		* Unless required by applicable law or agreed to in writing, software
		* distributed under the License is distributed on an "AS IS" BASIS,
		* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
		* See the License for the specific language governing permissions and
		* limitations under the License.
		*/

		package com.android.systemfeatures.errorprone;

		import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;

		import com.android.systemfeatures.RoSystemFeaturesMetadata;

		import com.google.auto.service.AutoService;
		import com.google.errorprone.BugPattern;
		import com.google.errorprone.VisitorState;
		import com.google.errorprone.bugpatterns.BugChecker;
		import com.google.errorprone.fixes.SuggestedFix;
		import com.google.errorprone.matchers.Description;
		import com.google.errorprone.matchers.Matcher;
		import com.google.errorprone.matchers.Matchers;
		import com.google.errorprone.util.ASTHelpers;
		import com.sun.source.tree.ExpressionTree;
		import com.sun.source.tree.MethodInvocationTree;
		import com.sun.tools.javac.code.Symbol;

		@AutoService(BugChecker.class)
		@BugPattern(
		name = "RoSystemFeaturesChecker",
		summary = "Use RoSystemFeature instead of PackageManager.hasSystemFeature",
		explanation =
		"Directly invoking `PackageManager.hasSystemFeature` is less efficient than using"
		+ " the `RoSystemFeatures` helper class. This check flags invocations like"
		+ " `context.getPackageManager().hasSystemFeature(PackageManager.FEATURE_FOO)`"
		+ " and suggests replacing them with"
		+ " `com.android.internal.pm.RoSystemFeatures.hasFeatureFoo(context)`.",
		severity = WARNING)
		public class RoSystemFeaturesChecker extends BugChecker
		implements BugChecker.MethodInvocationTreeMatcher {

		private static final String PACKAGE_MANAGER_CLASS = "android.content.pm.PackageManager";
		private static final String CONTEXT_CLASS = "android.content.Context";
		private static final String RO_SYSTEM_FEATURE_SIMPLE_CLASS = "RoSystemFeatures";
		private static final String RO_SYSTEM_FEATURE_CLASS =
		"com.android.internal.pm." + RO_SYSTEM_FEATURE_SIMPLE_CLASS;
		private static final String GET_PACKAGE_MANAGER_METHOD = "getPackageManager";
		private static final String HAS_SYSTEM_FEATURE_METHOD = "hasSystemFeature";
		private static final String FEATURE_PREFIX = "FEATURE_";

		private static final Matcher<ExpressionTree> HAS_SYSTEM_FEATURE_MATCHER =
		Matchers.instanceMethod()
		.onDescendantOf(PACKAGE_MANAGER_CLASS)
		.named(HAS_SYSTEM_FEATURE_METHOD)
		.withParameters(String.class.getName());

		private static final Matcher<ExpressionTree> GET_PACKAGE_MANAGER_MATCHER =
		Matchers.instanceMethod()
		.onDescendantOf(CONTEXT_CLASS)
		.named(GET_PACKAGE_MANAGER_METHOD);

		@Override
		public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
		if (!HAS_SYSTEM_FEATURE_MATCHER.matches(tree, state)) {
		return Description.NO_MATCH;
		}

		// Check if the PackageManager was obtained from a Context instance.
		ExpressionTree packageManager = ASTHelpers.getReceiver(tree);
		if (!GET_PACKAGE_MANAGER_MATCHER.matches(packageManager, state)) {
		return Description.NO_MATCH;
		}

		// Get the feature argument and check if it's a PackageManager.FEATURE_X constant.
		ExpressionTree feature = tree.getArguments().isEmpty() ? null : tree.getArguments().get(0);
		Symbol featureSymbol = ASTHelpers.getSymbol(feature);
		if (featureSymbol == null
		\|\| !featureSymbol.isStatic()
		\|\| !featureSymbol.getSimpleName().toString().startsWith(FEATURE_PREFIX)
		\|\| ASTHelpers.enclosingClass(featureSymbol) == null
		\|\| !ASTHelpers.enclosingClass(featureSymbol)
		.getQualifiedName()
		.contentEquals(PACKAGE_MANAGER_CLASS)) {
		return Description.NO_MATCH;
		}

		// Check if the feature argument is part of the RoSystemFeatures API surface.
		String featureName = featureSymbol.getSimpleName().toString();
		String methodName = RoSystemFeaturesMetadata.getMethodNameForFeatureName(featureName);
		if (methodName == null) {
		return Description.NO_MATCH;
		}

		// Generate the appropriate fix.
		String replacement =
		String.format(
		"%s.%s(%s)",
		RO_SYSTEM_FEATURE_SIMPLE_CLASS,
		methodName,
		state.getSourceForNode(ASTHelpers.getReceiver(packageManager)));
		// Note that ErrorProne doesn't offer a seamless way of removing the `PackageManager` import
		// if unused after fix application, so for now we only offer best effort import suggestions.
		SuggestedFix fix =
		SuggestedFix.builder()
		.replace(tree, replacement)
		.addImport(RO_SYSTEM_FEATURE_CLASS)
		.removeStaticImport(PACKAGE_MANAGER_CLASS + "." + featureName)
		.build();
		return describeMatch(tree, fix);
		}
		}

tools/systemfeatures/errorprone/tests/java/com/android/systemfeatures/errorprone/RoSystemFeaturesCheckerTest.java

0 → 100644

+123 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright (C) 2020 The Android Open Source Project
		*
		* Licensed under the Apache License, Version 2.0 (the "License");
		* you may not use this file except in compliance with the License.
		* You may obtain a copy of the License at
		*
		* http://www.apache.org/licenses/LICENSE-2.0
		*
		* Unless required by applicable law or agreed to in writing, software
		* distributed under the License is distributed on an "AS IS" BASIS,
		* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
		* See the License for the specific language governing permissions and
		* limitations under the License.
		*/

		package com.android.systemfeatures.errorprone;

		import com.google.errorprone.BugCheckerRefactoringTestHelper;
		import com.google.errorprone.CompilationTestHelper;

		import org.junit.Before;
		import org.junit.Test;
		import org.junit.runner.RunWith;
		import org.junit.runners.JUnit4;

		@RunWith(JUnit4.class)
		public class RoSystemFeaturesCheckerTest {
		private BugCheckerRefactoringTestHelper mRefactoringHelper;
		private CompilationTestHelper mCompilationHelper;

		@Before
		public void setUp() {
		mCompilationHelper =
		CompilationTestHelper.newInstance(RoSystemFeaturesChecker.class, getClass());
		mRefactoringHelper =
		BugCheckerRefactoringTestHelper.newInstance(
		RoSystemFeaturesChecker.class, getClass());
		}

		@Test
		public void testNoDiagnostic() {
		mCompilationHelper
		.addSourceFile("/android/content/Context.java")
		.addSourceFile("/android/content/pm/PackageManager.java")
		.addSourceLines("Example.java",
		"""
		import android.content.Context;
		import android.content.pm.PackageManager;
		public class Example {
		void test(Context context) {
		boolean hasCustomFeature = context.getPackageManager()
		.hasSystemFeature("my.custom.feature");
		boolean hasNonAnnotatedFeature = context.getPackageManager()
		.hasSystemFeature(PackageManager.FEATURE_NOT_ANNOTATED);
		boolean hasNonRoApiFeature = context.getPackageManager()
		.hasSystemFeature(PackageManager.FEATURE_NOT_IN_RO_FEATURE_API);
		}
		}
		""")
		.doTest();
		}

		@Test
		public void testDiagnostic() {
		mCompilationHelper
		.addSourceFile("/android/content/Context.java")
		.addSourceFile("/android/content/pm/PackageManager.java")
		.addSourceLines("Example.java",
		"""
		import android.content.Context;
		import android.content.pm.PackageManager;
		public class Example {
		void test(Context context) {
		boolean hasFeature = context.getPackageManager()
		// BUG: Diagnostic contains:
		.hasSystemFeature(PackageManager.FEATURE_PC);
		}
		}
		""")
		.doTest();
		}

		@Test
		public void testFix() {
		mRefactoringHelper
		.addInputLines("Example.java",
		"""
		import static android.content.pm.PackageManager.FEATURE_WATCH;

		import android.content.Context;
		import android.content.pm.PackageManager;
		public class Example {
		static class CustomContext extends Context {};
		private CustomContext mContext;
		void test(Context context) {
		boolean hasPc = mContext.getPackageManager()
		.hasSystemFeature(PackageManager.FEATURE_PC);
		boolean hasWatch = context.getPackageManager()
		.hasSystemFeature(FEATURE_WATCH);
		}
		}
		""")
		.addOutputLines("Example.java",
		"""
		import android.content.Context;
		import android.content.pm.PackageManager;
		import com.android.internal.pm.RoSystemFeatures;
		public class Example {
		static class CustomContext extends Context {};
		private CustomContext mContext;
		void test(Context context) {
		boolean hasPc = RoSystemFeatures.hasFeaturePc(mContext);
		boolean hasWatch = RoSystemFeatures.hasFeatureWatch(context);
		}
		}
		""")
		// Don't try compiling the output, as it requires pulling in the full set of code
		// dependencies.
		.allowBreakingChanges()
		.doTest();
		}
		}

tools/systemfeatures/src/com/android/systemfeatures/SystemFeaturesGenerator.kt

+50 −8

Original line number	Diff line number	Diff line
		@@ -53,11 +53,20 @@ import javax.lang.model.element.Modifier
		* public static ArrayMap<String, FeatureInfo> getReadOnlySystemEnabledFeatures();
		* }
		* </pre>
		*
		* <p> If `--metadata-only=true` is set, the resulting class would simply be:
		* <pre>
		* package com.foo;
		* public final class RoSystemFeatures {
		* public static String getMethodNameForFeatureName(String featureName);
		* }
		* </pre>
		*/
		object SystemFeaturesGenerator {
		private const val FEATURE_ARG = "--feature="
		private const val FEATURE_APIS_ARG = "--feature-apis="
		private const val READONLY_ARG = "--readonly="
		private const val METADATA_ONLY_ARG = "--metadata-only="
		private val PACKAGEMANAGER_CLASS = ClassName.get("android.content.pm", "PackageManager")
		private val CONTEXT_CLASS = ClassName.get("android.content", "Context")
		private val FEATUREINFO_CLASS = ClassName.get("android.content.pm", "FeatureInfo")
		@@ -84,6 +93,8 @@ object SystemFeaturesGenerator {
		println(" runtime passthrough API will be generated, regardless")
		println(" of the `--readonly` flag. This allows decoupling the")
		println(" API surface from variations in device feature sets.")
		println(" --metadata-only=true\|false Whether to simply output metadata about the")
		println(" generated API surface.")
		}

		/** Main entrypoint for build-time system feature codegen. */
		@@ -106,6 +117,7 @@ object SystemFeaturesGenerator {
		}

		var readonly = false
		var metadataOnly = false
		var outputClassName: ClassName? = null
		val featureArgs = mutableListOf<FeatureInfo>()
		// We could just as easily hardcode this list, as the static API surface should change
		@@ -115,6 +127,8 @@ object SystemFeaturesGenerator {
		when {
		arg.startsWith(READONLY_ARG) ->
		readonly = arg.substring(READONLY_ARG.length).toBoolean()
		arg.startsWith(METADATA_ONLY_ARG) ->
		metadataOnly = arg.substring(METADATA_ONLY_ARG.length).toBoolean()
		arg.startsWith(FEATURE_ARG) -> {
		featureArgs.add(parseFeatureArg(arg))
		}
		@@ -155,9 +169,13 @@ object SystemFeaturesGenerator {
		.addModifiers(Modifier.PUBLIC, Modifier.FINAL)
		.addJavadoc("@hide")

		if (metadataOnly) {
		addMetadataMethodToClass(classBuilder, features.values)
		} else {
		addFeatureMethodsToClass(classBuilder, features.values)
		addMaybeFeatureMethodToClass(classBuilder, features.values)
		addGetFeaturesMethodToClass(classBuilder, features.values)
		}

		// TODO(b/203143243): Add validation of build vs runtime values to ensure consistency.
		JavaFile.builder(outputClassName.packageName(), classBuilder.build())
		@@ -214,11 +232,8 @@ object SystemFeaturesGenerator {
		features: Collection<FeatureInfo>,
		) {
		for (feature in features) {
		// Turn "FEATURE_FOO" into "hasFeatureFoo".
		val methodName =
		"has" + CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.UPPER_CAMEL, feature.name)
		val methodBuilder =
		MethodSpec.methodBuilder(methodName)
		MethodSpec.methodBuilder(feature.methodName)
		.addModifiers(Modifier.PUBLIC, Modifier.STATIC)
		.addJavadoc("Check for ${feature.name}.\n\n@hide")
		.returns(Boolean::class.java)
		@@ -341,5 +356,32 @@ object SystemFeaturesGenerator {
		builder.addMethod(methodBuilder.build())
		}

		private data class FeatureInfo(val name: String, val version: Int?, val readonly: Boolean)
		/*
		* Adds a metadata helper method that maps FEATURE_FOO names to their generated hasFeatureFoo()
		* API counterpart, if defined.
		*/
		private fun addMetadataMethodToClass(
		builder: TypeSpec.Builder,
		features: Collection<FeatureInfo>,
		) {
		val methodBuilder =
		MethodSpec.methodBuilder("getMethodNameForFeatureName")
		.addModifiers(Modifier.PUBLIC, Modifier.STATIC)
		.addJavadoc("@return \"hasFeatureFoo\" if FEATURE_FOO is in the API, else null")
		.returns(String::class.java)
		.addParameter(String::class.java, "featureVarName")

		methodBuilder.beginControlFlow("switch (featureVarName)")
		for (feature in features) {
		methodBuilder.addStatement("case \$S: return \$S", feature.name, feature.methodName)
		}
		methodBuilder.addStatement("default: return null").endControlFlow()

		builder.addMethod(methodBuilder.build())
		}

		private data class FeatureInfo(val name: String, val version: Int?, val readonly: Boolean) {
		// Turn "FEATURE_FOO" into "hasFeatureFoo".
		val methodName get() = "has" + CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.UPPER_CAMEL, name)
		}
		}