[pseudo] Add grammar annotations support.

[hokein]

Summary

• Add annotation handling ([key=value]) in the BNF grammar parser;
• Define and setup the API in the grammar for attributes;
• Implement a builtin guard for two simple c++ contexual-override/final use cases;

[hokein]

This patch might cover too many things (at least we could split the guide/glr implementation bit), but want to give you an overview of the picture first.

[hokein]

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

90

new names are welcome. Attribute is the name I came up with (I think it is clearer than the original Hook),

97

I'm not quite happy with using the value as the ID, I think we can encode the Key into the ID as well (ID := Key | Value).

Similar to the generated enum name, currently we just use the name of Value (Override), it will be more confusing when we add more keys/values, one idea is to add key as well (GuardOverride etc?).

[sammccall]

Nice!

This has tests for the parsing-the-attribute bits, but I think we're missing tests for the actual guards added.

clang-tools-extra/pseudo/include/clang-pseudo/GLR.h

114 ↗

(On Diff #432538)

this signature seems a little off to me.

Guard logic is identified by a guard ID and we look it up, but we're not passing in the guard ID for some reason. Instead we pass in the rule ID, which this function uses to look up the guard ID again. Why not just pass the guard ID?

That said, it's a bit surprising that we have the rules declare separate guard rules, but then they're all implemented by one function. A map of functions seems more natural. (but not a performance advantage I guess)

Naming: it's confusing that this umbrella function is called "guard", but a specific rule like "guard=Override" is also called "guard". I'd either call this a GuardTester or bypass the issue by making this a DenseMap whose values are Guards.

Altogether I might write this as:

using Guard = llvm::function_ref<bool(llvm::ArrayRef<const ForestNode *> RHS, const TokenStream &, const Grammar &)>;
...
const DenseMap<AttributeID, Guard> &Guards;

129 ↗

(On Diff #432538)

either a reference, or a lightweight reference type like function_ref, for consistency with other fields?

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

22

nit: first "guard" should be "key"?

90

I don't understand why we need AttributeKey, and to parse things into it, rather than just using StringRef.

(Also here you've put it in the header, but it's not used in any interfaces)

97

Packing all the possible values into a single unstructured enum is weird, but the problem (if it's a problem) is _redundancy_ and packing the attribute name in there as well just makes that worse.

If we want to fix this, I think a sounder fix is something like:

// Grammar.h
using AttributeEnum = uint8_t;
struct Rule { ...; AttributeEnum Guard = 0; }
// CXX.h
enum class GuardID : AttributeEnum { Override = 1; };
enum class RecoveryID : AttributeEnum { Parens = 1; };

i.e. we keep track of the unique values per attribute name and emit an enum for each attribute. This probably means emitting the actual enum definitions in tablegen, rather than a generic .inc file.

(But I'm also fine with not fixing it at this point, and keeping one flat enum for the values)

98

I agree HookID is an unfortunately vague name, but I did consider and reject AttributeID which I think is actively confusing.
In the plain-english meaning, "guard" is an attribute, and "override" is its value. So assigning "override" an "attribute ID" is very confusing.

The difficulty of naming this is that we can't refer to the semantics (e.g. "Guard") as we want a single namespace for all of them, and all they have in common is syntax. However referring to "attribute values" makes for confusing internal data structures, as we don't model the whole attribute generically.

So we're left with names that suggest "this is some kind of anchor to attach custom code to". Maybe ExtensionID is a little clearer than HookID?

100

If we're going to use implicit bool conversions, we should not pretend this is some arbitrary value and drop the constant I think.

104

if this optional and rarely set, I think it's should be a constructor parameter - this constructor could become unwieldy.

It also forces you to process the attributes before creating the Rule, and reading the code doing it afterwards seems more natural.

clang-tools-extra/pseudo/include/clang-pseudo/cxx/CXX.h

39 ↗

(On Diff #432538)

this pattern is fragile. (and we're going to end up adding rules, too)

I'd suggest either:

• generate a separate file for each enum
• generate a file containing the full definiton of all the enums (we're not using the generality)
• add the fallback #defines to the .inc file so the caller doesn't have to provide them all

63 ↗

(On Diff #432538)

"guard" looks like a verb here. If we're going to use it as both noun and verb, the relationship between the two needs to match the metaphor (in this case, it needs to be the guard who is doing the guarding) and it's not clear that it is.

I think safest to stick to const Guard &getGuard() or so.

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

47

This patch takes this function from 80->120 lines, and it's starting to feel unwieldy.

I think moving building the UniqueNonterminals/SymbolIDs + the new maps into a function (which returns a struct of SymbolIds and AttrValueIDs) would make this more manageable.

51

unused

52

why ""?

60

AttrValueIDs never seems to get populated, so I'm not sure this works.

104

This doesn't seem worth diagnosing to me - it seems stylistically weird but not really a problem to put a guard wherever.

104

maybe another function to pull out here?
applyAttribute(StringRef Name, StringRef Value, AttributeID ValueID, Rule&, Element&)

227

Again, I don't think we should be aiming to provide nice diagnostics for each possible way we could get this wrong - the grammar here is part of the pseudo-parser, not user input.

259

(This seems like a condition we can diagnose when applying the attribute, instead of needing to do it eagerly here)

clang-tools-extra/pseudo/unittests/GLRTest.cpp

161 ↗

(On Diff #432538)

These empty streams aren't valid (in multiple ways: they're not finalized, and they don't contain the tokens that the nodes refer to).

Maybe add a tokenStreamForTest(StringLiteral) function somewhere? Safe because guaranteed to be null-terminated and have sufficient lifetime...

[hokein]

address the grammar part comments:

• Rename to ExtensionID;
• Simplify the BNF parsing bit;

[hokein]

I addressed comments on the grammar part, (the remaining GLR, pseudo_gen parts are not covered yet), I think it would be better them into different patches, so that we can land the grammar bit first, then start doing the error recovery and guard implementation in parallel.

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

104

I suppose you mean we should drop the Guard parameter in the constructor, yeah, that sounds good to me, and simplifies the BNF parsing bit.

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

52

This was for the sentinel default 0 attribute value.

60

oops, this part of code wasn't fully cleaned up. AttrValueIDs is not needed indeed.

104

sounds good, move the extension bits to a dedicate applyExtension.

104

This doesn't seem worth diagnosing to me - it seems stylistically weird but not really a problem to put a guard wherever.

It is valid in syntax level, but not in semantic level, I think this is confusing -- the bnf parser doesn't emit any warning on this usage, just silently ignore them (grammar writer might think the grammar is correct, guard should work even putting it in the middle of the rules).

On the other hand, as you mentioned, grammar file is not user-faced, and we're the current authors, it seems ok to not do it (in favour of simplicity) right now (we might need to reconsider improving it there are new grammar contributors).

227

fair enough, but we should keep minimal critical diagnostics at least to avoid shooting ourself in the foot.

[hokein]

remove a left-out change.

[sammccall]

Thanks, this looks better.
Sorry about confusion with the names - I think annotation is a great name, and we should only use "extension" for the narrower "opaque thing that an annotation value refers to".
Happy to chat about this offline if you like

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

22

Hmm, I think we misunderstood each other...

I think annotations is a good name for the syntactic [foo=bar] bit. I think the comment here is now harder to follow, and would prefer the old version.

It's specifically "bar" which indicates some identifier whose semantics will be provided externally, so "bar" is a reference to an extension.

If you want to mention the concept of an extension, then maybe at the end...
Each unique annotation value creates an extension point. The Override guard is implemented in CXX.cpp by binding the ExtensionID for Override to a C++ function that performs the check.

88

"an extension uniquely identifies an extension" is a tautology.

An extension is a piece of native code specific to a grammar that modifies the behavior of annotated rules.
One ExtensionID is assigned for each unique attribute value (all attributes share a namespace).

90

I don't think this last sentence is useful as-is, it looks important but it's not clear who it constrains.
Also "attribute" rather than "key", an attribute is a thing that objects have, a key is a thing maps have.

216

attribute values, or extension names

(attributes are syntactic and have string values, extensions are semantic and can be referred to by names).

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

106

doing this outside the loop above means this function has to handle all specs/rules and work out how they correspond, which is fragile.

Instead can we do it inside the loop, and just handle a single attribute at a time?

for (const auto &Spec : Specs) {
  // ... create rule
  for (const auto &Attribute : Spec.Attributes)
    applyAttribute(Attribute, T->Rules.back());
}

184

rather attributes :-(

244

For simplicity we could also drop this loop. If we ever need to specify two attributes on the same token, a := b [foo] [bar] works fine

247

If we drop this line, then [foo] will be equivalent to [foo=], i.e. attribute is present and points at the empty string. This seems reasonable to me, and is a good syntax for boolean attributes (where [foo] denotes set and no attribute denotes unset)

clang-tools-extra/pseudo/unittests/GrammarTest.cpp

104

if you like you could add 3 more rules, with no annotation, guard=override, guard=somethingelse
and verify they're zero, equal, nonequal respectively

[hokein]

• bring back the attribute concept, narrow down the ExtensionID scope (only used for semantic);
• loose and simplify the BNF annotations parsing; ([] only allows single attribute, attribute without value are acceptable);
• address other comments;

[hokein]

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

22

oops, I misunderstood your "extensionID" comment completely (sorry). Bring it back now.

216

renamed to attribute values.

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

247

This syntax for bool-type attributes seems good to me.
Note that there is a subtle difference with the string-type attributes, where we use the empty string as the sentinel attribute value (denote unset) whose ExtensionID is 0. I think it should not be a big issue (we only handle them is in the bnf parsing time, and propagate field values in Rule).

[sammccall]

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

91

nit: I'd drop this sentence, as using it to index into a table is only ever used to produce debug representations I think - usually we use ExtensionID as an index into a map

218

looking at this again, ExtensionNames seems clearer as ExtensionNames[ExtID] seems more obvious that the kinds agree. But up to you

[hokein]

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

218

I considered extensionNames, but decided to use AttributeValues. AttributeValues corresponds to the syntactic grammar [key=value] where we call value as the attribute value, while using ExtensionNames is less clearer. And yeah AttributeValues[ExtID] is somehow confusing, but I think it is fine, as it is just print code.

[uabelho]

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

235

I get a warning/error on this line with this commit:

13:31:17 ../../clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp:235:36: error: missing field 'Attributes' initializer [-Werror,-Wmissing-field-initializers]
13:31:17       Out.Sequence.push_back({Chunk});
13:31:17                                    ^
13:31:17 1 error generated.

I see the warning when compiling with clang 8.0.

[hokein]

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

235

sorry (I don't this warning enabled), should be fixed in 9ce232fba99c47c3246f06fcbe37c24b9d90585f.

[uabelho]

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

235

Thanks!

clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h

 //    translation-unit := declaration-seq_opt
 //    declaration-seq := declaration
 //    declaration-seq := declaration-seq declaration
 //
 //  A grammar formally describes a language, and it is constructed by a set of
 //  production rules. A rule is of BNF form (AAA := BBB CCC). A symbol is either
 //  nonterminal or terminal, identified by a SymbolID.
 //
+//  Annotations are supported in a syntax form of [key=value]. They specify
+//  attributes which are associated with either a grammar symbol (on the
+//  right-hand side of the symbol) or a grammar rule (at the end of the rule
+//  body).
+//  Attributes provide a way to inject custom code into the GLR parser. Each
+//  unique attribute value creates an extension point (identified by ExtensionID
+//  ), and an extension point corresponds to a piece of native code. For
+//  example, C++ grammar has a rule:
+//
+//    contextual-override := IDENTIFIER [guard=Override]
+//
+//  GLR parser only conducts the reduction of the rule if the IDENTIFIER
+//  content is `override`. This Override guard is implemented in CXX.cpp by
+//  binding the ExtensionID for the `Override` value to a specific C++ function
+//  that performs the check.
+//
 //  Notions about the BNF grammar:
 //  - "_" is the start symbol of the augmented grammar;
 //  - single-line comment is supported, starting with a #
 //  - A rule describes how a nonterminal (left side of :=) is constructed, and
 //    it is *per line* in the grammar file
 //  - Terminals (also called tokens) correspond to the clang::TokenKind; they
 //    are written in the grammar like "IDENTIFIER", "USING", "+"
 //  - Nonterminals are specified with "lower-case" names in the grammar; they
inline tok::TokenKind symbolToToken(SymbolID SID) {
   SID &= ~TokenFlag;
   assert(SID < NumTerminals);
   return static_cast<tok::TokenKind>(SID);
 }
 inline SymbolID tokenSymbol(tok::TokenKind TK) {
   return TokenFlag | static_cast<SymbolID>(TK);
 }
 
+// An extension is a piece of native code specific to a grammar that modifies
+// the behavior of annotated rules. One ExtensionID is assigned for each unique
+// attribute value (all attributes share a namespace).
+using ExtensionID = uint16_t;
+
 // A RuleID uniquely identifies a production rule in a grammar.
 // It is an index into a table of rules.
 using RuleID = uint16_t;
 // There are maximum 2^12 rules.
 static constexpr unsigned RuleBits = 12;
 
 // Represent a production rule in the grammar, e.g.
 //   expression := a b c
 //   ^Target       ^Sequence
 struct Rule {
   Rule(SymbolID Target, llvm::ArrayRef<SymbolID> Seq);
 
   // We occupy 4 bits for the sequence, in theory, it can be at most 2^4 tokens
   // long, however, we're stricter in order to reduce the size, we limit the max
   // length to 9 (this is the longest sequence in cxx grammar).
   static constexpr unsigned SizeBits = 4;
   static constexpr unsigned MaxElements = 9;
   static_assert(MaxElements <= (1 << SizeBits), "Exceeds the maximum limit");
   static_assert(SizeBits + SymbolBits <= 16,
                 "Must be able to store symbol ID + size efficiently");
 
   // 16 bits for target symbol and size of sequence:
   // SymbolID : 12 | Size : 4
   SymbolID Target : SymbolBits;
   uint8_t Size : SizeBits; // Size of the Sequence
   SymbolID Sequence[MaxElements];
 
+  // A guard extension controls whether a reduction of a rule will be conducted
+  // by the GLR parser.
+  // 0 is sentinel unset extension ID, indicating there is no guard extension
+  // being set for this rule.
+  ExtensionID Guard = 0;
+
   llvm::ArrayRef<SymbolID> seq() const {
     return llvm::ArrayRef<SymbolID>(Sequence, Size);
   }
   friend bool operator==(const Rule &L, const Rule &R) {
-    return L.Target == R.Target && L.seq() == R.seq();
+    return L.Target == R.Target && L.seq() == R.seq() && L.Guard == R.Guard;
   }
 };
 
 struct GrammarTable;
 
 // Grammar that describes a programming language, e.g. C++. It represents the
 // contents of the specified grammar.
 // It is a building block for constructing a table-based parser.
struct GrammarTable {
   // prerequisite reductions are reached before dependent ones).
   std::vector<Rule> Rules;
   // A table of terminals (aka tokens). It corresponds to the clang::Token.
   // clang::tok::TokenKind is the index of the table.
   llvm::ArrayRef<std::string> Terminals;
   // A table of nonterminals, sorted by name.
   // SymbolID is the index of the table.
   std::vector<Nonterminal> Nonterminals;
+  // A table of attribute values, sorted by name.
+  // ExtensionID is the index of the table.
+  std::vector<std::string> AttributeValues;
 };
 
 } // namespace pseudo
 } // namespace clang
 
 #endif // CLANG_PSEUDO_GRAMMAR_H

clang-tools-extra/pseudo/lib/grammar/Grammar.cpp

 
 std::string Grammar::dumpRule(RuleID RID) const {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   const Rule &R = T->Rules[RID];
   OS << symbolName(R.Target) << " :=";
   for (SymbolID SID : R.seq())
     OS << " " << symbolName(SID);
+  if (R.Guard)
+    OS << " [guard=" << T->AttributeValues[R.Guard] << "]";
   return Result;
 }
 
 std::string Grammar::dumpRules(SymbolID SID) const {
   assert(isNonterminal(SID));
   std::string Result;
   const auto &Range = T->Nonterminals[SID].RuleRange;
   for (RuleID RID = Range.Start; RID < Range.End; ++RID)

clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp

assert(llvm::all_of(Specs,
                               R.Sequence, [](const RuleSpec::Element &E) {
                                 return !E.Symbol.endswith(OptSuffix);
                               });
                         }) &&
            "Optional symbols should be eliminated!");
 
     auto T = std::make_unique<GrammarTable>();
 
     // Assemble the name->ID and ID->nonterminal name maps.
     llvm::DenseSet<llvm::StringRef> UniqueNonterminals;
     llvm::DenseMap<llvm::StringRef, SymbolID> SymbolIds;
+
+    llvm::DenseSet<llvm::StringRef> UniqueAttributeValues;
+
     for (uint16_t I = 0; I < NumTerminals; ++I)
       SymbolIds.try_emplace(T->Terminals[I], tokenSymbol(tok::TokenKind(I)));
     auto Consider = [&](llvm::StringRef Name) {
       if (!SymbolIds.count(Name))
         UniqueNonterminals.insert(Name);
     };
     for (const auto &Spec : Specs) {
       Consider(Spec.Target);
-      for (const RuleSpec::Element &Elt : Spec.Sequence)
+      for (const RuleSpec::Element &Elt : Spec.Sequence) {
         Consider(Elt.Symbol);
+        for (const auto& KV : Elt.Attributes)
+           UniqueAttributeValues.insert(KV.second);
+      }
     }
     llvm::for_each(UniqueNonterminals, [&T](llvm::StringRef Name) {
       T->Nonterminals.emplace_back();
       T->Nonterminals.back().Name = Name.str();
     });
     assert(T->Nonterminals.size() < (1 << (SymbolBits - 1)) &&
            "Too many nonterminals to fit in SymbolID bits!");
     llvm::sort(T->Nonterminals, [](const GrammarTable::Nonterminal &L,
                                    const GrammarTable::Nonterminal &R) {
       return L.Name < R.Name;
     });
+    // Add an empty string for the corresponding sentinel unset attribute.
+    T->AttributeValues.push_back("");
+    llvm::for_each(UniqueAttributeValues, [&T](llvm::StringRef Name) {
+      T->AttributeValues.emplace_back();
+      T->AttributeValues.back() = Name.str();
+    });
+    llvm::sort(T->AttributeValues);
+    assert(T->AttributeValues.front() == "");
+
     // Build name -> ID maps for nonterminals.
     for (SymbolID SID = 0; SID < T->Nonterminals.size(); ++SID)
       SymbolIds.try_emplace(T->Nonterminals[SID].Name, SID);
 
     // Convert the rules.
     T->Rules.reserve(Specs.size());
     std::vector<SymbolID> Symbols;
     auto Lookup = [SymbolIds](llvm::StringRef Name) {
       auto It = SymbolIds.find(Name);
       assert(It != SymbolIds.end() && "Didn't find the symbol in SymbolIds!");
       return It->second;
     };
     for (const auto &Spec : Specs) {
       assert(Spec.Sequence.size() <= Rule::MaxElements);
       Symbols.clear();
       for (const RuleSpec::Element &Elt : Spec.Sequence)
         Symbols.push_back(Lookup(Elt.Symbol));
       T->Rules.push_back(Rule(Lookup(Spec.Target), Symbols));
+      applyAttributes(Spec, *T, T->Rules.back());
     }
+
     assert(T->Rules.size() < (1 << RuleBits) &&
            "Too many rules to fit in RuleID bits!");
     const auto &SymbolOrder = getTopologicalOrder(T.get());
     llvm::stable_sort(
         T->Rules, [&SymbolOrder](const Rule &Left, const Rule &Right) {
           // Sorted by the topological order of the nonterminal Target.
           return SymbolOrder[Left.Target] < SymbolOrder[Right.Target];
         });
public:
   }
 
 private:
   // Text representation of a BNF grammar rule.
   struct RuleSpec {
     llvm::StringRef Target;
     struct Element {
       llvm::StringRef Symbol; // Name of the symbol
+      // Attributes that are associated to the sequence symbol or rule.
+      std::vector<std::pair<llvm::StringRef/*Key*/, llvm::StringRef/*Value*/>>
+          Attributes;
     };
     std::vector<Element> Sequence;
 
     std::string toString() const {
       std::vector<llvm::StringRef> Body;
       for (const auto &E : Sequence)
         Body.push_back(E.Symbol);
       return llvm::formatv("{0} := {1}", Target, llvm::join(Body, " "));
bool parseLine(llvm::StringRef Line, RuleSpec &Out) {
     }
 
     Out.Target = Parts.first.trim();
     Out.Sequence.clear();
     for (llvm::StringRef Chunk : llvm::split(Parts.second, ' ')) {
       Chunk = Chunk.trim();
       if (Chunk.empty())
         continue; // skip empty
+      if (Chunk.startswith("[") && Chunk.endswith("]")) {
+        if (Out.Sequence.empty())
+          continue;
+
+        parseAttributes(Chunk, Out.Sequence.back().Attributes);
+        continue;
+      }
 
       Out.Sequence.push_back({Chunk});
     }
     return true;
+  }
+
+  bool parseAttributes(
+      llvm::StringRef Content,
+      std::vector<std::pair<llvm::StringRef, llvm::StringRef>> &Out) {
+    assert(Content.startswith("[") && Content.endswith("]"));
+    auto KV = Content.drop_front().drop_back().split('=');
+    Out.push_back({KV.first, KV.second.trim()});
+
+    return true;
+  }
+  // Apply the parsed extensions (stored in RuleSpec) to the grammar Rule.
+  void applyAttributes(const RuleSpec& Spec, const GrammarTable& T, Rule& R) {
+    auto LookupExtensionID = [&T](llvm::StringRef Name) {
+      const auto It = llvm::partition_point(
+          T.AttributeValues, [&](llvm::StringRef X) { return X < Name; });
+      assert(It != T.AttributeValues.end() && *It == Name &&
+             "Didn't find the attribute in AttrValues!");
+      return It - T.AttributeValues.begin();
     };
+    for (const auto &KV : Spec.Sequence.back().Attributes) {
+      if (KV.first == "guard") {
+        R.Guard = LookupExtensionID(KV.second);
+        continue;
+      }
+      Diagnostics.push_back(
+          llvm::formatv("Unknown attribute '{0}'", KV.first).str());
+    }
+  }
 
   // Inlines all _opt symbols.
   // For example, a rule E := id +_opt id, after elimination, we have two
   // equivalent rules:
   //   1) E := id + id
   //   2) E := id id
   std::vector<RuleSpec> eliminateOptional(llvm::ArrayRef<RuleSpec> Input) {
     std::vector<RuleSpec> Results;

clang-tools-extra/pseudo/unittests/GrammarTest.cpp

TEST_F(GrammarTest, RuleIDSorted) {
   )bnf");
   ASSERT_TRUE(Diags.empty());
 
   EXPECT_LT(ruleFor("z"), ruleFor("y"));
   EXPECT_LT(ruleFor("y"), ruleFor("x"));
   EXPECT_LT(ruleFor("x"), ruleFor("_"));
 }
 
+TEST_F(GrammarTest, Annotation) {
+  build(R"bnf(
+    _ := x
+
+    x := y [guard=value]
+    y := IDENTIFIER [guard=final]
+
+  )bnf");
+  ASSERT_TRUE(Diags.empty());
+  EXPECT_EQ(G->lookupRule(ruleFor("_")).Guard, 0);
+  EXPECT_GT(G->lookupRule(ruleFor("x")).Guard, 0);
+  EXPECT_GT(G->lookupRule(ruleFor("y")).Guard, 0);
+  EXPECT_NE(G->lookupRule(ruleFor("x")).Guard,
+            G->lookupRule(ruleFor("y")).Guard);
+}
+
 TEST_F(GrammarTest, Diagnostics) {
   build(R"cpp(
     _ := ,_opt
     _ := undefined-sym
     null :=
     _ := IDENFIFIE # a typo of the terminal IDENFITIER
 
     invalid
     # cycle
     a := b
     b := a
+
+    _ := IDENTIFIER [unknown=value]
   )cpp");
 
   EXPECT_EQ(G->underscore(), id("_"));
   EXPECT_THAT(Diags, UnorderedElementsAre(
                          "Rule '_ := ,_opt' has a nullable RHS",
                          "Rule 'null := ' has a nullable RHS",
                          "No rules for nonterminal: undefined-sym",
                          "Failed to parse 'invalid': no separator :=",
                          "Token-like name IDENFIFIE is used as a nonterminal",
                          "No rules for nonterminal: IDENFIFIE",
-                         "The grammar contains a cycle involving symbol a"));
+                         "The grammar contains a cycle involving symbol a",
+                         "Unknown attribute 'unknown'"));
 }
 
 TEST_F(GrammarTest, FirstAndFollowSets) {
   build(
       R"bnf(
 _ := expr
 expr := expr - term
 expr := term