Make Error and ErrorOr copyable and assignable

[geoffromer]

No description provided.

[pk19604014]

curious, what's the motivation for the change?

[geoffromer]

The style guide encourages making types copyable when copying has a clear meaning, and discourages using const&/&& overload pairs. This PR aligns the code with both principles.

Note that llvm::Error seems to be move-only as part of its mechanism for preventing code from ignoring the error, but we've solved that problem with [[nodiscard]] instead.

[jonmeow]

If the main rationale is about style, I'll note:

"A type should not be copyable/movable if the meaning of copying/moving is unclear to a casual user, or if it incurs unexpected costs."

Just to note, my original intent had been to minimize accidental copy performance overhead (as copies may be expensive, particularly for ErrorOr). I believe in the toolchain we'd want to be more cautious about copies, and not providing a copy constructor is one way to avoid incurring related costs.

This may be more appropriate for executable_semantics, but maybe then the class should be moved correspondingly in order to decrease the likelihood of reuse elsewhere in the toolchain?

[geoffromer]

"Accidental" is importantly different from "unexpected" here. That passage of the style guide is saying you should disable copying if it costs meaningfully more than typical users would expect it to cost. It's not saying that you should disable copying if a user can incur unexpected costs by copying when they didn't mean to. If it was, that exception would swallow the rest of the rule, because any type holding a nontrivial amount of data can "incur unexpected costs" by that definition.

[jonmeow]

Honestly, I disagree -- I don't feel confident about when a copy will occur, and when a move will occur. To say that when I write a copy when I intend for a move is an "accident" and not "unexpected" ignores what I expect.

[geoffromer]

Are you arguing to completely remove the checks that errors are handled? Basically, removing nodiscard and the bool?

No, I prefer the status quo on both points, but the status quo is not what you think it is: there's no bool. Early revisions of #1137 had it, but it was removed during review. Neither Error nor ErrorOr has any run-time tracking of whether the error has been consumed. They do have some compile-time enforcement through nodiscard, but that's at best a weak approximation of linear typing, and certainly doesn't ensure the error is consumed. For example, it doesn't prevent bugs like this:

ErrorOr<Success> err = MightFail();
// ...
return Success();

nodiscard merely ensures that the author (and sufficiently attentive readers) are aware that MightFail() returns something. In other words, it ensures that bugs like the above are at least visible in the calling function.

Especially for ErrorOr, essentially all of the comparable designs I'm aware of use both nodiscard and a bool to track that the error is consumed.

absl::StatusOr doesn't have such a bool, and neither does the proposed std::expected. llvm::Error and llvm::ErrorOr do, but it comes at a substantial ergonomic cost: there's no way to read the error message without consuming the object. That means, for example, that they interoperate poorly with GoogleTest (as we discovered as soon as we started trying to use them), because GoogleTest's design pervasively assumes that you can observe the salient state of a value with const operations.

Conversely, I've never heard complaints about the lack of error-consumed tracking in either absl::StatusOr (which I used to co-own) or std::expected. It doesn't even seem to come up in the design rationale for std::expected, which addresses the feedback they've received pretty exhaustively. So I'm much more inclined to follow the examples of StatusOr and expected here, and both of those types are copyable.

[chandlerc]

I find the combination of [[nodiscard]] on the type (not just on a function), but zero dynamic checking... odd.

I guess this really gets to [[nodiscard]] on a type not meaning what I think it means. It doesn't mean an obligation to the holder at all, it just adds an obligation to the caller of a function that returns the type to do something with the returned value. Those values have no subsequent obligation at all. From that sense, I think I'm convinced that being copyable makes a lot of sense. But this hinges on [[nodiscard]] not creating on obligation to holder (to address @jonmeow point above) and thus copying it not creating more obligations (to address @zygoloid's point before that).

I'm honestly surprised that this pattern has worked well for absl::StatusOr and std::expected but 🤷🏻 - I guess I get to live with being surprised.

Curious whether this makes @zygoloid and @jonmeow happy or not?

[jonmeow]

If I understand correctly, what you're saying is that we can't completely rely on [[nodiscard]] because it doesn't handle every situation. It only errors when the return type is immediately discarded, for example:

MightFail();
return Success();

And as a consequence of not handling any case, there's no obligation for the type per se. Is that correct?

Note though, not every function is going to be named MightFail(), it may be something like AddEntity and not obviously have a failure mode. [[nodiscard]] means there will be something at the call site for a human reviewer to see (or author to hit a compile error and fix). Thus, even as an imperfect obligation, it still shifts more of the review burden to a compiler check.

I think that either way you go, not everybody's going to be happy. But if the decision is to not rely on these imperfect compiler checks, and especially if it hinges on [[nodiscard]] being imperfect, would that imply we should remove [[nodiscard]] everywhere in Carbon and simply rely on review to handle it?

[pk19604014]

Let's not remove [[nodiscard]] ;) For example, in the std::exit -> return-error conversion PR a nodiscard on ErrorBuilder flagged two places where I forgot to add return to the COMPILATION_ERROR() macro, which would have been very hard to spot during manual review process.

[geoffromer]

If I understand correctly, what you're saying is that we can't completely rely on [[nodiscard]] because it doesn't handle every situation. It only errors when the return type is immediately discarded, for example:

MightFail();
return Success();

And as a consequence of not handling any case, there's no obligation for the type per se. Is that correct?

I'm saying that we can't rely on it to enforce a linear typing/"obligation" model, because that's not what it's designed to do. I think we can rely on it to ensure that potential discarded-error bugs are locally visible.

Note though, not every function is going to be named MightFail(), it may be something like AddEntity and not obviously have a failure mode. [[nodiscard]] means there will be something at the call site for a human reviewer to see (or author to hit a compile error and fix). Thus, even as an imperfect obligation, it still shifts more of the review burden to a compiler check.

I think that either way you go, not everybody's going to be happy. But if the decision is to not rely on these imperfect compiler checks, and especially if it hinges on [[nodiscard]] being imperfect, would that imply we should remove [[nodiscard]] everywhere in Carbon and simply rely on review to handle it?

On the contrary, if we're going to "rely on review" to detect discarded-error bugs (and I think we have to, at least as a backstop), that seems like a strong argument for keeping nodiscard. As you noted in the previous paragraph, one of the main things nodiscard is good at is ensuring that there's something at the callsite for the reviewer to see.

[pk19604014]

Does removing this mean the code like return Error(std::move(result_or_error).error()) won't be able to use Error's move constructor?

[geoffromer]

Yes, but since Error is copyable, that code will still work, it just might be less efficient. I wouldn't be opposed to adding a non-const overload of error(), which would let people move out of it, but I'm not sure it's necessary.