[Mlir-commits] [mlir] [MLIR][TableGen] Fix ArrayRefParameter in struct format roundtrip (PR #189065)

[Fabian Mora]

================
@@ -899,10 +933,29 @@ void DefFormat::genParamsPrinter(ParamsDirective *el, FmtContext &ctx,
 
 void DefFormat::genStructPrinter(StructDirective *el, FmtContext &ctx,
                                  MethodBody &os) {
-  genCommaSeparatedPrinter(el->getElements(), ctx, os, [&](FormatElement *arg) {
-    ParameterElement *param = getEncapsulatedParameterElement(arg);
-    os << tgfmt("$_printer << \"$0 = \";\n", &ctx, param->getName());
-  });
+  ArrayRef<FormatElement *> elems = el->getElements();
+  // An `ArrayRefParameter` without a custom printer in a non-last struct
+  // position must be wrapped in `[...]` to avoid ambiguity with the
+  // struct-level comma separator. Track the element index via elemIdx, which is
+  // incremented once per element in the extraPost callback.
+  size_t elemIdx = 0;
+  genCommaSeparatedPrinter(
+      elems, ctx, os,
+      [&](FormatElement *arg) {
+        ParameterElement *param = getEncapsulatedParameterElement(arg);
+        os << tgfmt("$_printer << \"$0 = \";\n", &ctx, param->getName());
+        auto *paramEl = dyn_cast<ParameterElement>(arg);
+        if (paramEl && isUndelimitedArrayRefParam(paramEl) &&
+            elemIdx != elems.size() - 1)
+          os << tgfmt("$_printer << \"[\";\n", &ctx);
+      },
+      [&](FormatElement *arg) {
+        auto *paramEl = dyn_cast<ParameterElement>(arg);
+        if (paramEl && isUndelimitedArrayRefParam(paramEl) &&
+            elemIdx != elems.size() - 1)
+          os << tgfmt("$_printer << \"]\";\n", &ctx);
+        ++elemIdx;
+      });
----------------
fabianmcg wrote:

Should be impossible for these to be invoked with `elems.size()  == 0`, however, a comment saying `elems.size() - 1` is always safe, an assert or changing the condition so that we avoid `elems.size() - 1` would be great.

https://github.com/llvm/llvm-project/pull/189065