[MLIR] Fix parseFunctionLikeOp() to fail parsing empty regions

- Change parseOptionalRegion to return an OptionalParseResult.
- Change parseFunctionLikeOp() to fail parsing if the function body was parsed but was
  empty.
- See https://llvm.discourse.group/t/funcop-parsing-bug/2164

Differential Revision: https://reviews.llvm.org/D91886

Author: jurahul

flang/include/flang/Optimizer/Dialect/FIROps.td

@@ -2856,7 +2856,8 @@ def fir_DispatchTableOp : fir_Op<"dispatch_table",
 
     // Parse the optional table body.
     mlir::Region *body = result.addRegion();
-    if (parser.parseOptionalRegion(*body, llvm::None, llvm::None))
+    OptionalParseResult parseResult = parser.parseOptionalRegion(*body);
+    if (parseResult.hasValue() && failed(*parseResult))
       return mlir::failure();
 
     ensureTerminator(*body, parser.getBuilder(), result.location);

mlir/include/mlir/IR/OpImplementation.h

@@ -646,23 +646,23 @@ class OpAsmParser {
   // Region Parsing
   //===--------------------------------------------------------------------===//
 
-  /// Parses a region. Any parsed blocks are appended to "region" and must be
+  /// Parses a region. Any parsed blocks are appended to 'region' and must be
   /// moved to the op regions after the op is created. The first block of the
-  /// region takes "arguments" of types "argTypes". If "enableNameShadowing" is
+  /// region takes 'arguments' of types 'argTypes'. If 'enableNameShadowing' is
   /// set to true, the argument names are allowed to shadow the names of other
-  /// existing SSA values defined above the region scope. "enableNameShadowing"
+  /// existing SSA values defined above the region scope. 'enableNameShadowing'
   /// can only be set to true for regions attached to operations that are
-  /// "IsolatedFromAbove".
+  /// 'IsolatedFromAbove.
   virtual ParseResult parseRegion(Region &region,
                                   ArrayRef<OperandType> arguments = {},
                                   ArrayRef<Type> argTypes = {},
                                   bool enableNameShadowing = false) = 0;
 
   /// Parses a region if present.
-  virtual ParseResult parseOptionalRegion(Region &region,
-                                          ArrayRef<OperandType> arguments = {},
-                                          ArrayRef<Type> argTypes = {},
-                                          bool enableNameShadowing = false) = 0;
+  virtual OptionalParseResult
+  parseOptionalRegion(Region &region, ArrayRef<OperandType> arguments = {},
+                      ArrayRef<Type> argTypes = {},
+                      bool enableNameShadowing = false) = 0;
 
   /// Parses a region if present. If the region is present, a new region is
   /// allocated and placed in `region`. If no region is present or on failure,

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

@@ -1187,9 +1187,12 @@ static ParseResult parseGlobalOp(OpAsmParser &parser, OperationState &result) {
       return parser.emitError(parser.getNameLoc(),
                               "type can only be omitted for string globals");
     }
-  } else if (parser.parseOptionalRegion(initRegion, /*arguments=*/{},
-                                        /*argTypes=*/{})) {
-    return failure();
+  } else {
+    OptionalParseResult parseResult =
+        parser.parseOptionalRegion(initRegion, /*arguments=*/{},
+                                   /*argTypes=*/{});
+    if (parseResult.hasValue() && failed(*parseResult))
+      return failure();
   }
 
   result.addAttribute("type", TypeAttr::get(types[0]));
@@ -1398,8 +1401,9 @@ static ParseResult parseLLVMFuncOp(OpAsmParser &parser,
                              resultAttrs);
 
   auto *body = result.addRegion();
-  return parser.parseOptionalRegion(
+  OptionalParseResult parseResult = parser.parseOptionalRegion(
       *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes);
+  return failure(parseResult.hasValue() && failed(*parseResult));
 }
 
 // Print the LLVMFuncOp. Collects argument and result types and passes them to

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp

