AAPT2: Fail on invalid id names in compiled xml

AAPT2 was not erroring on invalid resource ids created in layouts with
creation syntax. This change causes this to error suring compile.

Bug: 71394154
Test: aapt2_tests
Change-Id: Idf16fb4bd011ed2d65e8c48f7cba0429ead5a055

3 files changed, 36 insertions, 10 deletions

diff --git a/tools/aapt2/Diagnostics.h b/tools/aapt2/Diagnostics.h
index 50e8b33ab9b1..30deb5555b21 100644
--- a/tools/aapt2/Diagnostics.h
+++ b/tools/aapt2/Diagnostics.h
@@ -133,11 +133,19 @@ class SourcePathDiagnostics : public IDiagnostics {
   void Log(Level level, DiagMessageActual& actual_msg) override {
     actual_msg.source.path = source_.path;
     diag_->Log(level, actual_msg);
+    if (level == Level::Error) {
+      error = true;
+    }
+  }
+
+  bool HadError() {
+    return error;
   }
 
  private:
   Source source_;
   IDiagnostics* diag_;
+  bool error = false;
 
   DISALLOW_COPY_AND_ASSIGN(SourcePathDiagnostics);
 };
diff --git a/tools/aapt2/compile/XmlIdCollector.cpp b/tools/aapt2/compile/XmlIdCollector.cpp
index d61a15af0d85..2199d003bccb 100644
--- a/tools/aapt2/compile/XmlIdCollector.cpp
+++ b/tools/aapt2/compile/XmlIdCollector.cpp
@@ -21,6 +21,7 @@
 
 #include "ResourceUtils.h"
 #include "ResourceValues.h"
+#include "text/Unicode.h"
 #include "xml/XmlDom.h"
 
 namespace aapt {
@@ -35,8 +36,9 @@ struct IdCollector : public xml::Visitor {
  public:
   using xml::Visitor::Visit;
 
-  explicit IdCollector(std::vector<SourcedResourceName>* out_symbols)
-      : out_symbols_(out_symbols) {}
+  explicit IdCollector(std::vector<SourcedResourceName>* out_symbols,
+                       SourcePathDiagnostics* source_diag) : out_symbols_(out_symbols),
+                                                             source_diag_(source_diag) {}
 
   void Visit(xml::Element* element) override {
     for (xml::Attribute& attr : element->attributes) {
@@ -44,12 +46,16 @@ struct IdCollector : public xml::Visitor {
       bool create = false;
       if (ResourceUtils::ParseReference(attr.value, &name, &create, nullptr)) {
         if (create && name.type == ResourceType::kId) {
-          auto iter = std::lower_bound(out_symbols_->begin(),
-                                       out_symbols_->end(), name, cmp_name);
-          if (iter == out_symbols_->end() || iter->name != name) {
-            out_symbols_->insert(iter,
-                                 SourcedResourceName{name.ToResourceName(),
-                                                     element->line_number});
+          if (!text::IsValidResourceEntryName(name.entry)) {
+            source_diag_->Error(DiagMessage(element->line_number)
+                                   << "id '" << name << "' has an invalid entry name");
+          } else {
+            auto iter = std::lower_bound(out_symbols_->begin(),
+                                         out_symbols_->end(), name, cmp_name);
+            if (iter == out_symbols_->end() || iter->name != name) {
+              out_symbols_->insert(iter, SourcedResourceName{name.ToResourceName(),
+                                                             element->line_number});
+            }
           }
         }
       }
@@ -60,15 +66,17 @@ struct IdCollector : public xml::Visitor {
 
  private:
   std::vector<SourcedResourceName>* out_symbols_;
+  SourcePathDiagnostics* source_diag_;
 };
 
 }  // namespace
 
 bool XmlIdCollector::Consume(IAaptContext* context, xml::XmlResource* xmlRes) {
   xmlRes->file.exported_symbols.clear();
-  IdCollector collector(&xmlRes->file.exported_symbols);
+  SourcePathDiagnostics source_diag(xmlRes->file.source, context->GetDiagnostics());
+  IdCollector collector(&xmlRes->file.exported_symbols, &source_diag);
   xmlRes->root->Accept(&collector);
-  return true;
+  return !source_diag.HadError();
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/compile/XmlIdCollector_test.cpp b/tools/aapt2/compile/XmlIdCollector_test.cpp
index 98da56d03ae3..d49af3bf903c 100644
--- a/tools/aapt2/compile/XmlIdCollector_test.cpp
+++ b/tools/aapt2/compile/XmlIdCollector_test.cpp
@@ -64,4 +64,14 @@ TEST(XmlIdCollectorTest, DontCollectNonIds) {
   EXPECT_TRUE(doc->file.exported_symbols.empty());
 }
 
+TEST(XmlIdCollectorTest, ErrorOnInvalidIds) {
+  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+
+  std::unique_ptr<xml::XmlResource> doc =
+      test::BuildXmlDom("<View foo=\"@+id/foo$bar\"/>");
+
+  XmlIdCollector collector;
+  ASSERT_FALSE(collector.Consume(context.get(), doc.get()));
+}
+
 }  // namespace aapt