Don't add API annotations in the internal R.java

I'm trying to enable a check for the following structure:
```
/** @hide */
public class Class1 {
    /** @hide */
    @SystemApi // Invalid because the class is hidden.
    public void method1() { }
}
```

The internal R.java file violates this, which this change is going to fix.

Bug: 159162473
Bug: 163419414
Test: build (treehugger)
Test: atest aapt2_tests

Exempt-From-Owner-Approval: Cherry-pick from goog/master
Merged-In: I613e8611ddaf5f8e4761d351d4cd0142d59c7cc9
(cherry picked from commit de6e6f2098)
Change-Id: Ia63f4e1026bc7f3df2e6bc7bbec02ee88a925e54

tools/aapt2/cmd/Link.cpp

@@ -1272,7 +1272,8 @@ class Linker {
       return false;
     }
 
-    ClassDefinition::WriteJavaFile(manifest_class.get(), package_utf8, true, &fout);
+    ClassDefinition::WriteJavaFile(manifest_class.get(), package_utf8, true,
+                                   false /* strip_api_annotations */, &fout);
     fout.Flush();
 
     if (fout.HadError()) {

tools/aapt2/java/AnnotationProcessor.cpp

@@ -123,7 +123,7 @@ void AnnotationProcessor::AppendNewLine() {
   }
 }
 
-void AnnotationProcessor::Print(Printer* printer) const {
+void AnnotationProcessor::Print(Printer* printer, bool strip_api_annotations) const {
   if (has_comments_) {
     std::string result = comment_.str();
     for (const StringPiece& line : util::Tokenize(result, '\n')) {
@@ -137,6 +137,9 @@ void AnnotationProcessor::Print(Printer* printer) const {
     printer->Println("@Deprecated");
   }
 
+  if (strip_api_annotations) {
+    return;
+  }
   for (const AnnotationRule& rule : sAnnotationRules) {
     const auto& it = annotation_parameter_map_.find(rule.bit_mask);
     if (it != annotation_parameter_map_.end()) {

tools/aapt2/java/AnnotationProcessor.h

@@ -65,7 +65,7 @@ class AnnotationProcessor {
   void AppendNewLine();
 
   // Writes the comments and annotations to the Printer.
-  void Print(text::Printer* printer) const;
+  void Print(text::Printer* printer, bool strip_api_annotations = false) const;
 
  private:
   std::stringstream comment_;

tools/aapt2/java/AnnotationProcessor_test.cpp

@@ -91,6 +91,21 @@ TEST(AnnotationProcessorTest, EmitsTestApiAnnotationAndRemovesFromComment) {
   EXPECT_THAT(annotations, HasSubstr("This is a test API"));
 }
 
+TEST(AnnotationProcessorTest, NotEmitSystemApiAnnotation) {
+  AnnotationProcessor processor;
+  processor.AppendComment("@SystemApi This is a system API");
+
+  std::string annotations;
+  StringOutputStream out(&annotations);
+  Printer printer(&out);
+  processor.Print(&printer, true /* strip_api_annotations */);
+  out.Flush();
+
+  EXPECT_THAT(annotations, Not(HasSubstr("@android.annotation.SystemApi")));
+  EXPECT_THAT(annotations, Not(HasSubstr("@SystemApi")));
+  EXPECT_THAT(annotations, HasSubstr("This is a system API"));
+}
+
 TEST(AnnotationProcessor, ExtractsFirstSentence) {
   EXPECT_THAT(AnnotationProcessor::ExtractFirstSentence("This is the only sentence"),
               Eq("This is the only sentence"));

tools/aapt2/java/ClassDefinition.cpp

@@ -23,15 +23,15 @@ using ::android::StringPiece;
 
 namespace aapt {
 
-void ClassMember::Print(bool /*final*/, Printer* printer) const {
-  processor_.Print(printer);
+void ClassMember::Print(bool /*final*/, Printer* printer, bool strip_api_annotations) const {
+  processor_.Print(printer, strip_api_annotations);
 }
 
 void MethodDefinition::AppendStatement(const StringPiece& statement) {
   statements_.push_back(statement.to_string());
 }
 
-void MethodDefinition::Print(bool final, Printer* printer) const {
+void MethodDefinition::Print(bool final, Printer* printer, bool) const {
   printer->Print(signature_).Println(" {");
   printer->Indent();
   for (const auto& statement : statements_) {
@@ -74,12 +74,12 @@ bool ClassDefinition::empty() const {
   return true;
 }
 
-void ClassDefinition::Print(bool final, Printer* printer) const {
+void ClassDefinition::Print(bool final, Printer* printer, bool strip_api_annotations) const {
   if (empty() && !create_if_empty_) {
     return;
   }
 
-  ClassMember::Print(final, printer);
+  ClassMember::Print(final,  printer, strip_api_annotations);
 
   printer->Print("public ");
   if (qualifier_ == ClassQualifier::kStatic) {
@@ -93,7 +93,7 @@ void ClassDefinition::Print(bool final, Printer* printer) const {
     // and takes precedence over a previous member with the same name. The overridden member is
     // set to nullptr.
     if (member != nullptr) {
-      member->Print(final, printer);
+      member->Print(final, printer, strip_api_annotations);
       printer->Println();
     }
   }
@@ -111,11 +111,11 @@ constexpr static const char* sWarningHeader =
     " */\n\n";
 
 void ClassDefinition::WriteJavaFile(const ClassDefinition* def, const StringPiece& package,
-                                    bool final, io::OutputStream* out) {
+                                    bool final, bool strip_api_annotations, io::OutputStream* out) {
   Printer printer(out);
   printer.Print(sWarningHeader).Print("package ").Print(package).Println(";");
   printer.Println();
-  def->Print(final, &printer);
+  def->Print(final, &printer, strip_api_annotations);
 }
 
 }  // namespace aapt

tools/aapt2/java/ClassDefinition.h

@@ -50,7 +50,7 @@ class ClassMember {
   // Writes the class member to the Printer. Subclasses should derive this method
   // to write their own data. Call this base method from the subclass to write out
   // this member's comments/annotations.
-  virtual void Print(bool final, text::Printer* printer) const;
+  virtual void Print(bool final, text::Printer* printer, bool strip_api_annotations = false) const;
 
  private:
   AnnotationProcessor processor_;
@@ -70,10 +70,11 @@ class PrimitiveMember : public ClassMember {
     return name_;
   }
 
-  void Print(bool final, text::Printer* printer) const override {
+  void Print(bool final, text::Printer* printer, bool strip_api_annotations = false)
+      const override {
     using std::to_string;
 
-    ClassMember::Print(final, printer);
+    ClassMember::Print(final, printer, strip_api_annotations);
 
     printer->Print("public static ");
     if (final) {
@@ -104,8 +105,9 @@ class PrimitiveMember<std::string> : public ClassMember {
     return name_;
   }
 
-  void Print(bool final, text::Printer* printer) const override {
-    ClassMember::Print(final, printer);
+  void Print(bool final, text::Printer* printer, bool strip_api_annotations = false)
+      const override {
+    ClassMember::Print(final, printer, strip_api_annotations);
 
     printer->Print("public static ");
     if (final) {
@@ -142,8 +144,9 @@ class PrimitiveArrayMember : public ClassMember {
     return name_;
   }
 
-  void Print(bool final, text::Printer* printer) const override {
-    ClassMember::Print(final, printer);
+  void Print(bool final, text::Printer* printer, bool strip_api_annotations = false)
+      const override {
+    ClassMember::Print(final, printer, strip_api_annotations);
 
     printer->Print("public static final int[] ").Print(name_).Print("={");
     printer->Indent();
@@ -195,7 +198,7 @@ class MethodDefinition : public ClassMember {
     return false;
   }
 
-  void Print(bool final, text::Printer* printer) const override;
+  void Print(bool final, text::Printer* printer, bool strip_api_annotations = false) const override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MethodDefinition);
@@ -209,7 +212,7 @@ enum class ClassQualifier { kNone, kStatic };
 class ClassDefinition : public ClassMember {
  public:
   static void WriteJavaFile(const ClassDefinition* def, const android::StringPiece& package,
-                            bool final, io::OutputStream* out);
+                            bool final, bool strip_api_annotations, io::OutputStream* out);
 
   ClassDefinition(const android::StringPiece& name, ClassQualifier qualifier, bool createIfEmpty)
       : name_(name.to_string()), qualifier_(qualifier), create_if_empty_(createIfEmpty) {}
@@ -227,7 +230,7 @@ class ClassDefinition : public ClassMember {
     return name_;
   }
 
-  void Print(bool final, text::Printer* printer) const override;
+  void Print(bool final, text::Printer* printer, bool strip_api_annotations = false) const override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ClassDefinition);

tools/aapt2/java/JavaClassGenerator.cpp

@@ -604,6 +604,8 @@ bool JavaClassGenerator::Generate(const StringPiece& package_name_to_generate,
     rewrite_method->AppendStatement("final int packageIdBits = p << 24;");
   }
 
+  const bool is_public = (options_.types == JavaClassGeneratorOptions::SymbolTypes::kPublic);
+
   for (const auto& package : table_->packages) {
     for (const auto& type : package->types) {
       if (type->type == ResourceType::kAttrPrivate) {
@@ -612,8 +614,7 @@ bool JavaClassGenerator::Generate(const StringPiece& package_name_to_generate,
       }
 
       // Stay consistent with AAPT and generate an empty type class if the R class is public.
-      const bool force_creation_if_empty =
-          (options_.types == JavaClassGeneratorOptions::SymbolTypes::kPublic);
+      const bool force_creation_if_empty = is_public;
 
       std::unique_ptr<ClassDefinition> class_def;
       if (out != nullptr) {
@@ -637,8 +638,7 @@ bool JavaClassGenerator::Generate(const StringPiece& package_name_to_generate,
         }
       }
 
-      if (out != nullptr && type->type == ResourceType::kStyleable &&
-          options_.types == JavaClassGeneratorOptions::SymbolTypes::kPublic) {
+      if (out != nullptr && type->type == ResourceType::kStyleable && is_public) {
         // When generating a public R class, we don't want Styleable to be part
         // of the API. It is only emitted for documentation purposes.
         class_def->GetCommentBuilder()->AppendComment("@doconly");
@@ -657,7 +657,7 @@ bool JavaClassGenerator::Generate(const StringPiece& package_name_to_generate,
 
   if (out != nullptr) {
     AppendJavaDocAnnotations(options_.javadoc_annotations, r_class.GetCommentBuilder());
-    ClassDefinition::WriteJavaFile(&r_class, out_package_name, options_.use_final, out);
+    ClassDefinition::WriteJavaFile(&r_class, out_package_name, options_.use_final, !is_public, out);
   }
   return true;
 }

tools/aapt2/java/ManifestClassGenerator_test.cpp

@@ -26,6 +26,7 @@ using ::testing::Not;
 namespace aapt {
 
 static ::testing::AssertionResult GetManifestClassText(IAaptContext* context, xml::XmlResource* res,
+                                                       bool strip_api_annotations,
                                                        std::string* out_str);
 
 TEST(ManifestClassGeneratorTest, NameIsProperlyGeneratedFromSymbol) {
@@ -39,7 +40,8 @@ TEST(ManifestClassGeneratorTest, NameIsProperlyGeneratedFromSymbol) {
       </manifest>)");
 
   std::string actual;
-  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(), &actual));
+  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(),
+                                   false /* strip_api_annotations */, &actual));
 
   ASSERT_THAT(actual, HasSubstr("public static final class permission {"));
   ASSERT_THAT(actual, HasSubstr("public static final class permission_group {"));
@@ -91,7 +93,8 @@ TEST(ManifestClassGeneratorTest, CommentsAndAnnotationsArePresent) {
       </manifest>)");
 
   std::string actual;
-  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(), &actual));
+  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(),
+                                   false /* strip_api_annotations */, &actual));
 
   const char* expected_access_internet = R"(    /**
      * Required to access the internet.
@@ -123,6 +126,55 @@ TEST(ManifestClassGeneratorTest, CommentsAndAnnotationsArePresent) {
   EXPECT_THAT(actual, HasSubstr(expected_test));
 }
 
+TEST(ManifestClassGeneratorTest, CommentsAndAnnotationsArePresentButNoApiAnnotations) {
+  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+  std::unique_ptr<xml::XmlResource> manifest = test::BuildXmlDom(R"(
+      <manifest xmlns:android="http://schemas.android.com/apk/res/android">
+        <!-- Required to access the internet.
+             Added in API 1. -->
+        <permission android:name="android.permission.ACCESS_INTERNET" />
+        <!-- @deprecated This permission is for playing outside. -->
+        <permission android:name="android.permission.PLAY_OUTSIDE" />
+        <!-- This is a private permission for system only!
+             @hide
+             @SystemApi -->
+        <permission android:name="android.permission.SECRET" />
+        <!-- @TestApi This is a test only permission. -->
+        <permission android:name="android.permission.TEST_ONLY" />
+      </manifest>)");
+
+  std::string actual;
+  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(),
+                                   true /* strip_api_annotations */, &actual));
+
+  const char* expected_access_internet = R"(    /**
+     * Required to access the internet.
+     * Added in API 1.
+     */
+    public static final String ACCESS_INTERNET="android.permission.ACCESS_INTERNET";)";
+  EXPECT_THAT(actual, HasSubstr(expected_access_internet));
+
+  const char* expected_play_outside = R"(    /**
+     * @deprecated This permission is for playing outside.
+     */
+    @Deprecated
+    public static final String PLAY_OUTSIDE="android.permission.PLAY_OUTSIDE";)";
+  EXPECT_THAT(actual, HasSubstr(expected_play_outside));
+
+  const char* expected_secret = R"(    /**
+     * This is a private permission for system only!
+     * @hide
+     */
+    public static final String SECRET="android.permission.SECRET";)";
+  EXPECT_THAT(actual, HasSubstr(expected_secret));
+
+  const char* expected_test = R"(    /**
+     * This is a test only permission.
+     */
+    public static final String TEST_ONLY="android.permission.TEST_ONLY";)";
+  EXPECT_THAT(actual, HasSubstr(expected_test));
+}
+
 // This is bad but part of public API behaviour so we need to preserve it.
 TEST(ManifestClassGeneratorTest, LastSeenPermissionWithSameLeafNameTakesPrecedence) {
   std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
@@ -135,7 +187,8 @@ TEST(ManifestClassGeneratorTest, LastSeenPermissionWithSameLeafNameTakesPreceden
       </manifest>)");
 
   std::string actual;
-  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(), &actual));
+  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(),
+                                   false  /* strip_api_annotations */, &actual));
   EXPECT_THAT(actual, HasSubstr("ACCESS_INTERNET=\"com.android.aapt.test.ACCESS_INTERNET\";"));
   EXPECT_THAT(actual, Not(HasSubstr("ACCESS_INTERNET=\"android.permission.ACCESS_INTERNET\";")));
   EXPECT_THAT(actual, Not(HasSubstr("ACCESS_INTERNET=\"com.android.sample.ACCESS_INTERNET\";")));
@@ -149,11 +202,13 @@ TEST(ManifestClassGeneratorTest, NormalizePermissionNames) {
         </manifest>)");
 
   std::string actual;
-  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(), &actual));
+  ASSERT_TRUE(GetManifestClassText(context.get(), manifest.get(),
+                                   false  /* strip_api_annotations */, &actual));
   EXPECT_THAT(actual, HasSubstr("access_internet=\"android.permission.access-internet\";"));
 }
 
 static ::testing::AssertionResult GetManifestClassText(IAaptContext* context, xml::XmlResource* res,
+                                                       bool strip_api_annotations,
                                                        std::string* out_str) {
   std::unique_ptr<ClassDefinition> manifest_class =
       GenerateManifestClass(context->GetDiagnostics(), res);
@@ -162,7 +217,7 @@ static ::testing::AssertionResult GetManifestClassText(IAaptContext* context, xm
   }
 
   StringOutputStream out(out_str);
-  manifest_class->WriteJavaFile(manifest_class.get(), "android", true, &out);
+  manifest_class->WriteJavaFile(manifest_class.get(), "android", true, strip_api_annotations, &out);
   out.Flush();
   return ::testing::AssertionSuccess();
 }