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

Duplicated implementation is not general enough error on function pointers #97997

Closed
madsmtm opened this issue Jun 11, 2022 · 4 comments · Fixed by #98022
Closed

Duplicated implementation is not general enough error on function pointers #97997

madsmtm opened this issue Jun 11, 2022 · 4 comments · Fixed by #98022
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@madsmtm
Copy link
Contributor

madsmtm commented Jun 11, 2022

Given the following code (playground link):

trait Foo {
    const ASSOC: bool = true;
}
impl<T> Foo for fn(T) {}

fn foo(_x: i32) {}

fn impls_foo<T: Foo>(_x: T) {}

fn main() {
    // Works, but if the below code is present, it is shown as failed
    impls_foo(foo as fn(i32));

    // Fails as expected, see https://github.com/rust-lang/rust/issues/56105
    <fn(&u8) as Foo>::ASSOC;
}

The current (nightly ec55c61 2022-06-10, regressed in the past week) output is:

   Compiling playground v0.0.1 (/playground)
error: implementation of `Foo` is not general enough
  --> src/main.rs:12:5
   |
12 |     impls_foo(foo as fn(i32));
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `Foo` is not general enough
   |
   = note: `Foo` would have to be implemented for the type `for<'r> fn(&'r u8)`
   = note: ...but `Foo` is actually implemented for the type `fn(&'0 u8)`, for some specific lifetime `'0`

error: implementation of `Foo` is not general enough
  --> src/main.rs:15:5
   |
15 |     <fn(&u8) as Foo>::ASSOC;
   |     ^^^^^^^^^^^^^^^^^^^^^^^ implementation of `Foo` is not general enough
   |
   = note: `Foo` would have to be implemented for the type `for<'r> fn(&'r u8)`
   = note: ...but `Foo` is actually implemented for the type `fn(&'0 u8)`, for some specific lifetime `'0`

error: could not compile `playground` due to 2 previous errors

Ideally the output should look like:

   Compiling playground v0.0.1 (/playground)
error: implementation of `Foo` is not general enough
  --> src/main.rs:15:5
   |
15 |     <fn(&u8) as Foo>::ASSOC;
   |     ^^^^^^^^^^^^^^^^^^^^^^^ implementation of `Foo` is not general enough
   |
   = note: `Foo` would have to be implemented for the type `for<'r> fn(&'r u8)`
   = note: ...but `Foo` is actually implemented for the type `fn(&'0 u8)`, for some specific lifetime `'0`

error: could not compile `playground` due to previous error

The desired result can be achieved by swapping the order of the two statements <fn(&u8) as Foo>::ASSOC; and impls_foo(foo as fn(i32));.

@madsmtm madsmtm added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2022
@TaKO8Ki TaKO8Ki self-assigned this Jun 12, 2022
@compiler-errors
Copy link
Member

compiler-errors commented Jun 12, 2022

searched nightlies: from nightly-2022-06-07 to nightly-2022-06-08
regressed nightly: nightly-2022-06-08
searched commit range: 50b0025...5435ed6
regressed commit: bb55bd4

bisected with cargo-bisect-rustc v0.6.3

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2022-06-07 --end=2022-06-08 --prompt 

@TaKO8Ki, are you working on this? I'll probably be investigating this due to it being related to the recent removal of the borrowck migration mode.

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Jun 12, 2022

@compiler-errors
I've been working on it a bit, but please feel free to take over this issue!

@compiler-errors
Copy link
Member

This actually looks like a particularly difficult issue.

I think it's either a bug in RegionInferenceContext::find_constraint_paths_between_regions, or something weird with the RegionInferenceContext.constraint_graph that is convincing us that the correct source for this region error is through the TypeAscription on line 12 of the example.

It may also be notable that we end up going thru a code path that is commented as "unusual" when emitting this error.

This issue seems to be out of my area of expertise, so I won't claim it. @TaKO8Ki, if you can't find a solution then it may be best to hand off to someone with mir-borrowck expertise.

@compiler-errors
Copy link
Member

Actually, what I said was totally wrong. This is definitely because we're passing in the wrong Location when visiting consts here. I think I have a fix for this, though I probably want to eliminate other dummy locations we're passing around mir borrowck...

cc @TaKO8Ki sorry for all the pings, but don't keep wasting time on this unless you've made significant progress 😛

@rustbot claim

@rustbot rustbot assigned compiler-errors and unassigned TaKO8Ki Jun 12, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 21, 2022
…span, r=oli-obk

Fix erroneous span for borrowck error

I am not confident that this is the correct fix, but it does the job. Open to suggestions for a real fix instead.

Fixes rust-lang#97997

The issue is that we pass a [dummy location](https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_middle/mir/visit.rs.html#302) when type-checking the ["required consts"](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.Body.html#structfield.required_consts) that are needed by the MIR body during borrowck. This means that when we fail to evaluate the constant, we use the span of `bb0[0]`, instead of the actual span of the constant.

There are quite a few other places that use `START_BLOCK.start_location()`, `Location::START`, etc. when calling for a random/unspecified `Location` value. This is because, unlike (for example) `Span`, we don't have a dummy/miscellaneous value to use instead. I would appreciate guidance (either in this PR, or a follow-up) on what needs to be done to clean this up in general.
@bors bors closed this as completed in a7f789b Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints 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