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

Build ItemTrees for block expressions to handle inner items #8911

Closed
jonas-schievink opened this issue May 21, 2021 · 3 comments · Fixed by #10863
Closed

Build ItemTrees for block expressions to handle inner items #8911

jonas-schievink opened this issue May 21, 2021 · 3 comments · Fixed by #10863
Labels
A-nameres name, path and module resolution E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now

Comments

@jonas-schievink
Copy link
Contributor

Currently, we lower inner items while building the ItemTree for a file. This is buggy and error-prone, because we just syntactically descend into all top-level nodes and treat any nested items as the inner items of the top-level node. For example:

fn f() {
    mod m {
        fn g() {}
    }
}

Here g will be registered as an inner item of f, but that's clearly wrong, only m should be an inner item.

This also happens when an inner trait is present: its methods are treated as inner items of the containing item:

fn f() {
    trait Tr {
        fn method(&self);
    }
}

This causes bogus completions and wrong name resolution results:

screenshot-2021-05-21-18:46:17

My idea for fixing this is to not store inner items in the file's ItemTree at all. Instead, we build an ItemTree for all items in a block expression when needed. This doesn't have to go through its own query either, because these trees are only built when the containing file is open. (an added long-term bonus of this change is that we can build a file's ItemTree without parsing item bodies, once the rest of r-a can deal with that)

Last time I tried implementing this it caused a hang in the testsuite, so there's probably some bug in how we traverse inner items while lowering bodies, or something's wrong with the container we choose for item locations.

Implementing this should also fix this bug (might be related to the hang?):

https://github.com/rust-analyzer/rust-analyzer/blob/de403b10448e23f232804596538de92fc57203d6/crates/hir_def/src/body/tests/block.rs#L156-L157

@jonas-schievink jonas-schievink added E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now A-nameres name, path and module resolution labels May 21, 2021
@lf-
Copy link
Contributor

lf- commented Jun 14, 2021

Was this fixed by #9244?

@jonas-schievink
Copy link
Contributor Author

No, that mostly involved hir_ty

@matklad
Copy link
Member

matklad commented Jun 14, 2021

Instead, we build an ItemTree for all items in a block expression when needed. This doesn't have to go through its own query either, because these trees are only built when the containing file is open. (an added long-term bonus of this change is that we can build a file's ItemTree without parsing item bodies, once the rest of r-a can deal with that)

food for thought: zig went in the opposite direction, from lazy lowering to whole file lowering: https://ziglang.org/download/0.8.0/release-notes.html#Whole-File-AST-Lowering. In a language without macros, I actually see a lot of merit in “pre-parsing” file into convenient form in one go. In a language with macros, I don’t know, I want to 🙈 and pretend they don’t exist :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-nameres name, path and module resolution E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants