Conversation
Refactor ErrorReason to be a std::variant that can carry extra information beyond an error code. Use this expanded capability to report the missing features for the error about distinct rec groups being identical after binary writing. Fixes #8519.
tlively
requested review from
aheejin and
kripken
and removed request for
a team
src/wasm-type.h
Outdated
| std::monostate, // NonStructDescriptor | ||
| std::monostate, // MismatchedDescriptor | ||
| std::monostate, // InvalidUnsharedDescriptor | ||
| std::monostate, // InvalidUnsharedDescribes |
There was a problem hiding this comment.
This looks kind of odd to have all these in comments? How about defining a type for each? After that, we wouldn't need ErrorReasonKind I think.
There was a problem hiding this comment.
Having ErrorReasonKind is still useful because doing a switch over the index() of the variant cast to an enum value is the only way to get both exhaustiveness checking and readable code. Given that we want the ErrorReasonKind enum anyway, defining separate types for each variant member would be redundant.
There was a problem hiding this comment.
in that case, can this variant be std::variant<ErrorReasonKind, RecGroupCollision>?
There was a problem hiding this comment.
It could, but as a follow-on I think it would be nice to add more data to several more of these errors, so we'd end up with more variants anyway. I also think it's good to treat errors with and without extra data uniformly, rather than collapsing all the errors without extra data into a single error code entry in the variant.
There was a problem hiding this comment.
To be clear, I'm not opposed to more variants. I am just concerned about this variant which has dozens of the same monostate, all identical - that makes it hard to understand what's going on, I think, and might make debugging one of these instances hard too. If this variant is parallel to the enum then it feels like we don't need both?
There was a problem hiding this comment.
The new types would help enforce that this does not get out of date. They wouldn't really do it, but at least the name identity would be harder to get wrong than code comments. If there were just 2 or three of these I'd think otherwise, but the large number means that editing one requires careful counting (or trusting the comments).
Anyhow, I don't mean to argue here endlessly, and I don't love the idea of dozens of new types either. But I don't follow your point about my suggestion std::variant<ErrorReasonKind, RecGroupCollision>:
I also think it's good to treat errors with and without extra data uniformly, rather than collapsing all the errors without extra data into a single error code entry in the variant.
What is the concrete benefit of the current code over that? "uniformity" is a general principle that is good, but is it worth larger code and a risk of two large things getting out of date?
There was a problem hiding this comment.
I'll give it a shot and see if there are any downsides in practice.
There was a problem hiding this comment.
Very sad that we don't have proper Rust enums here :'(
There was a problem hiding this comment.
Sounds good. I trust you to decide if there are downsides or not. lgtm after that, and with my other comment on showing the flag.
There was a problem hiding this comment.
Ok, yes, it is undeniably simpler to just have two cases in the variant for now.
src/wasm/wasm-type.cpp
Outdated
| if (auto* collision = std::get_if<TypeBuilder::RecGroupCollision>(&reason)) { | ||
| if (collision->missingFeatures != FeatureSet(FeatureSet::None)) { | ||
| os << " (missing features " << collision->missingFeatures.toString() | ||
| << ")"; |
There was a problem hiding this comment.
| << ")"; | |
| << " [--enable-" << collision->missingFeatures.toString() << "])"; |
This will print out the flag the user is missing, like we do in the validator.
There was a problem hiding this comment.
Done, but using iterFeatures in case there are multiple missing features. (In that case enabling any one of them would technically be sufficient, but the user almost certainly should have enabled all of them, so I don't think we need to clarify that in the error message.)
src/wasm-type.h
Outdated
| std::monostate, // NonStructDescriptor | ||
| std::monostate, // MismatchedDescriptor | ||
| std::monostate, // InvalidUnsharedDescriptor | ||
| std::monostate, // InvalidUnsharedDescribes |
There was a problem hiding this comment.
Sounds good. I trust you to decide if there are downsides or not. lgtm after that, and with my other comment on showing the flag.
2 participants
Refactor ErrorReason to be a std::variant that can carry extra information beyond an error code. Use this expanded capability to report the missing features for the error about distinct rec groups being identical after binary writing.
Fixes #8519.