[Support] Move ParseResult from OpDefinition.h to LogicalResult.h

[lattner]

Summary

This class is a helper for 'parser-like' use cases of LogicalResult
where the implicit conversion to bool is tolerable. It is used by the
operation asmparsers, but is more generic functionality that is closely
aligned with LogicalResult. Hoist it up to LogicalResult.h to make it
more accessible. This is part of Issue #54884

[lattner]

mlir/lib/Tools/PDLL/AST/Diagnostic.cpp

18 ↗

(On Diff #422745)

Just FYI, this is required because pdll uses the same class name as mlir core.

[mehdi_amini]

mlir/include/mlir/Support/LogicalResult.h

88

Isn't there a parser header that could host this?
It is quite more specific and narrow than "LogicalResult". Alternatively what about its own header? Either ParseResult.h or more generically ParserUtils.h (but then again why not in include/mlir/Parser/? include/mlir/Parser/Utils.h seems fine?)

[lattner]

I'd like to hoist this whole file up to LLVM/Support. LLVM has lots of parsers, and doesn't have a unified system for building these. "ParseResult" is perhaps misnamed - it is really "LogicalResult that is implicitly convertible to bool"

[mehdi_amini]

I have concerned about implicit conversion to bool in the context of LogicalResult though, it was very intentional that LogicalResult does not convert to bool implicitly. We only carved out a specific exception for the parser because of this somehow pre-existing common pattern of chaining the calls, so it isn't a "misname": it was very intentional to keep this scoped and localized.

It is so small (10 lines of code) that I would have each of the LLVM parser defining their own ParseResult if they which to do so. But if you really want to have this in llvm/Support, then llvm/Support/ParseResult.h would do it as well?

Something else that shows in this patch is a "layering violation" where the mlir/IR/Diagnostics.h classes are now exposed here.

[lattner]

Something else that shows in this patch is a "layering violation" where the mlir/IR/Diagnostics.h classes are now exposed here.

I agree, I'll work on this.

I have concerned about implicit conversion to bool in the context of LogicalResult though, it was very intentional that LogicalResult does not convert to bool implicitly. We only carved out a specific exception for the parser because of this somehow pre-existing common pattern of chaining the calls, so it isn't a "misname": it was very intentional to keep this scoped and localized.

I don't understand your concern here. This patch isn't making LogicalResult convertible to bool (fwiw, River is also staunchly opposed to this). I'm pretty aware of this history here. This patch is just moving the ParseResult class into LogicalResult.h.

You are correct that we could define a new ParseResult.h next to LogicalResult, but I don't see how a 5 line class warrants its own header, and there is nothing "dangerous" about what the patch is proposing. I also don't see how putting this in a Utils.h is better.

Are you /opposed/ to this moving into LogicalResult.h or are you just discussing?

-Chris

[lattner]

Move the implicit conversion from ParseResult.

It makes more sense to put these on InflightDiagnostic and Diagnostic,
which already have the corresponding conversion to LogicalResult.
Diagnostic.h already includes LogicalResult.h as well. This fixes a
layering issue.

[lattner]

Update class comments, clarifying rationale and further dissuading people
from using it for general error handling.

[rriddle]

I'm LGTM on having this moved to LogicalResult in general, having seen various copies or places using raw bool. Whether this is in LogicalResult.h or some ParseResult.h doesn't matter a lot to me (given how small it is at this point), the big thing to me is that the class has significant documentation that covers rationale/when it should be used/when it shouldn't. The important part to me is that any conversions to bool are well contained, and have documentation (I would have some concerns about this implicitness leaking outside of the places that it should).

mlir/lib/Tools/PDLL/AST/Diagnostic.cpp

18 ↗

(On Diff #422745)

Is this still required?

[lattner]

Drop unneeded change River pointed out.

[lattner]

You're right, the PDL change isn't needed anymore, dropped from the patch.

Do the comments on the class work for you, or do you recommend anything more in particular?

[rriddle]

LGTM from my point-of-view, but make sure that Mehdi is okay before pushing.

mlir/include/mlir/Support/LogicalResult.h

74

nit: Can we move this class to the bottom of the file?

81

parsering?

[mehdi_amini]

In D123760#3455710, @lattner wrote:

I have concerned about implicit conversion to bool in the context of LogicalResult though, it was very intentional that LogicalResult does not convert to bool implicitly. We only carved out a specific exception for the parser because of this somehow pre-existing common pattern of chaining the calls, so it isn't a "misname": it was very intentional to keep this scoped and localized.

I don't understand your concern here. This patch isn't making LogicalResult convertible to bool (fwiw, River is also staunchly opposed to this)

Right: sorry I added confusion to the discussion :)
You wrote that it is conceptually "LogicalResult that is implicitly convertible to bool" and I was just trying to say that I don't think we want ParseResult to be marketed that way and folks starting to use it widely outside of the context of the parser.

With the layering resolved by moving the conversions for InflightDiagnostic and Diagnostic, the current patch LGTM :)

