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

Go to definition doesn't work for trait implementations for structs inside function bodies #14782

Closed
HKalbasi opened this issue May 11, 2023 · 7 comments · Fixed by #14855
Closed
Labels
A-ide general IDE features C-bug Category: bug

Comments

@HKalbasi
Copy link
Member

trait T {
    type Ty;
    fn foo() -> Self::Ty;
}

fn f() {
    struct X {}
    impl T for X {
        type Ty = i32;
        fn foo() -> i32 {
            5
        }
    }
    let t = X::foo();
}

In this example r-a sees method, and can infer that the type of t is i32, but go to definition on foo will go to the trait function, not the impl one.

@HKalbasi HKalbasi added the C-bug Category: bug label May 11, 2023
@Veykril Veykril added the A-ide general IDE features label May 11, 2023
@Veykril
Copy link
Member

Veykril commented May 11, 2023

In general, IDE features like these interacting with impls should be aware of block impls for the cursor position.

@HKalbasi
Copy link
Member Author

Why cursor position? I noticed this when trying to mir interpret some function in standard library which used this pattern, I guess cursor position wouldn't work for it. Isn't is possible to consider the block impls of the self type in lookup_impl_method?

@Veykril
Copy link
Member

Veykril commented May 11, 2023

We cannot consider all block implementations, for that we would need to look into all bodies which goes against the lazy analysis strategy we employ in r-a.

@Veykril
Copy link
Member

Veykril commented May 11, 2023

(Note cursor position in this case would be identifier we are invoking this go to on, so take cursor position as "relevant context position for the current task")

@Veykril
Copy link
Member

Veykril commented May 11, 2023

Oh I see, sorry. I assumed the go to def fuctionality was faulty. But this is the infernece results not recording properly. Point still stands, for resolving thefoo in X::foo we should consider impls in the surrounding block and all ancestors (which I relaly thought we already do?)

@HKalbasi
Copy link
Member Author

We cannot consider all block implementations, for that we would need to look into all bodies which goes against the lazy analysis strategy we employ in r-a.

This case is allowed in rust-lang/rfcs#3373 so we should support it, but I think it is possible to do it without breaking laziness.

for resolving thefoo in X::foo we should consider impls in the surrounding block and all ancestors (which I relaly thought we already do?)

I guess we do it in method resolution, but not in the lookup_impl_method function.

@Veykril
Copy link
Member

Veykril commented May 11, 2023

Indeed, it looks like lookup_impl_method is losing the local context, that is the block its used in, so we need to thread that through somehow. The callers also don't respect block impls for the current block for the passed trait environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ide general IDE features C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants