Skip to content
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

const evaluatable: improve TooGeneric handling #77303

Merged
merged 1 commit into from
Oct 1, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 28, 2020

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2020
@lcnr lcnr force-pushed the const-evaluatable-TooGeneric branch from 2df7bd7 to a4783de Compare September 28, 2020 18:18
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any particular reason we need to walk this in tree order and not just visit all nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lcnr lcnr Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#![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>)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any kind of recursion should probably use ensure_sufficient_stack

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Sep 28, 2020
@@ -496,6 +496,13 @@ impl<'a, 'b, 'tcx> FulfillProcessor<'a, 'b, 'tcx> {
obligation.cause.span,
) {
Ok(()) => ProcessResult::Changed(vec![]),
Err(ErrorHandled::TooGeneric) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know anything about this part of the compiler...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

yeah, then let's have @varkor or @eddyb review that part

@lcnr
Copy link
Contributor Author

lcnr commented Sep 29, 2020

@bors r=oli-obk,varkor

@bors
Copy link
Contributor

bors commented Sep 29, 2020

📌 Commit a4783de has been approved by oli-obk,varkor

@bors bors added S-waiting-on-bors 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 Sep 29, 2020
@lcnr lcnr added the F-generic_const_exprs `#![feature(generic_const_exprs)]` label Sep 29, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 30, 2020
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2020
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`
@bors bors merged commit f235594 into rust-lang:master Oct 1, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 1, 2020
@lcnr lcnr deleted the const-evaluatable-TooGeneric branch October 1, 2020 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` F-generic_const_exprs `#![feature(generic_const_exprs)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants