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

Add a storage for the origin of const inference variables. #72328

Closed
lcnr opened this issue May 18, 2020 · 8 comments
Closed

Add a storage for the origin of const inference variables. #72328

lcnr opened this issue May 18, 2020 · 8 comments
Labels
A-const-generics Area: const generics (parameters and arguments) A-diagnostics Area: Messages for errors, warnings, and lints A-inference Area: Type inference C-enhancement Category: An issue proposing an enhancement or a PR with one. F-const_generics `#![feature(const_generics)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented May 18, 2020

We currently don't store the origin of const inference variables. This is needed to emit more helpful error messages.

For type inference variables, this is done using https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxtInner.html#method.type_variables and then
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/type_variable/struct.TypeVariableTable.html#method.var_origin.

We use this to provide better diagnostics in case type inference fails:

let ty_vars = &inner.type_variables();
let var_origin = ty_vars.var_origin(ty_vid);
if let TypeVariableOriginKind::TypeParameterDefinition(name, def_id) = var_origin.kind {
let parent_def_id = def_id.and_then(|def_id| self.tcx.parent(def_id));

A similar mechanism should probably also be used for consts.

@lcnr lcnr added A-const-generics Area: const generics (parameters and arguments) A-diagnostics Area: Messages for errors, warnings, and lints A-inference Area: Type inference C-enhancement Category: An issue proposing an enhancement or a PR with one. F-const_generics `#![feature(const_generics)]` and removed A-inference Area: Type inference labels May 18, 2020
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 18, 2020
@varkor
Copy link
Member

varkor commented Sep 13, 2020

It would be good to have some examples of where diagnostics are subpar because of this missing feature, to decide if it's a prerequisite for stabilising min_const_generics.

@lcnr
Copy link
Contributor Author

lcnr commented Sep 13, 2020

yeah, will look a bit into some interesting test cases here in the near future hopefully

@lcnr
Copy link
Contributor Author

lcnr commented Sep 14, 2020

One issue here is something like this:

#![feature(min_const_generics)]

struct Foo;

impl Foo {
    fn bar(self) -> Foo {
        Foo
    }

    fn baz<const N: usize>(self) -> Foo {
        println!("baz: {}", N);
        Foo
    }
}

fn main() {
    Foo.bar().bar().bar().bar().baz();
}

which results in

error[E0282]: type annotations needed
  --> src/main.rs:17:5
   |
17 |     Foo.bar().bar().bar().bar().baz();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: unable to infer the value of a const parameter

for baz<T> we instead get

error[E0282]: type annotations needed
  --> src/main.rs:17:33
   |
17 |     Foo.bar().bar().bar().bar().baz();
   |                                 ^^^ cannot infer type for type parameter `T` declared on the associated function `baz`

btw I am not actually sure if this issue makes sense, we already have the origin of const infer values stored, so
I don't actually think we need to add a new storage here 😆 we "just" have to use the current one

@pickfire
Copy link
Contributor

Is there any suggestion for user on how to fix this?

@lcnr
Copy link
Contributor Author

lcnr commented Sep 14, 2020

Not completely sure what you mean here @pickfire

Do you mean when the user encounters a type annotations needed error? If so, we should add a suggestion like the following if it's applicable

note: use the turbofish here, I know you want to

On working on this issue itself, I am not actually sure what this issue actually is about, as my initial reason for opening it was somewhat incorrect.

I do think we should further improve the error message when type inference gets stuck for const inference variables. I don't know the best way to do this though, so I can't really give any mentoring instructions here yet

@varkor
Copy link
Member

varkor commented Sep 14, 2020

I think the main reason this issue exists is that when type inference fails for type parameters, we already have lots of heuristics to suggest to the user how to correct it. We don't have any of those for consts yet. However, this might be something that we continue to improve over time, so there may not be any direct action we can take with regards to this issue now.

@pickfire
Copy link
Contributor

note: use the turbofish here, I know you want to

Yes, I don't know how to solve const issue but it would be good to have some sort of guidance.

@varkor
Copy link
Member

varkor commented Sep 15, 2020

I've opened #76737 for the turbofish suggestion. Let's simply open new issues as we encounter problems, and then we can close this one.

@varkor varkor closed this as completed Sep 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 16, 2020
improve const infer error

cc rust-lang#72328

reduces it from
```
error[E0282]: type annotations needed
  --> src/main.rs:17:5
   |
17 |     Foo.bar().bar().bar().bar().baz();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: unable to infer the value of a const parameter
```
to
```
error[E0282]: type annotations needed
  --> $DIR/method-chain.rs:21:33
   |
LL |     Foo.bar().bar().bar().bar().baz();
   |                                 ^^^
   |
   = note: cannot infer the value of the const parameter `N`
```

r? @varkor
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 16, 2020
improve const infer error

cc rust-lang#72328

reduces it from
```
error[E0282]: type annotations needed
  --> src/main.rs:17:5
   |
17 |     Foo.bar().bar().bar().bar().baz();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: unable to infer the value of a const parameter
```
to
```
error[E0282]: type annotations needed
  --> $DIR/method-chain.rs:21:33
   |
LL |     Foo.bar().bar().bar().bar().baz();
   |                                 ^^^
   |
   = note: cannot infer the value of the const parameter `N`
```

r? @varkor
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-diagnostics Area: Messages for errors, warnings, and lints A-inference Area: Type inference C-enhancement Category: An issue proposing an enhancement or a PR with one. F-const_generics `#![feature(const_generics)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants