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

fix: Goto implementation to impls inside blocks #16812

Merged
merged 1 commit into from Mar 19, 2024

Conversation

ShoyuVanilla
Copy link
Contributor

Fixes #3739

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2024
Comment on lines 270 to 221
// Collect nested impls inside function bodies, etc.
for (_, ns) in module_data.scope.entries() {
for module in ns.types.iter().map(|t| t.0).chain(ns.values.iter().map(|t| t.0)) {
let body: Option<DefWithBodyId> = match module {
ModuleDefId::FunctionId(it) => Some(it.into()),
ModuleDefId::EnumVariantId(it) => Some(it.into()),
ModuleDefId::ConstId(it) => Some(it.into()),
ModuleDefId::StaticId(it) => Some(it.into()),
_ => None,
};
if let Some(body) = body {
let body = db.body(body);
for (_, block_def_map) in body.blocks(db.upcast()) {
Self::collect_def_map(db, map, &block_def_map, query_depth);
}
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is quite verbose, but I couldn't find any good ways that can probe into those blocks 🤔

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We intentionally do not look into all blocks there are, as an IDE we try to be lazy and only analyze the crate structure on a global scale, which allows us to skip out on having to look into every body which would slow us down immensely. So supporting local impls like this isn't really possible.

So what we should do instead to make the first case work is have goto_implementation look into the block defmaps of the ADT/trait definition and iterate those. We can query the relevant impls via the *_impls_in_block queries (check for uses of those queries, that should show you how to use thisI think, if not just ask)

}

#[test]
fn goto_implementation_in_nested_blocks_2() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test we cannot support due to performance reasons, see the main review message

}

#[test]
fn goto_implementation_in_nested_blocks_1() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This we can support

@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 Mar 12, 2024
@ShoyuVanilla ShoyuVanilla marked this pull request as draft March 14, 2024 16:32
@ShoyuVanilla
Copy link
Contributor Author

I'm seeking some performant implementation; is there any way to find BlockId with current cursor location? 🤔

@Veykril
Copy link
Member

Veykril commented Mar 15, 2024

You can turn block expression ast nodes into blocks via

pub(super) fn block_to_def(&mut self, src: InFile<ast::BlockExpr>) -> Option<BlockId> {
(Note that will only yield Some for blocks that actually have local items or macro calls in them due to perf optimizations)

Though I tihnk it should be enough to just query the modules of the trait / ADT in question as those should be block modules if they are defined in blocks?

@ShoyuVanilla
Copy link
Contributor Author

Thank you! I'll look into that point 😄

@ShoyuVanilla ShoyuVanilla marked this pull request as ready for review March 19, 2024 13:05
@Veykril
Copy link
Member

Veykril commented Mar 19, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Mar 19, 2024

📌 Commit 967a864 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 19, 2024

⌛ Testing commit 967a864 with merge e03df77...

@bors
Copy link
Collaborator

bors commented Mar 19, 2024

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

@bors bors merged commit e03df77 into rust-lang:master Mar 19, 2024
11 checks passed
@ShoyuVanilla ShoyuVanilla deleted the issue-3739 branch March 19, 2024 13:32
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.

'0 implementations' on type nested in fn
4 participants