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 generic inference variables not properly tracked by traits/fulfill. #70180

Closed
eddyb opened this issue Mar 20, 2020 · 2 comments · Fixed by #72061
Closed

const generic inference variables not properly tracked by traits/fulfill. #70180

eddyb opened this issue Mar 20, 2020 · 2 comments · Fixed by #72061
Assignees
Labels
A-const-generics Area: const generics (parameters and arguments) A-inference Area: Type inference C-bug Category: This is a bug. F-const_generics `#![feature(const_generics)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 20, 2020

I've come up with this example (playground):

#![feature(const_generics)]

fn works() {
    let array/*: [_; _]*/ = default_array();
    let _: [_; 4] = array;
    Foo::foo(&array);
}

fn doesnt_work() {
    let array/*: [_; _]*/ = default_array();
    // ^ cannot infer type for type parameter `T` declared on the function `default_array`
    Foo::foo(&array);
    let _: [_; 4] = array;
}

trait Foo {
    fn foo(&self) {}
}

impl Foo for [i32; 4] {}
impl Foo for [i64; 8] {}

// Only needed because `[_; _]` is not valid type syntax.
fn default_array<T, const N: usize>() -> [T; N]
    where [T; N]: Default,
{
    Default::default()
}

My understanding is that the error is caused by the [$T; $C]: Foo obligation being stalled_on on just $T (which means it's not retried when $C is unified with 4), whereas it should be stalled on both $T and $C so that it's retried when either changes.

Right now stalled_on is limited to type inference variables, so its representation will need to change, and Ty::walk will need to expose constants too.

I believe #70107 ran into this as well, but for WF obligations, not trait ones.

I'm self-assigning this because I'm working on the fix (as part of a larger effort).

cc @nikomatsakis @varkor @yodaldevoid

@eddyb eddyb added the A-const-generics Area: const generics (parameters and arguments) label Mar 20, 2020
@eddyb eddyb self-assigned this Mar 20, 2020
@jonas-schievink jonas-schievink added A-inference Area: Type inference C-bug Category: This is a bug. F-const_generics `#![feature(const_generics)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 20, 2020
@eddyb
Copy link
Member Author

eddyb commented Mar 20, 2020

Note to self: the PR fixing this should also test this example (playground):

#![feature(const_generics)]

fn works() {
    let array/*: [u8; _]*/ = default_byte_array();
    let _: [_; 4] = array;
    Foo::foo(&array);
}

fn doesnt_work() {
    let array/*: [u8; _]*/ = default_byte_array();
    Foo::foo(&array);
    // ^ cannot infer type for type parameter `T` declared on the trait `Foo`
    let _: [_; 4] = array;
}

trait Foo<T> {
    fn foo(&self) {}
}

impl Foo<i32> for [u8; 4] {}
impl Foo<i64> for [u8; 8] {}

// Only needed because `[u8; _]` is not valid type syntax.
fn default_byte_array<const N: usize>() -> [u8; N]
    where [u8; N]: Default,
{
    Default::default()
}

The reason for this is that this second example has the type and const inference variables in different generic args of Foo, meaning fixing it also requires changing this to has_infer_types_or_consts:

.filter(|t| t.has_infer_types())

@varkor
Copy link
Member

varkor commented Mar 20, 2020

I've definitely noticed problems with inference involving consts before, but wasn't able to pin down where the issue was stemming from. It would be great if this could be addressed once and for all.

Centril added a commit to Centril/rust that referenced this issue Mar 24, 2020
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
Centril added a commit to Centril/rust that referenced this issue Mar 24, 2020
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
Centril added a commit to Centril/rust that referenced this issue Mar 24, 2020
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 25, 2020
add regression tests for stalled_on const vars

closes rust-lang#70180

Afaict this has been fixed sometime after rust-lang#70213

`trait_ref_type_vars` correctly adds const infers and I did not find any remaining `FIXME`s which correspond to this issue.
https://github.com/rust-lang/rust/blob/7c59a81a5fcbaaca311f744cd7c68d99bfbb05d3/src/librustc_trait_selection/traits/fulfill.rs#L555-L557

Added both examples from the issue as regression tests and renamed `trait_ref_type_vars` -> `trait_ref_infer_vars`.

r? @eddyb
@bors bors closed this as completed in b3f1b95 May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) A-inference Area: Type inference C-bug Category: This is a bug. F-const_generics `#![feature(const_generics)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants