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
Test: build (treehugger)
Test: atest aapt2_tests
Change-Id: I613e8611ddaf5f8e4761d351d4cd0142d59c7cc9
diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index 3a3fb28..72cb41a 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -1272,7 +1272,8 @@
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()) {
diff --git a/tools/aapt2/java/AnnotationProcessor.cpp b/tools/aapt2/java/AnnotationProcessor.cpp
index cec59e7..482d91a 100644
--- a/tools/aapt2/java/AnnotationProcessor.cpp
+++ b/tools/aapt2/java/AnnotationProcessor.cpp
@@ -123,7 +123,7 @@
}
}
-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 @@
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()) {
diff --git a/tools/aapt2/java/AnnotationProcessor.h b/tools/aapt2/java/AnnotationProcessor.h
index fdb5846..f217afb 100644
--- a/tools/aapt2/java/AnnotationProcessor.h
+++ b/tools/aapt2/java/AnnotationProcessor.h
@@ -65,7 +65,7 @@
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_;
diff --git a/tools/aapt2/java/AnnotationProcessor_test.cpp b/tools/aapt2/java/AnnotationProcessor_test.cpp
index 7d0a4e9..6bc8902 100644
--- a/tools/aapt2/java/AnnotationProcessor_test.cpp
+++ b/tools/aapt2/java/AnnotationProcessor_test.cpp
@@ -91,6 +91,21 @@
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"));
diff --git a/tools/aapt2/java/ClassDefinition.cpp b/tools/aapt2/java/ClassDefinition.cpp
index f5f5b05..3163497 100644
--- a/tools/aapt2/java/ClassDefinition.cpp
+++ b/tools/aapt2/java/ClassDefinition.cpp
@@ -23,15 +23,15 @@
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 @@
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 @@
// 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 @@
" */\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
diff --git a/tools/aapt2/java/ClassDefinition.h b/tools/aapt2/java/ClassDefinition.h
index fb11266..1e4b681 100644
--- a/tools/aapt2/java/ClassDefinition.h
+++ b/tools/aapt2/java/ClassDefinition.h
@@ -50,7 +50,7 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);
diff --git a/tools/aapt2/java/JavaClassGenerator.cpp b/tools/aapt2/java/JavaClassGenerator.cpp
index bb541fe..dffad3b 100644
--- a/tools/aapt2/java/JavaClassGenerator.cpp
+++ b/tools/aapt2/java/JavaClassGenerator.cpp
@@ -604,6 +604,8 @@
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 @@
}
// 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 @@
}
}
- 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 @@
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;
}
diff --git a/tools/aapt2/java/ManifestClassGenerator_test.cpp b/tools/aapt2/java/ManifestClassGenerator_test.cpp
index ab7f9a1..3858fc7 100644
--- a/tools/aapt2/java/ManifestClassGenerator_test.cpp
+++ b/tools/aapt2/java/ManifestClassGenerator_test.cpp
@@ -26,6 +26,7 @@
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 @@
</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 @@
</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 @@
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 @@
</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 @@
</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 @@
}
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();
}