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

Support local imports (use statements in blocks) #1165

Closed
suhr opened this issue Apr 19, 2019 · 25 comments · Fixed by #7631
Closed

Support local imports (use statements in blocks) #1165

suhr opened this issue Apr 19, 2019 · 25 comments · Fixed by #7631
Labels
E-hard S-actionable Someone could pick this issue up and work on it right now

Comments

@suhr
Copy link

suhr commented Apr 19, 2019

Inside a function, I often write something like this:

use Request as R;

match ... {
    R::Info => {...},
    ...
}

But RA seems to have a problem with such imports:

Failed to exactly resolve the symbol. This is probably because rust_analyzer does not yet support traits.
These items were found instead:

Request
@edwin0cheng
Copy link
Member

Yeah, Seem like current ra_hir::expr::Body do not store local items information :

pub struct Body {
    /// The def of the item this body belongs to
    owner: DefWithBody,
    exprs: Arena<ExprId, Expr>,
    pats: Arena<PatId, Pat>,
    /// The patterns for the function's parameters. While the parameter types are
    /// part of the function signature, the patterns are not (they don't change
    /// the external type of the function).
    ///
    /// If this `Body` is for the body of a constant, this will just be
    /// empty.
    params: Vec<PatId>,
    /// The `ExprId` of the actual body expression.
    body_expr: ExprId,
}

An here is a simplifed test case for this bug:

enum Request {
    Info
}

fn main() {
    use Request as R;    
    let _r = R::Info;
}
hover on _r = {unknown}

@matklad
Copy link
Member

matklad commented Apr 19, 2019

cc @pnkfelix, supporting local items (\w imports) and laziness is going to be a major design constarint on the name resolution library.

The current code in rust-analyzer just completely ignores local items and builds a Resolver instance for top-level scope.

We can ask the Resolver to process item bodies at the same time as it processes modules, but that would kill laziness.

Instead, I hope that for each function we can create a separate Resolver instance on-demand which references the parent resolver. That is, each function gets its own independent resolver, but they all share the same module-level scope

@ice1000
Copy link
Contributor

ice1000 commented Nov 27, 2019

Is this planned to be implemented (yet)?

@matklad
Copy link
Member

matklad commented Nov 28, 2019

This is not planned for the nearest future

@flodiebold flodiebold changed the title Support local imports Support local imports (use statements in blocks) Jul 12, 2020
@oblitum
Copy link
Contributor

oblitum commented Jul 14, 2020

Just avoided creating another duplicate of this... Given the amount of refs and duplicates this looks like top priority.

@Dushistov
Copy link

Dushistov commented Jul 21, 2020

use Enum::* inside function also covered by this issue?
Or it is covered by #4749 ?

@ice1000
Copy link
Contributor

ice1000 commented Jul 21, 2020

use Enum::* inside function also covered by this issue?
Or it is covered by #4749 ?

I guess the answer is "both"

@flodiebold
Copy link
Member

Any use inside a function does not work at all, because of this issue. #4749 is a specific issue with an assist.

@MikailBag
Copy link
Contributor

If RA desugars each function with local imports into a module with use and function within it, will it work?

@lnicola
Copy link
Member

lnicola commented Sep 23, 2020

@MikailBag you can have use statements at scope block level.

@flodiebold
Copy link
Member

It's not that simple, the anonymous modules created by block-level items work differently from named modules (in that everything from the containing named module stays in scope).

@flodiebold
Copy link
Member

Also, the main complication here is not actually implementing the logic, but doing it in a way that doesn't require us to parse all function bodies all the time.

@ice1000
Copy link
Contributor

ice1000 commented Sep 23, 2020

If RA desugars each function with local imports into a module with use and function within it, will it work?

This will produce significant large amount of modules, which is no good.

@matklad
Copy link
Member

matklad commented Sep 24, 2020

Yeah, the #5922 is still how I think we should proceed here. Basically, we first should refactor existing code to be more modular, and than we should add support for local modules.

PRs welcome, but be warned that this is a large task. I'd estimate it as 1-2 weeks full-time for someone who is already familiar with the relevant infra.

