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

SCIP index symbols for struct/enum/const/static/impl nested in functions do not include function name #18771

Open
mgsloan opened this issue Dec 27, 2024 · 6 comments
Labels
C-bug Category: bug

Comments

@mgsloan
Copy link
Contributor

mgsloan commented Dec 27, 2024

This is demonstrated in tests included in #18758

The reason I did not fix this in that PR is that hir representation only appears to know what module these definitions are in, and not whether they are contained in a function. I am not familiar enough with rust-analyzer internals to provide such information. The fix would involve a change to the enclosing_definition method or the methods it calls.

@mgsloan mgsloan added the C-bug Category: bug label Dec 27, 2024
@ChayimFriedman2
Copy link
Contributor

The information whether something is inside function (or anything with body) is whether the module is a block module (this exists in hir-def, but adding it to hir should be easy enough). The problem is that block modules usually have no names. Handling it can be done by special-casing block modules to look at the name of the enclosing definitions.

@mgsloan
Copy link
Contributor Author

mgsloan commented Dec 28, 2024

@ChayimFriedman2 Thanks for the info, glad this is in the representation. I've attempted to write a method to get the info, but got stuck going from BlockId to the containing FunctionId. Maybe I could find the function name in the syntax tree... But it would be much cleaner to get a FunctionId.

    pub fn function_parent(self, db: &dyn HirDatabase) -> Option<Function> {
        let containing_block = self.id.containing_block()?;
        let node = containing_block.lookup(db).ast_id.file_syntax(db);
    }

Would really appreciate it if on of you rust-analyzer experts would solve this part - feel free to push a commit to this branch adding such a function. I'm happy to make the moniker.rs changes based upon it.

@Veykril
Copy link
Member

Veykril commented Dec 29, 2024

I believe we don't actually have an association from block to its container, only block to its parent module (which may be a block again).

@ChayimFriedman2
Copy link
Contributor

Indeed; but we do preserve an association between a block and its ast::BlockExpr, and we can use that (ancestors_with_macros()) to find the container.

@mgsloan
Copy link
Contributor Author

mgsloan commented Dec 29, 2024

Thanks for the further info @ChayimFriedman2 ! I tried for about 15 minutes to sort out how to write the code for this by referencing this seemingly related code I found by searching for ancestors_with_macros.

I would prefer to not block merging #18758 on this concern. It takes SCIP's symbol information from being wildly out of spec to only having a few known flaws. If we are blocking merge on this concern, then I would really appreciate it if someone that knows rust-analyzer better than me could update enclosing_definition on my branch to return functions when things are nested within them, instead of skipping over the function definitions.

@ChayimFriedman2
Copy link
Contributor

I don't think we should block on that either.

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

No branches or pull requests

3 participants