Should mergedParsedLibraries always emit an error ?

[nicolasvasilache]

In #68661, @ftynse notes

Is it necessarily a verification error? Also, hasn't mergeSymbolsInto already emitted a more precise diagnostic?

about the following:

return mergedParsedLibraries->emitError()
               << "failed to verify merged transform module";

[ingomueller-net]

Proposed fix: #69331.

Note that the PR currently still emits another error. I think that it can be helpful to have some context for the precise error. (I wonder whether the function should return InFlightDiagnostics instead of LogicalResult...? That doesn't seem to be a common pattern, though...)

[ftynse]

It cannot remove InFlightDiagnostic. It emits the message in the destructor, which may or may not get called on return depending on copy elision. That's (partially) why we have DiagnosedSilenceableFailure in the transform dialect infrastructure. That carries a diagnostic that is explicitly reportable. But it is more expensive. If we definitely want to emit an diagnostic, we should do that in a place that has the most context and just propagate failure(). Otherwise, we can use DiagnosedSilenceableFailure in TD library or, alternatively, pass in an optional callback for error emission.

[joker-eph]

It emits the message in the destructor, which may or may not get called on return depending on copy elision.

You can always move explicitly?

[ftynse]

return std::move(...) prevents elision so the destructor is always called. Did you mean something else?

[joker-eph]

This is what I mean: C++17 guarantees copy-elision in many cases, in the case where it does not you can move instead: the destructor will be called but that shouldn't have any effect on a diagnostic that has been moved from.

[ingomueller-net]

I sounds like the conclusion is to have mergeSymbolsInto return an InFlightDiagnostics and attach the additional information there. Did I get that right?

[ingomueller-net]

I sounds like the conclusion is to have mergeSymbolsInto return an InFlightDiagnostics and attach the additional information there. Did I get that right?

I modified #69331 to implement to what I understood is the suggestion.