Move various checks to typeck so them failing causes the typeck result to get tainted by oli-obk · Pull Request #96046 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation39 Commits9 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Some changes occured to the CTFE / Miri engine
cc @rust-lang/miri
Some changes occured to the CTFE / Miri engine
cc @rust-lang/miri
Relevant to the compiler team, which will review and decide on the PR/issue.
label
r? @jackh726
(rust-highfive has picked a reviewer for you, use r? to override)
| @@ -148,8 +148,6 @@ pub enum InvalidProgramInfo<'tcx> { |
|---|
| /// (which unfortunately typeck does not reject). |
| /// Not using `FnAbiError` as that contains a nested `LayoutError`. |
| FnAbiAdjustForForeignAbi(call::AdjustForForeignAbiError), |
| /// An invalid transmute happened. |
| TransmuteSizeDiff(Ty<'tcx>, Ty<'tcx>), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great. :D So does this also fix #79047 ?
What about SizeOfUnsizedType, which seems similar to me?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar, but when I looked into it it was a different beast that I'll tackle seperately
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great. :D So does this also fix #79047 ?
yes, the corresponding test has been updated
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to keep the issue open for SizeOfUnsizedType though. Do we have a nice example of that (or an existing separate issue)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I saw an example when I removed that variant ^^ but I don't remember where
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about SizeOfUnsizedType, which seems similar to me?
I have opened #97477 for this.
This looks fine to me, but I'm not really familiar with this code, so maybe @RalfJung wants to give the final say?
typeck is outside my purview, sorry -- I can't really help with this PR, other than that I like the end-to-end effect of turning that one check in the interpreter into an assertion.
Rerolling the reviewer dice
r? rust-lang/compiler
r? rust-lang/compiler
I won't be able to get to reviewing things for the upcoming next couple weeks.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really review the typeck algorithm, but left a few questions where comments could be appropriate.
| fcx.check_transmutes(); |
|---|
| } |
| fcx.check_asms(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the difference between check_transmutes and check_asms?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to replicate the previous diagnostics. For transmute that meant not to check if there were other errors already happening, as that would just cause useless errors in the transmute checks
| @@ -274,6 +274,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { |
|---|
| error: &SelectionError<'tcx>, |
| fallback_has_occurred: bool, |
| ) { |
| self.set_tainted_by_errors(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change anything?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. It avoids ICEs and bogus errors in CTFE.
… const eval can avoid running at all
📌 Commit 6ba8da6 has been approved by cjgillot
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
⌛ Testing commit 6ba8da6 with merge d3e7c4b8bb35907bab7d8714b84b21e7bc39fda0...
This comment has been minimized.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
📌 Commit 4332c2f has been approved by cjgillot
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Finished benchmarking commit (56fd680): comparison url.
Instruction count
- Primary benchmarks: 🎉 relevant improvements found
- Secondary benchmarks: mixed results
| mean1 | max | count2 | |
|---|---|---|---|
| Regressions 😿 (primary) | N/A | N/A | 0 |
| Regressions 😿 (secondary) | 0.9% | 1.1% | 4 |
| Improvements 🎉 (primary) | -0.3% | -0.4% | 4 |
| Improvements 🎉 (secondary) | -0.3% | -0.3% | 24 |
| All 😿🎉 (primary) | -0.3% | -0.4% | 4 |
Max RSS (memory usage)
Results
- Primary benchmarks: 🎉 relevant improvements found
- Secondary benchmarks: 😿 relevant regression found
| mean1 | max | count2 | |
|---|---|---|---|
| Regressions 😿 (primary) | N/A | N/A | 0 |
| Regressions 😿 (secondary) | 2.5% | 2.5% | 1 |
| Improvements 🎉 (primary) | -2.4% | -2.8% | 2 |
| Improvements 🎉 (secondary) | N/A | N/A | 0 |
| All 😿🎉 (primary) | -2.4% | -2.8% | 2 |
Cycles
Results
- Primary benchmarks: no relevant changes found
- Secondary benchmarks: 🎉 relevant improvements found
| mean1 | max | count2 | |
|---|---|---|---|
| Regressions 😿 (primary) | N/A | N/A | 0 |
| Regressions 😿 (secondary) | N/A | N/A | 0 |
| Improvements 🎉 (primary) | N/A | N/A | 0 |
| Improvements 🎉 (secondary) | -2.8% | -3.2% | 2 |
| All 😿🎉 (primary) | N/A | N/A | 0 |
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
@rustbot label: -perf-regression
Footnotes
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.