[mlir] Fix two AttributeParser aborts

[jpienaar]

Summary

Reproducers that resulted in triggering the following asserts

mlir::NamedAttribute::NamedAttribute(mlir::StringAttr, mlir::Attribute)
mlir/lib/IR/Attributes.cpp:29:3
consumeToken
mlir/lib/Parser/Parser.h:126

[Mogball]

mlir/lib/Parser/AttributeParser.cpp

274–290

This should be moved before uses of the empty name.

mlir/test/IR/invalid.mlir

1649

Wasn't it the convention to note have these in-line? @rriddle

1653

https://reviews.llvm.org/D122295

[jpienaar]

Rebase post EOF fix landing & move valid identifier check earlier before first
string use.

[jpienaar]

mlir/test/IR/invalid.mlir

1649

This file is about 50-50 (84 out of line, 100 in-line), but I'll make it out of line and could make it consistent in pure NFC follow up.

[rriddle]

mlir/lib/Parser/AttributeParser.cpp

172

Why is this necessary? Which crash is this hitting?

277

Do we not have an empty method?

mlir/test/IR/invalid.mlir

1649

Yeah, we should clean these up.

[Mogball]

mlir/lib/Parser/AttributeParser.cpp

172

":<eof>" is crashing here because it reads past EOF.

277

we do not

[jpienaar]

mlir/lib/Parser/AttributeParser.cpp

172

Yes, consumeToken inside crashes. I add error too as that would trigger assert there too. I'm not 100% sure what cases the reset & consume here wants to support/won't error on so I can adjust this if there is something better fitting.

mlir/lib/Parser/AttributeParser.cpp

case Token::at_identifier: {
 
     // Parse any nested references.
     std::vector<FlatSymbolRefAttr> nestedRefs;
     while (getToken().is(Token::colon)) {
       // Check for the '::' prefix.
       const char *curPointer = getToken().getLoc().getPointer();
       consumeToken(Token::colon);
       if (!consumeIf(Token::colon)) {
+        if (getToken().isNot(Token::eof, Token::error)) {
           state.lex.resetPointer(curPointer);
           consumeToken();
+        }
         break;
       }
       // Parse the reference itself.
       auto curLoc = getToken().getLoc();
       if (getToken().isNot(Token::at_identifier)) {
         emitError(curLoc, "expected nested symbol reference identifier");
         return Attribute();
       }
ParseResult Parser::parseAttributeDict(NamedAttrList &attributes) {
   auto parseElt = [&]() -> ParseResult {
     // The name of an attribute can either be a bare identifier, or a string.
     Optional<StringAttr> nameId;
     if (getToken().is(Token::string))
       nameId = builder.getStringAttr(getToken().getStringValue());
     else if (getToken().isAny(Token::bare_identifier, Token::inttype) ||
              getToken().isKeyword())
       nameId = builder.getStringAttr(getTokenSpelling());
     else
       return emitError("expected attribute name");
+
+    if (nameId->size() == 0)
+      return emitError("expected valid attribute name");
+
     if (!seenKeys.insert(*nameId).second)
       return emitError("duplicate key '")
              << nameId->getValue() << "' in dictionary attribute";
     consumeToken();
 
     // Lazy load a dialect in the context if there is a possible namespace.
     auto splitName = nameId->strref().split('.');
     if (!splitName.second.empty())
       getContext()->getOrLoadDialect(splitName.first);
 
     // Try to parse the '=' for the attribute value.
     if (!consumeIf(Token::equal)) {
       // If there is no '=', we treat this as a unit attribute.
       attributes.push_back({*nameId, builder.getUnitAttr()});
       return success();
     }
 
     auto attr = parseAttribute();
     if (!attr)

mlir/test/IR/invalid.mlir

test.graph_region {
     }) : () -> ()
   }
   return
 }
 
 // -----
 
 func @foo() {} // expected-error {{expected non-empty function body}}
+
+// -----
+
+// expected-error@+1 {{expected valid attribute name}}
+"t"(){""}
+
+// -----
+
+// expected-error@+2 {{expected ']'}}
+"f"() { b = [@m:
+