[MLIR] Check that the prop-dict dictionnary does not have extra unknown entries

[joker-eph]

At the moment we would just ignore them, which can be surprising if the user had a typo for a unit attribute for example.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Mehdi Amini (joker-eph)

Changes

At the moment we would just ignore them, which can be surprising if the user had a typo for a unit attribute for example.

---

Full diff: https://github.com/llvm/llvm-project/pull/138668.diff

2 Files Affected:

• (modified) mlir/test/IR/invalid-custom-print-parse.mlir (+7)
• (modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+15-1)

diff --git a/mlir/test/IR/invalid-custom-print-parse.mlir b/mlir/test/IR/invalid-custom-print-parse.mlir
index 00da145e35e0b..29e201c6f12f5 100644
--- a/mlir/test/IR/invalid-custom-print-parse.mlir
+++ b/mlir/test/IR/invalid-custom-print-parse.mlir
@@ -19,3 +19,10 @@ test.custom_dimension_list_attr dimension_list = [2x3]
 
 // expected-error @below {{expected attribute value}}
 test.optional_custom_attr foo
+
+// -----
+
+// expected-error @below {{Unknown key '"foo"' when parsing properties dictionnary}}
+test.op_with_enum_prop_attr_form <{value = 0 : i32, foo}>
+
+
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index a0d947fe8a0df..09550b7196a17 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -1300,6 +1300,11 @@ if (!dict) {
   emitError() << "expected DictionaryAttr to set properties";
   return ::mlir::failure();
 }
+// keep track of used keys in the input dictionnary to be able to error out
+// if there are some unknown ones.
+DenseSet<StringAttr> usedKeys;
+MLIRContext *ctx = dict.getContext();
+(void)ctx;
 )decl";
 
   // {0}: fromAttribute call
@@ -1310,6 +1315,7 @@ auto setFromAttr = [] (auto &propStorage, ::mlir::Attribute propAttr,
          ::llvm::function_ref<::mlir::InFlightDiagnostic()> emitError) -> ::mlir::LogicalResult {{
   {0};
 };
+usedKeys.insert(StringAttr::get(ctx, "{1}"));
 auto attr = dict.get("{1}");
 if (!attr && {2}) {{
   emitError() << "expected key entry for {1} in DictionaryAttr to set "
@@ -1357,6 +1363,7 @@ if (attr && ::mlir::failed(setFromAttr(prop.{1}, attr, emitError)))
     body << formatv(R"decl(
 auto &propStorage = prop.{0};
 auto attr = dict.get("{0}");
+usedKeys.insert(StringAttr::get(ctx, "{0}"));
 if (attr || /*isRequired=*/{1}) {{
   if (!attr) {{
     emitError() << "expected key entry for {0} in DictionaryAttr to set "
@@ -1374,7 +1381,14 @@ if (attr || /*isRequired=*/{1}) {{
 )decl",
                     namedAttr.name, isRequired);
   }
-  body << "return ::mlir::success();\n";
+  body << R"decl(
+for (NamedAttribute attr : dict) {
+  if (!usedKeys.contains(attr.getName()))
+    return emitError() << "Unknown key '" << attr.getName() <<
+        "' when parsing properties dictionnary";
+}
+return ::mlir::success();
+)decl";
 }
 
 void OperationFormat::genParser(Operator &op, OpClass &opClass) {

[krzysz00]

The tracking set doesn't need to be a set of attributes? We could use a llvm::StringSet or such here?

That aside, good idea!

[joker-eph]

The tracking set doesn't need to be a set of attributes? We could use a llvm::StringSet or such here?

The contains query will be a pointer comparison with a set of attributes. The llvm::StringSet is owning and will make a deep copy of the string internally.

[jpienaar]

Would previously these just have been dropped?

[jpienaar]

Looks good with River's comment addressed

[krzysz00]

Approved with changes noted in other comments

[joker-eph]

Would previously these just have been dropped?

Yes! :(