At the same time, I wouldn't worry to much about "core design" here. I think both of these are true:

  • the whole DefCollector thing needs to be rewritten
  • this won't affect code outside of the name resolution, as the interface is isolated enough.

Also, +1 to this comment regarding the language :-)

@djc
Copy link

djc commented Sep 24, 2020

At the risk of dragging this on more, I just want to make the point that from the perspective of this particular user, false positive compilation errors would have the most positive impact on my experience using RA -- substantially more than refactoring tools, search & replace tools, further performance improvements or more code sharing. Of course this disregards the experience of RA contributors and wider ecosystem benefits from things like code sharing, but I feel like it should count for something.

I don't know if/how RA prioritizes work currently -- making that more transparent might also help in the sense of making it more concrete what trade-offs are affecting issues like this. Also, this is the kind of thing where I wonder if a focused fundraising effort could help -- I for one would gladly contribute some cash to help prioritize this, but I don't think my one-man business can currently sustain paying for the entirety of 1-2 person-weeks of work.

@ice1000
Copy link
Contributor

ice1000 commented Sep 24, 2020

For wording, my initial intention was to bring some "ping"-like comments so there can be some discussion. This issue is so important but is only widely mentioned, and is on page 3 when sorting issues with "most discussed". I apologize for the moronic words though, I have made some attempts on trying to make this work (like introducing block-modules) before, but they turns out to be not good design, so I'm aware of the difficulty of the task.

Thanks for everyone who have made RA so great!

mrk-its pushed a commit to mrk-its/bevy that referenced this issue Oct 6, 2020
@jonas-schievink jonas-schievink added E-hard S-actionable Someone could pick this issue up and work on it right now labels Jan 16, 2021
bors bot added a commit that referenced this issue Jan 18, 2021
7336: Rename `CrateDefMap` to `DefMap` r=matklad a=jonas-schievink

I propose handling local items by computing a `DefMap` for every block expression, using the regular (early) name resolution algorithm. The result of that will be a `DefMap` that has a reference to the parent `DefMap`, which is either the one computed for the containing block expression, or the crate's root `DefMap`. Name resolution will fall back to a name in the parent `DefMap` if it cannot be resolved in the inner block.

The `DefMap`s computed for block expressions will go through a separate query that can be garbage-collected much more aggressively, since these `DefMap`s should be cheap to compute and are never part of a crate's public API.

The first step towards that is to make `CrateDefMap` not specific to crates anymore, hence this rename (if this plans sounds reasonable).

cc #7325 and #1165

Co-authored-by: Jonas Schievink <[email protected]>
@emilk
Copy link

emilk commented Feb 5, 2021

I'd just like to chime in with some motivation on why I think supporting local use statements is an important issue:

Increasingly I only put a use statement at the top of a file if the imported symbol will be used throughout the file. Instead I prefer my use-statements inside my functions. I find that this improves the readability of my code as it makes it more obvious where a certain symbol came from (without the reader having to scroll to the top of the file). But maybe even more importantly it improves refactorability (yes, it is a word). Earlier when moving code between files I needed to spend a lot of time figuring out which use statements (and before that #includes) to copy and/or move to the other file. When the use statements are part of the function body, that work is almost completely gone.

In short: just like I try to limit the scope of my variables, I try to limit the scope of my use statements.

bors bot added a commit that referenced this issue Feb 10, 2021
7631: Add test for #1165 r=jonas-schievink a=jonas-schievink

Closes #1165

bors r+

Co-authored-by: Jonas Schievink <[email protected]>
@jonas-schievink
Copy link
Contributor

This is now implemented and was fixed by #7614 / #7627.

There are still some known bugs and missing parts, like local impls (which should at least work for types that are also local), and some remaining cleanups to be done, but the bulk of the work is now done, so closing.

@umanwizard
Copy link
Contributor

It's so beautiful...

image

Thanks @jonas-schievink ! this has been my most-desired missing feature in rust-analyzer for such a long time!

@djc
Copy link

djc commented Feb 10, 2021

@jonas-schievink woo! Thanks so much for picking this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard 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.