Skip to content

fix(ty): should query impls in nearest block #13897

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

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jan 6, 2023

fix #13895

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2023
@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2023
@bvanjoi bvanjoi force-pushed the nearest-block-search branch from c564065 to 9a15cc8 Compare January 10, 2023 02:30
@lowr
Copy link
Contributor

lowr commented Jan 10, 2023

Thanks for working on this, but I think the problem is much broader. As I commented in the original issue, impls are treated as top-level items in a module even if they are declared inside local blocks, unlike any other kind of items. So we should be able to resolve any associated functions/consts in any impls as long as the the assocs are visible from current module. Take a look at this playground, which rust-analyzer wouldn't be able to resolve the called methods even with this PR (yes I'm being pedantic but it's valid Rust that we should be able to analyze after all :/).

I think this is more of an architectural issue around hir rather than hir-ty. To correctly resolve associated items, we should look into every block in a crate to find impls, but I still haven't got clear vision of how to implement that without breaking current incremental model much.

(This can be a temporary solution that solves most cases though. I'm curious why Rust decided to treat impls differently)

@Veykril
Copy link
Member

Veykril commented Jan 10, 2023

We don't support those kinds of impls globally intentionally, because doing so would be way too expensive and as you've said, breaks incremental. That kind of code where you split impls across different blocks is questionable in the first place and should be rare to encounter.

@lowr
Copy link
Contributor

lowr commented Jan 10, 2023

Makes sense, do we document it anywhere? I think it's worth noting.

@Veykril
Copy link
Member

Veykril commented Jan 10, 2023

I don't think it is documented anywhere (only mentioned in some issue somewhere)

@lowr
Copy link
Contributor

lowr commented Jan 10, 2023

Got it, I might go about documenting it somewhere. Thanks for the clarification and sorry for the noise!

@Veykril
Copy link
Member

Veykril commented Jan 10, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 10, 2023

📌 Commit 9a15cc8 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 10, 2023

⌛ Testing commit 9a15cc8 with merge 75877d7...

@bors
Copy link
Contributor

bors commented Jan 10, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 75877d7 to master...

@bors bors merged commit 75877d7 into rust-lang:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When defining types in a function, use statement in impl functions causes unrelated type inference to fail
5 participants