-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use translatable diagnostics in rustc_const_eval
#111677
Use translatable diagnostics in rustc_const_eval
#111677
Conversation
r? @oli-obk (rustbot has picked a reviewer for you, use r? to override) |
cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
dc3d10b
to
6fbb7b3
Compare
This comment has been minimized.
This comment has been minimized.
We have plans to change the error rendering for const-eval and Miri errors quite a bit, see rust-lang/miri#2200. I have no idea how that interacts with these translation changes, does it make them harder or easier? I expect harder because now there is another huge new subsystem one has to learn to even being doing such a change. |
dest.layout.size.bytes(), | ||
src.layout.ty, | ||
dest.layout.ty, | ||
fluent::const_eval_invalid_transmute, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this has a dedicated message now, why should it still use the Custom
variant?
Or conversely, if Custom
variants are now 'typed' like this, then why do we need the other variants at all?
Also, for Miri we can still use arbitrary strings here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that this doesn't need to be Custom
, but I think this should be done as followup work since this PR is already enormous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with leaving FIXMEs, I was just trying to understand why the code looks the way it does. If anything I feel it'd make sense to move more messages to Custom
than the other way around, but I might be missing some parts of the trade-off here.
pub required: u64, | ||
#[subdiagnostic] | ||
pub frames: Vec<FrameNote>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all these types here auto-generated or do they need to be manually synced with the .flt file (in terms of field/variable names etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually synced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uff, that's suboptimal, and so easy to get wrong. I thought "there is no variable named foo
" is an error you see at runtime in Python bot not Rust ;) . Are there plans to improve this? Currently it looks like translations introduce quite a bit of boilerplate and make the code significantly more fragile. I'm very happy the core interpreter rarely changes these days, or else I would be worried about its future development being impacted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's definitely increasing the work required to add a variant to InterpError
.
I think it would be nice if there is a DSL that generated both fluent resources and the Rust types that derived Diagnostic
and Subdiagnostic
, but I'm not sure if that's the plan.
cc @rust-lang/wg-diagnostics any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR to validate variables used open (#111269) but it unfortunately doesn't work with subdiagnostics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fluent is already doing some form of code generation (right? I saw some use
statements that look like this), so I was hoping a fluent line like
middle_uninit_unsized_local =
unsized local is used while uninitialized {$variable}
could auto-generate a corresponding Rust struct with a field for the variable
. Then we don't need a separate pass to check things, we get the check just directly as part of compilation.
I assume there are good reasons for why fluent works the way it does, but it sure does not feel very "rust-y" to me.
I think I could do it in this PR by adding an enum that prints brief "title"s for errors. This PR also makes it impossible to use |
tests/ui/consts/const-eval/heap/alloc_intrinsic_nontransient_fail.stderr
Outdated
Show resolved
Hide resolved
Probably better to separate functional changes from major refactors like this. However Miri currently relies quite a bit on the ability to put InterpError into the title... |
☔ The latest upstream changes (presumably #111680) made this pull request unmergeable. Please resolve the merge conflicts. |
The Miri subtree was changed cc @rust-lang/miri
|
4c49144
to
bc2f393
Compare
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5d2f4a1
to
b569845
Compare
fixed ci error @bors r=oli-obk,RalfJung |
📌 Commit b569845b920793b9797c2a704a24a11569e18d2a has been approved by It is now in the queue for this repository. |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
b569845
to
f6c2bc5
Compare
@bors p=1 (bitrotty) |
rebased @bors r=oli-obk,RalfJung |
☀️ Test successful - checks-actions |
Finished benchmarking commit (33c3d10): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 644.915s -> 644.751s (-0.03%) |
This PR:
no_span
parameter tonote
/help
attributes when usingSubdiagnostic
to allow adding notes/helps without using a span