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

type-mismatch when struct is defined in block expression #14443

Closed
jhgg opened this issue Mar 30, 2023 · 6 comments · Fixed by #14505
Closed

type-mismatch when struct is defined in block expression #14443

jhgg opened this issue Mar 30, 2023 · 6 comments · Fixed by #14505
Assignees
Labels
A-ty type system / type inference / traits / method resolution C-bug Category: bug

Comments

@jhgg
Copy link
Contributor

jhgg commented Mar 30, 2023

minimization:

trait T {}

fn dyn_t(d: &dyn T) {}

fn main() {
    struct A;
    impl T for A {}

    let a = A;

    let b = {
        struct B;
        impl T for B {}

        B
    };

    dyn_t(&a);
    dyn_t(&b);
    //    ^^ type-mismatch: expected &dyn T, found &B
}

this is minimized from code that prost generates: https://github.com/tokio-rs/prost/blob/34d0e2b309a1c8fa33871ccd8404573dc3b81b15/prost-derive/src/field/scalar.rs#L220 -> https://github.com/tokio-rs/prost/blob/34d0e2b309a1c8fa33871ccd8404573dc3b81b15/prost-derive/src/field/mod.rs#L149

rust-analyzer version: 0.4.1453-standalone
rustc version: rustc 1.66.1 (90743e729 2023-01-10)

@jhgg jhgg added the C-bug Category: bug label Mar 30, 2023
@Veykril Veykril added the A-ty type system / type inference / traits / method resolution label Mar 30, 2023
@Veykril
Copy link
Member

Veykril commented Mar 30, 2023

Oh, that is unfortunate. The trait implementation is done in the inner block, but observed in an outer block where it is not "visible". So this works as intended on our side.

Though this made me realize, block scopes are not part of rust-lang/rfcs#3373 so maybe we should revisit how we handle impl fetching for block def maps.

@jhgg
Copy link
Contributor Author

jhgg commented Mar 30, 2023

but observed in an outer block where it is not "visible". So this works as intended on our side.

Can you clarify what you mean by that? rustc is happy with this.

@Veykril
Copy link
Member

Veykril commented Mar 31, 2023

rustc is also happy with

trait T {}

fn dyn_t(d: &dyn T) {
    struct A;
}

fn foo() {
    impl T for A {}
}

fn main() {
    dyn_t(&a);
}

which r-a is not, though this case at least will most likely be rejectedi n the future by rustc as well.

Basically r-a currently assumes that trait impls are only visible if they are introduced in non-local scopes or block parent scopes. The reason for non-local scopes is incrementality, if we don't make this assumption we are required to parse all function, const and static bodies in all crates to know all trait implementations which would ruin our performance.

For local scopes we currently only consider ancestor block scopes for trait implementations due to simplicity. I am not sure how easy it would be for us to consider all block scopes in a body (though I am sure this would also have a perf impact since we now have to eagerly visit all block scopes upfront).

@Veykril
Copy link
Member

Veykril commented Mar 31, 2023

That is to annotate your example as to why r-a fails:

...
fn main() {
	...
    let b = {
        struct B;
        impl T for B {} // Introduces the trait impl for B locally here, making it only visibile in this block and its children (which of there are none)

        B
    };

    dyn_t(&a);
    dyn_t(&b); // demands B to impl T, but the impl is not visible to r-a here as it's introduced in a child block, not in this block or its parent (which does not exist)
    //    ^^ type-mismatch: expected &dyn T, found &B
}

@jhgg
Copy link
Contributor Author

jhgg commented Mar 31, 2023

I see - yeah, that makes sense. I do wonder if this is a common pattern enough though to special case, looking for traits in the same block that the struct itself was defined.

I do agree that impl of a trait in a different function entirely is much harder to incrementally analyze, and probably not very common at all.

@Veykril
Copy link
Member

Veykril commented Mar 31, 2023

I see - yeah, that makes sense. I do wonder if this is a common pattern enough though to special case, looking for traits in the same block that the struct itself was defined.

Now that you say that, we actually had this behavior before a week ago or so, previously we only checked the block scope of the implementing type and the defined trait. I "fixed" it by having it consider ancestor blocks instead, so clearly we can at least merge these two strategies which should cover 99.9% of all these cases then. cc https://github.com/rust-lang/rust-analyzer/pull/14424/files#diff-94608f32cdf930e839dffef675856b398d2dc412a3fd9607e3037267429cac6eL111-L157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ty type system / type inference / traits / method resolution C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants