aidl: retain comments after annotations am: 2d32abc343 am: adbdd46a81

Original change: https://android-review.googlesource.com/c/platform/system/tools/aidl/+/2889794

Change-Id: I1f10508300d4a50ed6f0e07dcde825eda9d69eca
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

9 files changed, 65 insertions, 54 deletions

diff --git a/aidl_language.h b/aidl_language.h
index cf74a67e..8529bdb3 100644
--- a/aidl_language.h
+++ b/aidl_language.h
@@ -160,7 +160,9 @@ class AidlNode {
   virtual void DispatchVisit(AidlVisitor&) const = 0;
 
   const Comments& GetComments() const { return comments_; }
-  void SetComments(const Comments& comments) { comments_ = comments; }
+  void PrependComments(const Comments& comments) {
+    comments_.insert(comments_.begin(), comments.begin(), comments.end());
+  }
 
   static void ClearUnvisitedNodes();
   static const std::vector<AidlLocation>& GetLocationsOfUnvisitedNodes();
@@ -343,6 +345,13 @@ class AidlAnnotatable : public AidlCommentable {
   virtual ~AidlAnnotatable() = default;
 
   void Annotate(vector<std::unique_ptr<AidlAnnotation>>&& annotations) {
+    for (auto i = annotations.rbegin(); i != annotations.rend(); ++i) {
+      // TODO: we may want to mark all comments as "unoutputed"
+      // in the lexer phase, and then verify after successful backend
+      // output that every comment is "emitted". If we do this, we would
+      // need to move rather than copy the comments here.
+      PrependComments((*i)->GetComments());
+    }
     for (auto& annotation : annotations) {
       annotations_.emplace_back(std::move(annotation));
     }
@@ -892,6 +901,8 @@ class AidlMethod : public AidlMember {
   const std::string& GetName() const { return name_; }
   bool HasId() const { return has_id_; }
   int GetId() const { return id_; }
+
+  // TODO: Set is errorprone, what if it was set before?
   void SetId(unsigned id) { id_ = id; }
 
   const std::vector<std::unique_ptr<AidlArgument>>& GetArguments() const {
@@ -1133,6 +1144,7 @@ class AidlEnumerator : public AidlCommentable {
   string ValueString(const AidlTypeSpecifier& backing_type,
                      const ConstantValueDecorator& decorator) const;
 
+  // TODO: Set is errorprone. What if it was set before?
   void SetValue(std::unique_ptr<AidlConstantValue> value) { value_ = std::move(value); }
   bool IsValueUserSpecified() const { return value_user_specified_; }
 
diff --git a/aidl_language_y.yy b/aidl_language_y.yy
index 3f5df548..749c850c 100644
--- a/aidl_language_y.yy
+++ b/aidl_language_y.yy
@@ -185,17 +185,20 @@ AidlLocation loc(const yy::parser::location_type& l) {
 
 document
  : optional_package imports decls {
-    Comments comments;
-    if ($1) {
-      comments = $1->GetComments();
-    } else if (!$2->empty()) {
-      comments = $2->front()->GetComments();
-    }
     std::vector<std::string> imports;
     for (const auto& import : *$2) {
       imports.push_back(import->GetText());
     }
-    ps->MakeDocument(loc(@1), comments, std::move(imports), std::move(*$3));
+
+    ps->MakeDocument(loc(@1), Comments(), std::move(imports), std::move(*$3));
+
+    for (auto i = $2->rbegin(); i != $2->rend(); ++i) {
+      ps->GetDocument()->PrependComments((*i)->GetComments());
+    }
+    if ($1) {
+      ps->GetDocument()->PrependComments($1->GetComments());
+    }
+
     delete $1;
     delete $2;
     delete $3;
@@ -271,9 +274,7 @@ decl
    {
     $$ = $2;
 
-    if ($1->size() > 0 && $$ != nullptr) {
-      // copy comments from annotation to decl
-      $$->SetComments($1->begin()->get()->GetComments());
+    if ($$ != nullptr) {
       $$->Annotate(std::move(*$1));
     }
 
@@ -571,11 +572,7 @@ constant_value_non_empty_list
 
 constant_decl
  : annotation_list CONST type identifier '=' const_expr ';' {
-    if ($1->size() > 0) {
-      $3->SetComments($1->begin()->get()->GetComments());
-    } else {
-      $3->SetComments($2->GetComments());
-    }
+    $3->PrependComments($2->GetComments());
     // TODO(b/151102494) do not merge annotations.
     $3->Annotate(std::move(*$1));
     $$ = new AidlConstantDeclaration(loc(@4), $3, $4->GetText(), $6);
@@ -633,13 +630,15 @@ union_decl
 
 method_decl
  : type identifier '(' arg_list ')' ';' {
-    $$ = new AidlMethod(loc(@2), false, $1, $2->GetText(), $4, $1->GetComments());
+    $$ = new AidlMethod(loc(@2), false, $1, $2->GetText(), $4, $2->GetComments());
+    $$->PrependComments($1->GetComments());
     delete $2;
   }
  | annotation_list ONEWAY type identifier '(' arg_list ')' ';' {
-    const auto& comments = ($1->size() > 0) ? $1->begin()->get()->GetComments() : $2->GetComments();
-    $$ = new AidlMethod(loc(@4), true, $3, $4->GetText(), $6, comments);
+    $$ = new AidlMethod(loc(@4), true, $3, $4->GetText(), $6, $4->GetComments());
+    $3->PrependComments($2->GetComments());
     $3->Annotate(std::move(*$1));
+    $$->PrependComments($3->GetComments());
     delete $1;
     delete $2;
     delete $4;
@@ -650,19 +649,21 @@ method_decl
         AIDL_ERROR(loc(@7)) << "Could not parse int value: " << $7->GetText();
         ps->AddError();
     }
-    $$ = new AidlMethod(loc(@2), false, $1, $2->GetText(), $4, $1->GetComments(), serial);
+    $$ = new AidlMethod(loc(@2), false, $1, $2->GetText(), $4, $2->GetComments(), serial);
+    $$->PrependComments($1->GetComments());
     delete $2;
     delete $7;
   }
  | annotation_list ONEWAY type identifier '(' arg_list ')' '=' INTVALUE ';' {
-    const auto& comments = ($1->size() > 0) ? $1->begin()->get()->GetComments() : $2->GetComments();
     int32_t serial = 0;
     if (!android::base::ParseInt($9->GetText(), &serial)) {
         AIDL_ERROR(loc(@9)) << "Could not parse int value: " << $9->GetText();
         ps->AddError();
     }
-    $$ = new AidlMethod(loc(@4), true, $3, $4->GetText(), $6, comments, serial);
+    $$ = new AidlMethod(loc(@4), true, $3, $4->GetText(), $6, $4->GetComments(), serial);
+    $3->PrependComments($2->GetComments());
     $3->Annotate(std::move(*$1));
+    $$->PrependComments($3->GetComments());
     delete $1;
     delete $2;
     delete $4;
@@ -698,10 +699,7 @@ arg
 non_array_type
  : annotation_list qualified_name {
     $$ = new AidlTypeSpecifier(loc(@2), $2->GetText(), /*array=*/std::nullopt, nullptr, $2->GetComments());
-    if (!$1->empty()) {
-      $$->SetComments($1->begin()->get()->GetComments());
-      $$->Annotate(std::move(*$1));
-    }
+    $$->Annotate(std::move(*$1));
     delete $1;
     delete $2;
   }
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp
index d9f1564c..21693e1c 100644
--- a/aidl_unittest.cpp
+++ b/aidl_unittest.cpp
@@ -2481,6 +2481,7 @@ TEST_F(AidlTest, ApiDump) {
                                "// comment\n"
                                "package foo.bar;\n"
                                "import foo.bar.IFoo;\n"
+                               "@JavaDerive(equals=true, toString=true)\n"
                                "/* @hide*/\n"
                                "parcelable Data {\n"
                                "   // @hide\n"
@@ -2523,6 +2524,7 @@ interface IFoo {
   EXPECT_TRUE(io_delegate_.GetWrittenContents("dump/foo/bar/Data.aidl", &actual));
   EXPECT_EQ(string("// comment\n").append(string(kPreamble)).append(R"(package foo.bar;
 /* @hide */
+@JavaDerive(equals=true, toString=true)
 parcelable Data {
   int x = 10;
   int y;
diff --git a/comments.cpp b/comments.cpp
index 8feee285..f576013f 100644
--- a/comments.cpp
+++ b/comments.cpp
@@ -171,42 +171,25 @@ Comment::Comment(const std::string& body) : body(body) {
   }
 }
 
-// Returns the immediate block comment from the list of comments.
-// Only the last/block comment can have the tag.
-//
-//   /* @hide */
-//   int x;
-//
-// But tags in line or distant comments don't count. In the following,
-// the variable 'x' is not hidden.
-//
-//    // @hide
-//    int x;
-//
-//    /* @hide */
-//    /* this is the immemediate comment to 'x' */
-//    int x;
-//
-static std::optional<Comment> GetValidComment(const Comments& comments) {
-  if (!comments.empty() && comments.back().type == Comment::Type::BLOCK) {
-    return comments.back();
-  }
-  return std::nullopt;
-}
-
 // Sees if comments have the @hide tag.
 // Example: /** @hide */
 bool HasHideInComments(const Comments& comments) {
-  const auto valid_comment = GetValidComment(comments);
-  return valid_comment && std::regex_search(valid_comment->body, kTagHideRegex);
+  for (const Comment& comment : comments) {
+    if (comment.type != Comment::Type::BLOCK) continue;
+    if (!std::regex_search(comment.body, kTagHideRegex)) continue;
+    return true;
+  }
+  return false;
 }
 
 // Finds the @deprecated tag in comments and returns it with optional note which
 // follows the tag.
 // Example: /** @deprecated reason */
 std::optional<Deprecated> FindDeprecated(const Comments& comments) {
-  if (const auto valid_comment = GetValidComment(comments); valid_comment) {
-    for (const auto& [name, description] : BlockTags(comments.back())) {
+  for (const Comment& comment : comments) {
+    if (comment.type != Comment::Type::BLOCK) continue;
+
+    for (const auto& [name, description] : BlockTags(comment)) {
       // take the first @deprecated
       if (kTagDeprecated == name) {
         return Deprecated{description};
diff --git a/comments.h b/comments.h
index 8bf7b30f..d1015ef7 100644
--- a/comments.h
+++ b/comments.h
@@ -38,6 +38,7 @@ struct Comment {
   friend std::ostream& operator<<(std::ostream& out, const Comment& c) { return out << c.body; }
 };
 
+// TODO: smoreland says don't do this - instead use 'std::vector<Comment>' everywhere
 using Comments = std::vector<Comment>;
 
 bool HasHideInComments(const Comments& comments);
@@ -51,4 +52,4 @@ std::optional<Deprecated> FindDeprecated(const Comments& comments);
 std::string FormatCommentsForJava(const Comments& comments);
 
 }  // namespace aidl
-}  // namespace android
\ No newline at end of file
+}  // namespace android
diff --git a/parser.h b/parser.h
index 074d6d50..51b88aa3 100644
--- a/parser.h
+++ b/parser.h
@@ -76,6 +76,8 @@ class Parser {
   const std::string& FileName() const { return filename_; }
   void* Scanner() const { return scanner_; }
 
+  AidlDocument* GetDocument() { return document_.get(); }
+
   // This restricts the grammar to something more reasonable. One alternative
   // would be to support multiple sets of type specifiers in our AST, but then a
   // lot of later code would have to deal with this more complicated type. So,
diff --git a/tests/android/aidl/tests/ITestService.aidl b/tests/android/aidl/tests/ITestService.aidl
index 101ca3ab..3c98c3ea 100644
--- a/tests/android/aidl/tests/ITestService.aidl
+++ b/tests/android/aidl/tests/ITestService.aidl
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+// package comment
 package android.aidl.tests;
 
 import android.aidl.tests.BackendType;
@@ -22,6 +23,7 @@ import android.aidl.tests.CircularParcelable;
 import android.aidl.tests.ICircular;
 import android.aidl.tests.INamedCallback;
 import android.aidl.tests.INewName;
+// import comment
 import android.aidl.tests.IOldName;
 import android.aidl.tests.IntEnum;
 import android.aidl.tests.LongEnum;
@@ -35,8 +37,13 @@ import android.aidl.tests.extension.ExtendableParcelable;
  * interface comment
  */
 @SuppressWarnings(value={"inout-parameter", "mixed-oneway", "out-array", "interface-name"})
+/**
+ * interface comment 2
+ */
 @SensitiveData
+// interface comment 3
 @JavaDefault
+// interface comment 4
 @JavaDelegator
 interface ITestService {
     // Test that constants are accessible
diff --git a/tests/golden_output/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java b/tests/golden_output/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java
index 4011316e..bb1770b8 100644
--- a/tests/golden_output/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java
+++ b/tests/golden_output/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java
@@ -4,6 +4,9 @@
  */
 package android.aidl.tests;
 /** interface comment */
+/** interface comment 2 */
+// interface comment 3
+// interface comment 4
 public interface ITestService extends android.os.IInterface
 {
   /** Default implementation for ITestService. */
diff --git a/tests/golden_output/frozen/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java b/tests/golden_output/frozen/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java
index 4011316e..bb1770b8 100644
--- a/tests/golden_output/frozen/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java
+++ b/tests/golden_output/frozen/aidl-test-interface-java-source/gen/android/aidl/tests/ITestService.java
@@ -4,6 +4,9 @@
  */
 package android.aidl.tests;
 /** interface comment */
+/** interface comment 2 */
+// interface comment 3
+// interface comment 4
 public interface ITestService extends android.os.IInterface
 {
   /** Default implementation for ITestService. */