-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Continue to get rid of ty::Const::{try_}eval*
#130950
Conversation
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in engine.rs, potentially modifying the public API of Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_sanitizers cc @rust-lang/project-exploit-mitigations, @rcvalle The Miri subtree was changed cc @rust-lang/miri |
try::Const::{try_}eval*
This comment has been minimized.
This comment has been minimized.
ah debuginfo |
627a151
to
98b5f4f
Compare
This comment has been minimized.
This comment has been minimized.
if let Some(len) = | ||
len.try_eval_target_usize(self.tcx, self.tcx.param_env(adt.did())) | ||
{ | ||
if let Some(len) = len.try_to_target_usize(self.tcx) { |
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.
This possibly may need to be normalized w/ lazy norm?
@@ -290,6 +290,36 @@ where | |||
Ok(ty) | |||
} | |||
} | |||
|
|||
/// Normalize a type for when it is structurally matched on. |
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.
We need this so we can structurally normalize a const in consider_builtin_transmute_candidate
. That structurally matches on an Assume
struct which contains a bunch of bools for configuration.
@@ -2220,117 +2220,136 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { | |||
span: Span, | |||
) -> GetSafeTransmuteErrorAndReason { | |||
use rustc_transmute::Answer; | |||
self.probe(|_| { |
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.
Best reviewed without whitespace. We added a probe so we can normalize below without side-effects.
@@ -134,13 +133,8 @@ mod rustc { | |||
use rustc_middle::ty::ScalarInt; | |||
use rustc_span::symbol::sym; | |||
|
|||
let Ok((ty, cv)) = c.eval(tcx, param_env, DUMMY_SP) else { | |||
return Some(Self { |
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.
This was sketchy. If a const was failing to eval, we just returned "whatever". 🤔
try::Const::{try_}eval*
ty::Const::{try_}eval*
@rustbot author some doctest is failing fml |
☔ The latest upstream changes (presumably #131036) made this pull request unmergeable. Please resolve the merge conflicts. |
98b5f4f
to
66760f4
Compare
@rustbot ready |
☔ The latest upstream changes (presumably #131892) made this pull request unmergeable. Please resolve the merge conflicts. |
I resolved the large threads since they were making it hard to review the actual code changes themselves and it also seemed like @RalfJung had a better understanding of what's going on now? Apologies if this was over-eager ^^ |
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.
Thank you :3 Looks good to me, sorry for taking so long to review. Once you resolve the nit, rebase, and maybe squash some of these commits? r=me
66760f4
to
5cf8107
Compare
@bors r+ rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (f2ba411): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -1.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
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: 781.459s -> 781.789s (0.04%) |
This PR mostly does:
try_eval_*
andeval_*
helpers fromty::Const
, and replace their usages withtry_to_*
.ty::Const::eval
.ty::Const::normalize
toty::Const::normalize_internal
. This function is still used in the normalization code itself.TransmuteFrom
goal.I'm happy to split it out further; for example, I could probably land the first part which removes the helpers, or the changes to codegen which are more obvious than the changes to tools.
r? BoxyUwU
Part of #130704