[lattner]

cool, thank you both for the help!

[lattner]

Oh sorry, I didn't see River's additional comments. On it.

mlir/include/mlir/IR/Diagnostics.h

public:
   }
   iterator_range<const_note_iterator> getNotes() const {
     return llvm::make_pointee_range(notes);
   }
 
   /// Allow a diagnostic to be converted to 'failure'.
   operator LogicalResult() const;
 
+  /// Allow a diagnostic to be converted to 'failure'.
+  operator ParseResult() const { return ParseResult(LogicalResult(*this)); }
+
   /// Allow a diagnostic to be converted to FailureOr<T>. Always results in
   /// 'failure' because this cast cannot possibly return an object of 'T'.
   template <typename T>
   operator FailureOr<T>() const {
     return failure();
   }
 
 private:
public:
 
   /// Abandons this diagnostic so that it will no longer be reported.
   void abandon();
 
   /// Allow an inflight diagnostic to be converted to 'failure', otherwise
   /// 'success' if this is an empty diagnostic.
   operator LogicalResult() const;
 
+  /// Allow an inflight diagnostic to be converted to 'failure', otherwise
+  /// 'success' if this is an empty diagnostic.
+  operator ParseResult() const { return ParseResult(LogicalResult(*this)); }
+
   /// Allow an inflight diagnostic to be converted to FailureOr<T>. Always
   /// results in 'failure' because this cast cannot possibly return an object of
   /// 'T'.
   template <typename T>
   operator FailureOr<T>() const {
     return failure();
   }
 

mlir/include/mlir/IR/OpDefinition.h

 #include "llvm/Support/PointerLikeTypeTraits.h"
 
 #include <type_traits>
 
 namespace mlir {
 class Builder;
 class OpBuilder;
 
-/// This class represents success/failure for operation parsing. It is
-/// essentially a simple wrapper class around LogicalResult that allows for
-/// explicit conversion to bool. This allows for the parser to chain together
-/// parse rules without the clutter of "failed/succeeded".
-class ParseResult : public LogicalResult {
-public:
-  ParseResult(LogicalResult result = success()) : LogicalResult(result) {}
-
-  // Allow diagnostics emitted during parsing to be converted to failure.
-  ParseResult(const InFlightDiagnostic &) : LogicalResult(failure()) {}
-  ParseResult(const Diagnostic &) : LogicalResult(failure()) {}
-
-  /// Failure is true in a boolean context.
-  explicit operator bool() const { return failed(); }
-};
 /// This class implements `Optional` functionality for ParseResult. We don't
 /// directly use Optional here, because it provides an implicit conversion
 /// to 'bool' which we want to avoid. This class is used to implement tri-state
 /// 'parseOptional' functions that may have a failure mode when parsing that
 /// shouldn't be attributed to "not present".
 class OptionalParseResult {
 public:
   OptionalParseResult() = default;

mlir/include/mlir/Support/LogicalResult.h

 /// Utility function that returns true if the provided LogicalResult corresponds
 /// to a success value.
 inline bool succeeded(LogicalResult result) { return result.succeeded(); }
 
 /// Utility function that returns true if the provided LogicalResult corresponds
 /// to a failure value.
 inline bool failed(LogicalResult result) { return result.failed(); }
 
+/// This class represents success/failure for parsing-like operations that find
+/// it important to chain together failable operations with `||`.  This is an
+/// extended version of `LogicalResult` that allows for explicit conversion to
+/// bool.
+///
+/// This class should not be used for general error handling cases - we prefer
+/// to keep the logic explicit with the `succeeded`/`failed` predicates.
+/// However, traditional monadic-style parsering logic can sometimes get
+/// swallowed up in boilerplate without this, so we provide this for narrow
+/// cases where it is important.
+///
+class ParseResult : public LogicalResult {
+public:
+  ParseResult(LogicalResult result = success()) : LogicalResult(result) {}
+
+  /// Failure is true in a boolean context.
+  explicit operator bool() const { return failed(); }
+};
+
 /// This class provides support for representing a failure result, or a valid
 /// value of type `T`. This allows for integrating with LogicalResult, while
 /// also providing a value on the success path.
 template <typename T> class LLVM_NODISCARD FailureOr : public Optional<T> {
 public:
   /// Allow constructing from a LogicalResult. The result *must* be a failure.
   /// Success results should use a proper instance of type `T`.
   FailureOr(LogicalResult result) {