[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

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

[mehdi_amini]

mlir/include/mlir/Support/LogicalResult.h

90

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

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

76

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

83

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/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

 //===- LogicalResult.h - Utilities for handling success/failure -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_SUPPORT_LOGICALRESULT_H
 #define MLIR_SUPPORT_LOGICALRESULT_H
 
 #include "mlir/Support/LLVM.h"
 #include "llvm/ADT/Optional.h"
 
 namespace mlir {
+class InFlightDiagnostic;
+class Diagnostic;
 
 /// This class represents an efficient way to signal success or failure. It
 /// should be preferred over the use of `bool` when appropriate, as it avoids
 /// all of the ambiguity that arises in interpreting a boolean result. This
 /// class is marked as NODISCARD to ensure that the result is processed. Users
 /// may explicitly discard a result by using `(void)`, e.g.
 /// `(void)functionThatReturnsALogicalResult();`. Given the intended nature of
 /// this class, it generally shouldn't be used as the result of functions that

 /// 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 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 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) {

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

 
 using namespace mlir;
 using namespace mlir::pdll::ast;
 
 //===----------------------------------------------------------------------===//
 // InFlightDiagnostic
 //===----------------------------------------------------------------------===//
 
-void InFlightDiagnostic::report() {
+void mlir::pdll::ast::InFlightDiagnostic::report() {
   // If this diagnostic is still inflight and it hasn't been abandoned, then
   // report it.
   if (isInFlight()) {
     owner->report(std::move(*impl));
     owner = nullptr;
   }
   impl.reset();
 }