const evaluatable: improve TooGeneric handling
#77303
Conversation
rust-highfive
added
the
S-waiting-on-review
lcnr
force-pushed
the
const-evaluatable-TooGeneric
branch
from
2df7bd7 to
a4783de
Compare
| @@ -421,6 +517,33 @@ pub(super) fn try_unify_abstract_consts<'tcx>( | |||
| // on `ErrorReported`. | |||
| } | |||
|
|
|||
| fn walk_abstract_const<'tcx, F>(tcx: TyCtxt<'tcx>, ct: AbstractConst<'tcx>, mut f: F) | |||
There was a problem hiding this comment.
is there any particular reason we need to walk this in tree order and not just visit all nodes?
There was a problem hiding this comment.
So for subtrees that would be incorrect, as they can contain nodes from other subtrees.
I think if we always use a method like this for a complete tree that would be fine. Kinda afraid that we accidentially incluse unused nodes though 🤔
About unused nodes, do we want to emit an error if we encounter them during AbstractConst building.
Let me test how this deals with things like N + 1; 3 rn.
There was a problem hiding this comment.
#![feature(const_generics, const_evaluatable_checked)]
fn test<const N: usize>() -> usize {
N; 2
}results in
fn test() -> usize {
let mut _0: usize; // return place in scope 0 at ./example.rs:3:30: 3:35
let _1: usize; // in scope 0 at ./example.rs:4:5: 4:6
bb0: {
StorageLive(_1); // scope 0 at ./example.rs:4:5: 4:6
_1 = const N; // scope 0 at ./example.rs:4:5: 4:6
StorageDead(_1); // scope 0 at ./example.rs:4:6: 4:7
_0 = const 2_usize; // scope 0 at ./example.rs:4:8: 4:9
return; // scope 0 at ./example.rs:5:2: 5:2
}
}
which is currently allowed. Do we want to forbid unused nodes in abstract const building?
edit: I think we do, will open a PR for that (once this is merged as I want to reuse walk_abstract_const for that)
| F: FnMut(Node<'tcx>), | ||
| { | ||
| recurse(tcx, ct, &mut f); | ||
| fn recurse<'tcx>(tcx: TyCtxt<'tcx>, ct: AbstractConst<'tcx>, f: &mut dyn FnMut(Node<'tcx>)) { |
There was a problem hiding this comment.
any kind of recursion should probably use ensure_sufficient_stack
There was a problem hiding this comment.
Will we ever get really deep trees here?
so something like 1 + 1 + 1 + 1 + 1 .... is probably the deepest we can go,
I think we catch errors here somewhere else first.
I personally would like to wait on an actual bug report here, as ensure_sufficient_stack has a small perf impact
There was a problem hiding this comment.
It may occur in generated code somewhere, and if the rest of the compiler code is already recursion depth resistant we can hit this here. Once this is merged and in nightly I'll try to build a test that hits this.
| @@ -496,6 +496,13 @@ impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> { | |||
| obligation.cause.span, | |||
| ) { | |||
| Ok(()) => ProcessResult::Changed(vec![]), | |||
| Err(ErrorHandled::TooGeneric) => { | |||
There was a problem hiding this comment.
I don't know anything about this part of the compiler...
|
@bors r=oli-obk,varkor |
|
📌 Commit a4783de has been approved by |
bors
added
S-waiting-on-bors
…r=oli-obk,varkor const evaluatable: improve `TooGeneric` handling Instead of emitting an error in `fulfill`, we now correctly stall on inference variables. As `const_eval_resolve` returns `ErrorHandled::TooGeneric` when encountering generic parameters on which we actually do want to error, we check for inference variables and eagerly emit an error if they don't exist, returning `ErrorHandled::Reported` instead. Also contains a small bugfix for `ConstEquate` where we previously only stalled on type variables. This is probably a leftover from when we did not yet support stalling on const inference variables. r? @oli-obk cc @varkor @eddyb
Rollup of 12 pull requests Successful merges: - rust-lang#76909 (Add Iterator::advance_by and DoubleEndedIterator::advance_back_by) - rust-lang#77153 (Fix recursive nonterminal expansion during pretty-print/reparse check) - rust-lang#77202 (Defer Apple SDKROOT detection to link time.) - rust-lang#77303 (const evaluatable: improve `TooGeneric` handling) - rust-lang#77305 (move candidate_from_obligation_no_cache) - rust-lang#77315 (Rename AllocErr to AllocError) - rust-lang#77319 (Stable hashing: add comments and tests concerning platform-independence) - rust-lang#77324 (Don't fire `const_item_mutation` lint on writes through a pointer) - rust-lang#77343 (Validate `rustc_args_required_const`) - rust-lang#77349 (Update cargo) - rust-lang#77360 (References to ZSTs may be at arbitrary aligned addresses) - rust-lang#77371 (Remove trailing space in error message) Failed merges: r? `@ghost`
Instead of emitting an error in
fulfill, we now correctly stall on inference variables.As
const_eval_resolvereturnsErrorHandled::TooGenericwhen encountering generic parameters on whichwe actually do want to error, we check for inference variables and eagerly emit an error if they don't exist, returning
ErrorHandled::Reportedinstead.Also contains a small bugfix for
ConstEquatewhere we previously only stalled on type variables. This is probably a leftover fromwhen we did not yet support stalling on const inference variables.
r? @oli-obk cc @varkor @eddyb