Changing enum autofill to use aidl file location instead of cpp

This location is printed when there are any issues with the aidl node.
In this case, when implicitly starting enumerators from 0 or implicitly
adding 1 to the next enumerator, we are now using the location of that
enumerator in the .aidl files instead of the .cpp file. This is intended
to give more useful information to the user.

Added a check to make sure we don't output internal file locations.

Test: atest aidl_unittests aidl_integration_test
Change-Id: I0b2b7a18e69c313668aea395a2a02a77ae27dfc1

5 files changed, 39 insertions, 22 deletions

diff --git a/aidl.cpp b/aidl.cpp
index 2c9bd2ec..e225548d 100644
--- a/aidl.cpp
+++ b/aidl.cpp
@@ -374,7 +374,7 @@ bool parse_preprocessed_file(const IoDelegate& io_delegate, const string& filena
     }
 
     AidlLocation::Point point = {.line = lineno, .column = 0 /*column*/};
-    AidlLocation location = AidlLocation(filename, point, point);
+    AidlLocation location = AidlLocation(filename, point, point, AidlLocation::Source::EXTERNAL);
 
     if (decl == "parcelable") {
       // ParcelFileDescriptor is treated as a built-in type, but it's also in the framework.aidl.
diff --git a/aidl_language.cpp b/aidl_language.cpp
index a9175627..bd2474c1 100644
--- a/aidl_language.cpp
+++ b/aidl_language.cpp
@@ -89,8 +89,8 @@ AidlToken::AidlToken(const std::string& text, const std::string& comments)
     : text_(text),
       comments_(comments) {}
 
-AidlLocation::AidlLocation(const std::string& file, Point begin, Point end)
-    : file_(file), begin_(begin), end_(end) {}
+AidlLocation::AidlLocation(const std::string& file, Point begin, Point end, Source source)
+    : file_(file), begin_(begin), end_(end), source_(source) {}
 
 std::ostream& operator<<(std::ostream& os, const AidlLocation& l) {
   os << l.file_ << ":" << l.begin_.line << "." << l.begin_.column << "-";
@@ -122,6 +122,12 @@ AidlError::AidlError(bool fatal) : os_(std::cerr), fatal_(fatal) {
   os_ << "ERROR: ";
 }
 
+AidlError::AidlError(bool fatal, const AidlLocation& location) : AidlError(fatal) {
+  CHECK(!location.IsInternal())
+      << "Logging an internal location should not happen. Offending location: " << location;
+  os_ << location << ": ";
+}
+
 bool AidlError::sHadError = false;
 
 static const string kNullable("nullable");
@@ -938,8 +944,8 @@ bool AidlEnumDeclaration::Autofill() {
   for (const auto& enumerator : enumerators_) {
     if (enumerator->GetValue() == nullptr) {
       if (previous == nullptr) {
-        enumerator->SetValue(std::unique_ptr<AidlConstantValue>(
-            AidlConstantValue::Integral(AIDL_LOCATION_HERE, "0")));
+        enumerator->SetValue(
+            std::unique_ptr<AidlConstantValue>(AidlConstantValue::Integral(GetLocation(), "0")));
       } else {
         auto prev_value = std::unique_ptr<AidlConstantValue>(
             AidlConstantValue::ShallowIntegralCopy(*previous->GetValue()));
@@ -947,9 +953,8 @@ bool AidlEnumDeclaration::Autofill() {
           return false;
         }
         enumerator->SetValue(std::make_unique<AidlBinaryConstExpression>(
-            AIDL_LOCATION_HERE, std::move(prev_value), "+",
-            std::unique_ptr<AidlConstantValue>(
-                AidlConstantValue::Integral(AIDL_LOCATION_HERE, "1"))));
+            GetLocation(), std::move(prev_value), "+",
+            std::unique_ptr<AidlConstantValue>(AidlConstantValue::Integral(GetLocation(), "1"))));
       }
     }
     previous = enumerator.get();
diff --git a/aidl_language.h b/aidl_language.h
index 55bed950..97c8ba38 100644
--- a/aidl_language.h
+++ b/aidl_language.h
@@ -74,7 +74,16 @@ class AidlLocation {
     int column;
   };
 
-  AidlLocation(const std::string& file, Point begin, Point end);
+  enum class Source {
+    // From internal aidl source code
+    INTERNAL = 0,
+    // From a parsed file
+    EXTERNAL = 1
+  };
+
+  AidlLocation(const std::string& file, Point begin, Point end, Source source);
+
+  bool IsInternal() const { return source_ == Source::INTERNAL; }
 
   friend std::ostream& operator<<(std::ostream& os, const AidlLocation& l);
   friend class AidlNode;
@@ -83,12 +92,11 @@ class AidlLocation {
   const std::string file_;
   Point begin_;
   Point end_;
+  Source source_;
 };
 
-#define AIDL_LOCATION_HERE                   \
-  AidlLocation {                             \
-    __FILE__, {__LINE__, 0}, { __LINE__, 0 } \
-  }
+#define AIDL_LOCATION_HERE \
+  AidlLocation { __FILE__, {__LINE__, 0}, {__LINE__, 0}, AidlLocation::Source::INTERNAL }
 
 std::ostream& operator<<(std::ostream& os, const AidlLocation& l);
 
@@ -101,15 +109,15 @@ class AidlNode {
   AidlNode(AidlNode&&) = default;
   virtual ~AidlNode() = default;
 
-  // DO NOT ADD. This is intentionally omitted. Nothing should refer to the location
-  // for a functional purpose. It is only for error messages.
-  // NO const AidlLocation& GetLocation() const { return location_; } NO
-
-  // To be able to print AidlLocation (nothing else should use this information)
+  // To be able to print AidlLocation
   friend class AidlError;
   friend std::string android::aidl::mappings::dump_location(const AidlNode&);
   friend std::string android::aidl::java::dump_location(const AidlNode&);
 
+ protected:
+  // This should only be used to construct implicit nodes related to existing nodes
+  const AidlLocation& GetLocation() const { return location_; }
+
  private:
   std::string PrintLine() const;
   std::string PrintLocation() const;
@@ -120,9 +128,7 @@ class AidlNode {
 class AidlError {
  public:
   AidlError(bool fatal, const std::string& filename) : AidlError(fatal) { os_ << filename << ": "; }
-  AidlError(bool fatal, const AidlLocation& location) : AidlError(fatal) {
-    os_ << location << ": ";
-  }
+  AidlError(bool fatal, const AidlLocation& location);
   AidlError(bool fatal, const AidlNode& node) : AidlError(fatal, node.location_) {}
   AidlError(bool fatal, const AidlNode* node) : AidlError(fatal, *node) {}
 
diff --git a/aidl_language_y.yy b/aidl_language_y.yy
index 67a7b11b..3a1fda93 100644
--- a/aidl_language_y.yy
+++ b/aidl_language_y.yy
@@ -39,7 +39,7 @@ AidlLocation loc(const yy::parser::location_type& begin, const yy::parser::locat
     .line = end.end.line,
     .column = end.end.column,
   };
-  return AidlLocation(*begin.begin.filename, begin_point, end_point);
+  return AidlLocation(*begin.begin.filename, begin_point, end_point, AidlLocation::Source::EXTERNAL);
 }
 
 AidlLocation loc(const yy::parser::location_type& l) {
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp
index 8e428564..1281669f 100644
--- a/aidl_unittest.cpp
+++ b/aidl_unittest.cpp
@@ -1801,6 +1801,11 @@ TEST_F(AidlTest, FailOnOutOfBoundsInt64MinConstInt) {
 
 TEST_F(AidlTest, FailOnOutOfBoundsAutofilledEnum) {
   AidlError reported_error;
+  const string expected_stderr =
+      "ERROR: p/TestEnum.aidl:3.35-44: Invalid type specifier for an int32 "
+      "literal: byte\n"
+      "ERROR: p/TestEnum.aidl:5.1-36: Enumerator type differs from enum backing type.\n";
+  CaptureStderr();
   EXPECT_EQ(nullptr, Parse("p/TestEnum.aidl",
                            R"(package p;
                               @Backing(type="byte")
@@ -1811,6 +1816,7 @@ TEST_F(AidlTest, FailOnOutOfBoundsAutofilledEnum) {
                              )",
                            typenames_, Options::Language::CPP, &reported_error));
   EXPECT_EQ(AidlError::BAD_TYPE, reported_error);
+  EXPECT_EQ(expected_stderr, GetCapturedStderr());
 }
 
 }  // namespace aidl