Decouple system feature API codegen from feature definitions

The set of build-time, readonly features will vary for each device. To
ensure we at least have a minimal static API surface to target, allow
specifying a minimal feature API set alongside any explicitly defined
feature values.

This will let us start migrating a subset of internal hasSystemFeature
API queries to use the new API, independent of integration with the
build system and per-device build flag settings.

Initially, this will just be a passthrough API surface with no build or
runtime behavioral differences. As we integrate per-device build flags,
the (sub)set of explicitly defined features will change how each API
behaves.

Bug: 203143243
Test: atest systemfeatures-gen-tests --host
Change-Id: I3c84375bdc1a2e31cc72fa09f3b980c2daa85bb2
diff --git a/tools/systemfeatures/Android.bp b/tools/systemfeatures/Android.bp
index 2cebfe9..aca25eb 100644
--- a/tools/systemfeatures/Android.bp
+++ b/tools/systemfeatures/Android.bp
@@ -30,9 +30,9 @@
 genrule {
     name: "systemfeatures-gen-tests-srcs",
     cmd: "$(location systemfeatures-gen-tool) com.android.systemfeatures.RwNoFeatures --readonly=false > $(location RwNoFeatures.java) && " +
-        "$(location systemfeatures-gen-tool) com.android.systemfeatures.RoNoFeatures --readonly=true > $(location RoNoFeatures.java) && " +
+        "$(location systemfeatures-gen-tool) com.android.systemfeatures.RoNoFeatures --readonly=true --feature-apis=WATCH > $(location RoNoFeatures.java) && " +
         "$(location systemfeatures-gen-tool) com.android.systemfeatures.RwFeatures --readonly=false --feature=WATCH:1 --feature=WIFI:0 --feature=VULKAN:-1 --feature=AUTO: > $(location RwFeatures.java) && " +
-        "$(location systemfeatures-gen-tool) com.android.systemfeatures.RoFeatures --readonly=true --feature=WATCH:1 --feature=WIFI:0 --feature=VULKAN:-1 --feature=AUTO: > $(location RoFeatures.java)",
+        "$(location systemfeatures-gen-tool) com.android.systemfeatures.RoFeatures --readonly=true --feature=WATCH:1 --feature=WIFI:0 --feature=VULKAN:-1 --feature=AUTO: --feature-apis=WATCH,PC > $(location RoFeatures.java)",
     out: [
         "RwNoFeatures.java",
         "RoNoFeatures.java",
diff --git a/tools/systemfeatures/src/com/android/systemfeatures/SystemFeaturesGenerator.kt b/tools/systemfeatures/src/com/android/systemfeatures/SystemFeaturesGenerator.kt
index 9bfda45..e537ffc 100644
--- a/tools/systemfeatures/src/com/android/systemfeatures/SystemFeaturesGenerator.kt
+++ b/tools/systemfeatures/src/com/android/systemfeatures/SystemFeaturesGenerator.kt
@@ -32,6 +32,7 @@
  * <pre>
  *   <cmd> com.foo.RoSystemFeatures --readonly=true \
  *           --feature=WATCH:0 --feature=AUTOMOTIVE: --feature=VULKAN:9348
+ *           --feature-apis=WATCH,PC,LEANBACK
  * </pre>
  *
  * This generates a class that has the following signature:
@@ -45,12 +46,15 @@
  *     public static boolean hasFeatureAutomotive(Context context);
  *     @AssumeTrueForR8
  *     public static boolean hasFeatureVulkan(Context context);
+ *     public static boolean hasFeaturePc(Context context);
+ *     public static boolean hasFeatureLeanback(Context context);
  *     public static Boolean maybeHasFeature(String feature, int version);
  * }
  * </pre>
  */
 object SystemFeaturesGenerator {
     private const val FEATURE_ARG = "--feature="
+    private const val FEATURE_APIS_ARG = "--feature-apis="
     private const val READONLY_ARG = "--readonly="
     private val PACKAGEMANAGER_CLASS = ClassName.get("android.content.pm", "PackageManager")
     private val CONTEXT_CLASS = ClassName.get("android.content", "Context")
@@ -64,6 +68,15 @@
         println(" Options:")
         println("  --readonly=true|false    Whether to encode features as build-time constants")
         println("  --feature=\$NAME:\$VER   A feature+version pair (blank version == disabled)")
+        println("                           This will always generate associated query APIs,")
+        println("                           adding to or replacing those from `--feature-apis=`.")
+        println("  --feature-apis=\$NAME_1,\$NAME_2")
+        println("                           A comma-separated set of features for which to always")
+        println("                           generate named query APIs. If a feature in this set is")
+        println("                           not explicitly defined via `--feature=`, then a simple")
+        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.")
     }
 
     /** Main entrypoint for build-time system feature codegen. */
@@ -76,18 +89,42 @@
 
         var readonly = false
         var outputClassName: ClassName? = null
-        val features = mutableListOf<FeatureInfo>()
+        val featureArgs = mutableListOf<FeatureArg>()
+        // We could just as easily hardcode this list, as the static API surface should change
+        // somewhat infrequently, but this decouples the codegen from the framework completely.
+        val featureApiArgs = mutableSetOf<String>()
         for (arg in args) {
             when {
                 arg.startsWith(READONLY_ARG) ->
                     readonly = arg.substring(READONLY_ARG.length).toBoolean()
                 arg.startsWith(FEATURE_ARG) -> {
-                    features.add(parseFeatureArg(arg))
+                    featureArgs.add(parseFeatureArg(arg))
+                }
+                arg.startsWith(FEATURE_APIS_ARG) -> {
+                    featureApiArgs.addAll(
+                        arg.substring(FEATURE_APIS_ARG.length).split(",").map {
+                            parseFeatureName(it)
+                        }
+                    )
                 }
                 else -> outputClassName = ClassName.bestGuess(arg)
             }
         }
 
+        // First load in all of the feature APIs we want to generate. Explicit feature definitions
+        // will then override this set with the appropriate readonly and version value.
+        val features = mutableMapOf<String, FeatureInfo>()
+        featureApiArgs.associateByTo(
+            features,
+            { it },
+            { FeatureInfo(it, version = null, readonly = false) },
+        )
+        featureArgs.associateByTo(
+            features,
+            { it.name },
+            { FeatureInfo(it.name, it.version, readonly) },
+        )
+
         outputClassName
             ?: run {
                 println("Output class name must be provided.")
@@ -100,8 +137,8 @@
                 .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
                 .addJavadoc("@hide")
 
-        addFeatureMethodsToClass(classBuilder, readonly, features)
-        addMaybeFeatureMethodToClass(classBuilder, readonly, features)
+        addFeatureMethodsToClass(classBuilder, features.values)
+        addMaybeFeatureMethodToClass(classBuilder, features.values)
 
         // TODO(b/203143243): Add validation of build vs runtime values to ensure consistency.
         JavaFile.builder(outputClassName.packageName(), classBuilder.build())
@@ -115,13 +152,16 @@
      *   * "--feature=WATCH:7" -> Feature enabled w/ version 7
      *   * "--feature=WATCH:"  -> Feature disabled
      */
-    private fun parseFeatureArg(arg: String): FeatureInfo {
+    private fun parseFeatureArg(arg: String): FeatureArg {
         val featureArgs = arg.substring(FEATURE_ARG.length).split(":")
-        val name = featureArgs[0].let { if (!it.startsWith("FEATURE_")) "FEATURE_$it" else it }
+        val name = parseFeatureName(featureArgs[0])
         val version = featureArgs.getOrNull(1)?.toIntOrNull()
-        return FeatureInfo(name, version)
+        return FeatureArg(name, version)
     }
 
+    private fun parseFeatureName(name: String): String =
+        if (name.startsWith("FEATURE_")) name else "FEATURE_$name"
+
     /*
      * Adds per-feature query methods to the class with the form:
      * {@code public static boolean hasFeatureX(Context context)},
@@ -129,8 +169,7 @@
      */
     private fun addFeatureMethodsToClass(
         builder: TypeSpec.Builder,
-        readonly: Boolean,
-        features: List<FeatureInfo>
+        features: Collection<FeatureInfo>,
     ) {
         for (feature in features) {
             // Turn "FEATURE_FOO" into "hasFeatureFoo".
@@ -142,7 +181,7 @@
                     .returns(Boolean::class.java)
                     .addParameter(CONTEXT_CLASS, "context")
 
-            if (readonly) {
+            if (feature.readonly) {
                 val featureEnabled = compareValues(feature.version, 0) >= 0
                 methodBuilder.addAnnotation(
                     if (featureEnabled) ASSUME_TRUE_CLASS else ASSUME_FALSE_CLASS
@@ -158,19 +197,17 @@
             builder.addMethod(methodBuilder.build())
         }
 
-        if (!readonly) {
-            builder.addMethod(
-                MethodSpec.methodBuilder("hasFeatureFallback")
-                    .addModifiers(Modifier.PRIVATE, Modifier.STATIC)
-                    .returns(Boolean::class.java)
-                    .addParameter(CONTEXT_CLASS, "context")
-                    .addParameter(String::class.java, "featureName")
-                    .addStatement(
-                        "return context.getPackageManager().hasSystemFeature(featureName, 0)"
-                    )
-                    .build()
-            )
-        }
+        // This is a trivial method, even if unused based on readonly-codegen, it does little harm
+        // to always include it.
+        builder.addMethod(
+            MethodSpec.methodBuilder("hasFeatureFallback")
+                .addModifiers(Modifier.PRIVATE, Modifier.STATIC)
+                .returns(Boolean::class.java)
+                .addParameter(CONTEXT_CLASS, "context")
+                .addParameter(String::class.java, "featureName")
+                .addStatement("return context.getPackageManager().hasSystemFeature(featureName, 0)")
+                .build()
+        )
     }
 
     /*
@@ -185,8 +222,7 @@
      */
     private fun addMaybeFeatureMethodToClass(
         builder: TypeSpec.Builder,
-        readonly: Boolean,
-        features: List<FeatureInfo>
+        features: Collection<FeatureInfo>,
     ) {
         val methodBuilder =
             MethodSpec.methodBuilder("maybeHasFeature")
@@ -196,16 +232,27 @@
                 .addParameter(String::class.java, "featureName")
                 .addParameter(Int::class.java, "version")
 
-        if (readonly) {
-            methodBuilder.beginControlFlow("switch (featureName)")
-            for (feature in features) {
-                methodBuilder.addCode("case \$T.\$N: ", PACKAGEMANAGER_CLASS, feature.name)
-                if (feature.version != null) {
-                    methodBuilder.addStatement("return \$L >= version", feature.version)
-                } else {
-                    methodBuilder.addStatement("return false")
-                }
+        var hasSwitchBlock = false
+        for (feature in features) {
+            // We only return non-null results for queries against readonly-defined features.
+            if (!feature.readonly) {
+                continue
             }
+            if (!hasSwitchBlock) {
+                // As an optimization, only create the switch block if needed. Even an empty
+                // switch-on-string block can induce a hash, which we can avoid if readonly
+                // support is completely disabled.
+                hasSwitchBlock = true
+                methodBuilder.beginControlFlow("switch (featureName)")
+            }
+            methodBuilder.addCode("case \$T.\$N: ", PACKAGEMANAGER_CLASS, feature.name)
+            if (feature.version != null) {
+                methodBuilder.addStatement("return \$L >= version", feature.version)
+            } else {
+                methodBuilder.addStatement("return false")
+            }
+        }
+        if (hasSwitchBlock) {
             methodBuilder.addCode("default: ")
             methodBuilder.addStatement("break")
             methodBuilder.endControlFlow()
@@ -214,5 +261,7 @@
         builder.addMethod(methodBuilder.build())
     }
 
-    private data class FeatureInfo(val name: String, val version: Int?)
+    private data class FeatureArg(val name: String, val version: Int?)
+
+    private data class FeatureInfo(val name: String, val version: Int?, val readonly: Boolean)
 }
diff --git a/tools/systemfeatures/tests/PackageManager.java b/tools/systemfeatures/tests/PackageManager.java
index 645d500..db67048 100644
--- a/tools/systemfeatures/tests/PackageManager.java
+++ b/tools/systemfeatures/tests/PackageManager.java
@@ -19,6 +19,7 @@
 /** Stub for testing */
 public class PackageManager {
     public static final String FEATURE_AUTO = "automotive";
+    public static final String FEATURE_PC = "pc";
     public static final String FEATURE_VULKAN = "vulkan";
     public static final String FEATURE_WATCH = "watch";
     public static final String FEATURE_WIFI = "wifi";
diff --git a/tools/systemfeatures/tests/SystemFeaturesGeneratorTest.java b/tools/systemfeatures/tests/SystemFeaturesGeneratorTest.java
index 547d2cb..6dfd244 100644
--- a/tools/systemfeatures/tests/SystemFeaturesGeneratorTest.java
+++ b/tools/systemfeatures/tests/SystemFeaturesGeneratorTest.java
@@ -68,6 +68,13 @@
         assertThat(RoNoFeatures.maybeHasFeature(PackageManager.FEATURE_VULKAN, 0)).isNull();
         assertThat(RoNoFeatures.maybeHasFeature(PackageManager.FEATURE_AUTO, 0)).isNull();
         assertThat(RoNoFeatures.maybeHasFeature("com.arbitrary.feature", 0)).isNull();
+
+        // Also ensure we fall back to the PackageManager for feature APIs without an accompanying
+        // versioned feature definition.
+        when(mPackageManager.hasSystemFeature(PackageManager.FEATURE_WATCH, 0)).thenReturn(true);
+        assertThat(RwFeatures.hasFeatureWatch(mContext)).isTrue();
+        when(mPackageManager.hasSystemFeature(PackageManager.FEATURE_WATCH, 0)).thenReturn(false);
+        assertThat(RwFeatures.hasFeatureWatch(mContext)).isFalse();
     }
 
     @Test
@@ -127,6 +134,16 @@
         assertThat(RoFeatures.maybeHasFeature(PackageManager.FEATURE_AUTO, 0)).isFalse();
         assertThat(RoFeatures.maybeHasFeature(PackageManager.FEATURE_AUTO, 100)).isFalse();
 
+        // For feature APIs without an associated feature definition, conditional queries should
+        // report null, and explicit queries should report runtime-defined versions.
+        when(mPackageManager.hasSystemFeature(PackageManager.FEATURE_PC, 0)).thenReturn(true);
+        assertThat(RoFeatures.hasFeaturePc(mContext)).isTrue();
+        when(mPackageManager.hasSystemFeature(PackageManager.FEATURE_PC, 0)).thenReturn(false);
+        assertThat(RoFeatures.hasFeaturePc(mContext)).isFalse();
+        assertThat(RoFeatures.maybeHasFeature(PackageManager.FEATURE_PC, -1)).isNull();
+        assertThat(RoFeatures.maybeHasFeature(PackageManager.FEATURE_PC, 0)).isNull();
+        assertThat(RoFeatures.maybeHasFeature(PackageManager.FEATURE_PC, 100)).isNull();
+
         // For undefined types, conditional queries should report null (unknown).
         assertThat(RoFeatures.maybeHasFeature("com.arbitrary.feature", -1)).isNull();
         assertThat(RoFeatures.maybeHasFeature("com.arbitrary.feature", 0)).isNull();