Fix exceeding bitshifts not emitting for assoc. consts (properly this time, I swear!) #71663
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Cc @rust-lang/wg-const-eval |
There was a problem hiding this comment.
I love it, this is very precisely targetting the issue, I wonder if we should extend the use of try_validation_pat to other situations (out of scope for this PR obviously).
Do we have a test for the ICE that was caused by the earlier fix? Just to make sure we don't regress that case again :)
Yes we do: |
Co-Authored-By: Ralf Jung <post@ralfj.de> Co-Authored-By: Oliver Scherer <github35764891676564198441@oli-obk.de>
Avoids having to repeat InterpErrorInfo { .. }
jumbatm
force-pushed
the
caller-handles-validation-error
branch
from
9de5632
to
bd18ad4
Compare
| } | ||
| /// Like try_validation, but will throw a validation error if any of the patterns in $p are | ||
| /// matched. Other errors are passed back to the caller, unchanged. This lets you use the patterns | ||
| /// as a kind of validation blacklist: |
There was a problem hiding this comment.
Isn't it more of a whitelist? Unexpected errors will bubble up to the root and cause ICEs.
| Err(_) => throw_validation_failure!($what, $where), | ||
| // We catch the error and turn it into a validation failure. We are okay with | ||
| // allocation here as this can only slow down builds that fail anyway. | ||
| $( Err(InterpErrorInfo { kind: $p, .. }) )|+ => |
There was a problem hiding this comment.
Wow this is advanced macro magic, cool stuff.
| /// | ||
| /// ``` | ||
| /// let v = try_validation_pat!(some_fn(), some_path, { | ||
| /// Foo | Bar | Baz => { "{:?}", some_failure } expected { "{}", expected_value }, |
There was a problem hiding this comment.
This is great. :D
I think my one complaint is that using curly braces to delemit parameters is a bit odd. We don't write format_args!{ "{:?}", some_failure } either. But I don't have a better proposal off the top of my head either... @oli-obk any ideas?
There was a problem hiding this comment.
I can't come up with a nicer syntax off the top of my head. Parens would definitely look worse:
Foo | Bar | Baz => ("{:?}", some_failure) expected("{}", expected_value),
I think it's fine as it is.
There was a problem hiding this comment.
Okay, let's go with curly braces then.
Should we replace expected by @expected or so to more clearly mark it as a keyword? Not sure what the usual macro conventions are, this is certainly something I have seen before.
There was a problem hiding this comment.
I think it's standing out enough by the definitely not-legal-rust syntax. But I wouldn't mind an additional @ if you think that makes it clearer.
There was a problem hiding this comment.
Okay, let's go with this for now, we can always change it later.
|
📌 Commit bd18ad4 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 4 pull requests Successful merges: - rust-lang#71398 (Add `RefCell::take`) - rust-lang#71663 (Fix exceeding bitshifts not emitting for assoc. consts (properly this time, I swear!)) - rust-lang#71726 (Suggest deref when coercing `ty::Ref` to `ty::RawPtr` with arbitrary mutability) - rust-lang#71808 (Add long error explanation for E0539) Failed merges: r? @ghost
Fixes #69021 and fixes #71353.
As described in #71353 (comment), this PR:
try_validation!calledtry_validation_pat!that allows specific failures to be turned into validation failures (but returns the rest, unchanged), andInvalidProgramto be returned out of validationr? @RalfJung