@@ -1754,8 +1754,9 @@ static ParseResult parseFuncOp(OpAsmParser &parser, OperationState &state) {
 
   // Parse the optional function body.
   auto *body = state.addRegion();
-  return parser.parseOptionalRegion(
+  OptionalParseResult result = parser.parseOptionalRegion(
       *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes);
+  return failure(result.hasValue() && failed(*result));
 }
 
 static void print(spirv::FuncOp fnOp, OpAsmPrinter &printer) {

mlir/lib/IR/FunctionImplementation.cpp

@@ -204,10 +204,21 @@ mlir::impl::parseFunctionLikeOp(OpAsmParser &parser, OperationState &result,
   assert(resultAttrs.size() == resultTypes.size());
   addArgAndResultAttrs(builder, result, argAttrs, resultAttrs);
 
-  // Parse the optional function body.
+  // Parse the optional function body. The printer will not print the body if
+  // its empty, so disallow parsing of empty body in the parser.
   auto *body = result.addRegion();
-  return parser.parseOptionalRegion(
-      *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes);
+  llvm::SMLoc loc = parser.getCurrentLocation();
+  OptionalParseResult parseResult = parser.parseOptionalRegion(
+      *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes,
+      /*enableNameShadowing=*/false);
+  if (parseResult.hasValue()) {
+    if (failed(*parseResult))
+      return failure();
+    // Function body was parsed, make sure its not empty.
+    if (body->empty())
+      return parser.emitError(loc, "expected non-empty function body");
+  }
+  return success();
 }
 
 // Print a function result list.

mlir/lib/Parser/Parser.cpp

@@ -1433,12 +1433,12 @@ class CustomOpAsmParser : public OpAsmParser {
   }
 
   /// Parses a region if present.
-  ParseResult parseOptionalRegion(Region &region,
-                                  ArrayRef<OperandType> arguments,
-                                  ArrayRef<Type> argTypes,
-                                  bool enableNameShadowing) override {
+  OptionalParseResult parseOptionalRegion(Region &region,
+                                          ArrayRef<OperandType> arguments,
+                                          ArrayRef<Type> argTypes,
+                                          bool enableNameShadowing) override {
     if (parser.getToken().isNot(Token::l_brace))
-      return success();
+      return llvm::None;
     return parseRegion(region, arguments, argTypes, enableNameShadowing);
   }
 

mlir/test/Dialect/SCF/canonicalize.mlir

@@ -86,7 +86,7 @@ func @nested_unused(%cond1: i1, %cond2: i1) -> (index) {
 
 // -----
 
-func private @side_effect() {}
+func private @side_effect()
 func @all_unused(%cond: i1) {
   %c0 = constant 0 : index
   %c1 = constant 1 : index

mlir/test/IR/invalid.mlir

@@ -1572,3 +1572,7 @@ func @invalid_region_dominance_with_dominance_free_regions() {
   }
   return
 }
+
+// -----
+
+func @foo() {} // expected-error {{expected non-empty function body}}

mlir/test/IR/traits.mlir

@@ -344,12 +344,10 @@ func @failedSingleBlockImplicitTerminator_missing_terminator() {
 
 // Test that operation with the SymbolTable Trait define a new symbol scope.
 "test.symbol_scope"() ({
-  func private @foo() {
-  }
+  func private @foo()
   "test.finish" () : () -> ()
 }) : () -> ()
-func private @foo() {
-}
+func private @foo() 
 
 // -----
 

mlir/tools/mlir-tblgen/OpFormatGen.cpp

@@ -652,8 +652,11 @@ const char *regionListEnsureTerminatorParserCode = R"(
 ///
 /// {0}: The name of the region.
 const char *optionalRegionParserCode = R"(
-  if (parser.parseOptionalRegion(*{0}Region))
-    return ::mlir::failure();
+  {
+     auto parseResult = parser.parseOptionalRegion(*{0}Region);
+     if (parseResult.hasValue() && failed(*parseResult))
+       return ::mlir::failure();
+  }
 )";
 
 /// The code snippet used to generate a parser call for a region.