Create the Tag enum for unions as a DERIVED location We create internal Tag enums inside of unions for the descriminators. We throw errors in some cases if those are handled in a backward incompatible way. We treat errors from INTERNAL locations as fatal errors and it causes problems with our fuzzers. Now we have a DERIVED location type for this case where we want the type to be logged when there is a problem, but it is not an EXTERNAL type from another aidl file. Test: atest aidl_unittests Bug: 402354469 Change-Id: I7a028b9c8040f6901750a0308238cc2ed7a830fa
diff --git a/aidl_language.h b/aidl_language.h index 8d25e61..5a724c5 100644 --- a/aidl_language.h +++ b/aidl_language.h
@@ -167,7 +167,7 @@ static void ClearUnvisitedNodes(); static const std::vector<AidlLocation>& GetLocationsOfUnvisitedNodes(); void MarkVisited() const; - bool IsUserDefined() const { return !GetLocation().IsInternal(); } + bool IsUserDefined() const { return !GetLocation().IsInternal() && !GetLocation().IsDerived(); } private: std::string PrintLine() const;
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp index ca4f036..6b85169 100644 --- a/aidl_unittest.cpp +++ b/aidl_unittest.cpp
@@ -3397,6 +3397,22 @@ EXPECT_EQ(expected_stderr, GetCapturedStderr()); } +TEST_F(AidlTestIncompatibleChanges, RemovedTypeNotInternal) { + // We create new Tag enums internal to unions and they show up in these logs + const string expected_stderr = + "ERROR: old/p/Foo.aidl:1.16-20: Removed type: p.Foo\n" + "ERROR: (derived from)old/p/Foo.aidl:1.16-20: Removed type: p.Foo.Tag\n"; + io_delegate_.SetFileContents("old/p/Foo.aidl", + "package p;" + "union Foo {" + " int foo;" + " int bar;" + "}"); + CaptureStderr(); + EXPECT_FALSE(::android::aidl::check_api(options_, io_delegate_)); + EXPECT_EQ(expected_stderr, GetCapturedStderr()); +} + TEST_F(AidlTestIncompatibleChanges, RemovedMethod) { const string expected_stderr = "ERROR: old/p/IFoo.aidl:1.61-65: Removed or changed method: p.IFoo.bar(String)\n"; @@ -3552,7 +3568,8 @@ TEST_F(AidlTestIncompatibleChanges, RemovedUnionField) { const string expected_stderr = - "ERROR: new/p/Union.aidl:1.16-22: Number of fields in p.Union is reduced from 2 to 1.\n"; + "ERROR: new/p/Union.aidl:1.16-22: Number of fields in p.Union is reduced from 2 to 1.\n" + "ERROR: (derived from)new/p/Union.aidl:1.16-22: Removed enumerator from p.Union.Tag: num\n"; io_delegate_.SetFileContents("old/p/Union.aidl", "package p;" "union Union {"
diff --git a/diagnostics.cpp b/diagnostics.cpp index 5c51f03..4f004b0 100644 --- a/diagnostics.cpp +++ b/diagnostics.cpp
@@ -61,7 +61,7 @@ DiagnosticsContext(DiagnosticMapping mapping) : mapping_({std::move(mapping)}) {} AidlErrorLog Report(const AidlLocation& loc, DiagnosticID id, DiagnosticSeverity force_severity = DiagnosticSeverity::DISABLED) { - if (loc.IsInternal()) { + if (loc.IsDerived() || loc.IsInternal()) { return AidlErrorLog(AidlErrorLog::NO_OP, loc); } const std::string suffix = " [-W" + to_string(id) + "]";
diff --git a/location.cpp b/location.cpp index 4b8a278..623df6a 100644 --- a/location.cpp +++ b/location.cpp
@@ -20,6 +20,9 @@ : file_(file), begin_(begin), end_(end), source_(source) {} std::ostream& operator<<(std::ostream& os, const AidlLocation& l) { + if (l.source_ == AidlLocation::Source::DERIVED_INTERNAL) { + os << "(derived from)"; + } os << l.file_; if (l.LocationKnown()) { os << ":" << l.begin_.line << "." << l.begin_.column << "-"; @@ -30,3 +33,8 @@ } return os; } + +std::optional<AidlLocation> AidlLocation::ToDerivedLocation() const { + if (source_ != Source::EXTERNAL) return std::nullopt; + return AidlLocation(file_, begin_, end_, Source::DERIVED_INTERNAL); +}
diff --git a/location.h b/location.h index 9ccad24..d46c46a 100644 --- a/location.h +++ b/location.h
@@ -17,6 +17,7 @@ #pragma once #include <iostream> +#include <optional> #include <string> class AidlLocation { @@ -28,9 +29,13 @@ enum class Source { // From internal aidl source code - INTERNAL = 0, + INTERNAL, // From a parsed file - EXTERNAL = 1 + EXTERNAL, + // Derived from a parsed file. These are used for types generated by + // the compiler that we still want to track mostly like EXTERNAL types. + // An example is the Tag enum that is generated for each EXTERNAL union. + DERIVED_INTERNAL, }; AidlLocation(const std::string& file, Point begin, Point end, Source source); @@ -38,12 +43,17 @@ : AidlLocation(file, {0, 0}, {0, 0}, source) {} bool IsInternal() const { return source_ == Source::INTERNAL; } + bool IsDerived() const { return source_ == Source::DERIVED_INTERNAL; } // The first line of a file is line 1. bool LocationKnown() const { return begin_.line != 0; } std::string GetFile() const { return file_; } + // Get an AidlLocation derived from this external location. + // nullopt if this location is not EXTERNAL + std::optional<AidlLocation> ToDerivedLocation() const; + friend std::ostream& operator<<(std::ostream& os, const AidlLocation& l); friend class AidlNode; @@ -60,4 +70,4 @@ #define AIDL_LOCATION_HERE \ (AidlLocation{__FILE__, {__LINE__, 0}, {__LINE__, 0}, AidlLocation::Source::INTERNAL}) -std::ostream& operator<<(std::ostream& os, const AidlLocation& l); \ No newline at end of file +std::ostream& operator<<(std::ostream& os, const AidlLocation& l);
diff --git a/parser.cpp b/parser.cpp index edb110b..cec403a 100644 --- a/parser.cpp +++ b/parser.cpp
@@ -34,10 +34,16 @@ void Visit(const AidlUnionDecl& decl) override { std::vector<std::unique_ptr<AidlEnumerator>> enumerators; for (const auto& field : decl.GetFields()) { - enumerators.push_back(std::make_unique<AidlEnumerator>(AIDL_LOCATION_HERE, field->GetName(), - nullptr, field->GetComments())); + auto derived = field->GetLocation().ToDerivedLocation(); + AIDL_FATAL_IF(!derived, field) + << "Failed to get a derived location. Is this not an external type?"; + enumerators.push_back(std::make_unique<AidlEnumerator>(*derived, field->GetName(), nullptr, + field->GetComments())); } - auto tag_enum = std::make_unique<AidlEnumDeclaration>(AIDL_LOCATION_HERE, "Tag", &enumerators, + auto derived = decl.GetLocation().ToDerivedLocation(); + AIDL_FATAL_IF(!derived, decl) + << "Failed to get a derived location. Is this not an external type?"; + auto tag_enum = std::make_unique<AidlEnumDeclaration>(*derived, "Tag", &enumerators, decl.GetPackage(), Comments{}); // Tag for @FixedSize union is limited to "byte" type so that it can be passed via FMQ with // with lower overhead.