Stream: t-compiler/rust-analyzer

Topic: #6787 lifetime reference search support


Lukas Wirth (Dec 10 2020 at 15:12, on Zulip):

@matklad regarding your comment for the PR

The search code feels like something that should be done at the hir level, but is done in the IDE instead. I think we should be able to re-use the generic "find text&resovle" infrastructure here.

I am unsure what exactly you mean with that, are you referring to the symbol_index infrastructure?

Regarding the other comment you propose handling this similar to how NameRefs or Names are handled in goto_definition and the like if I understood this correctly?

matklad (Dec 10 2020 at 15:13, on Zulip):

@Lukas Wirth so the way search works is that you only need to implement goto definition.

matklad (Dec 10 2020 at 15:13, on Zulip):

And then the serach should work magially

matklad (Dec 10 2020 at 15:14, on Zulip):

Because the default impl for "search references to foo" just lists all the refrences, and checks which of them resolve to foo

matklad (Dec 10 2020 at 15:15, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ide_db/src/search.rs#L282

matklad (Dec 10 2020 at 15:15, on Zulip):

&def == self.def is the main trick

matklad (Dec 10 2020 at 15:16, on Zulip):

So, if NameRefClass::classify works for lifetimes, than search should also work

Lukas Wirth (Dec 10 2020 at 15:17, on Zulip):

But lifetimes aren't NameRefs, they are solely lifetime tokens nested in various other nodes.

matklad (Dec 10 2020 at 15:18, on Zulip):

:thinking:

Lukas Wirth (Dec 10 2020 at 15:19, on Zulip):
Name =
  'ident'

NameRef =
  'ident' | 'int_number'

https://github.com/rust-analyzer/ungrammar/blob/master/rust.ungram#L23-L27

Lukas Wirth (Dec 10 2020 at 15:19, on Zulip):

That's part of why my PR is so big and well, kind of messy to be fair :sweat_smile:

matklad (Dec 10 2020 at 15:20, on Zulip):

Hm, so there are two points here:

matklad (Dec 10 2020 at 15:20, on Zulip):

But it's unclear if we should extend NameClass and NameRefClass to handle lifetimes...

matklad (Dec 10 2020 at 15:22, on Zulip):

I guess, we can take two paths:

    LABEL@5617..5620
      NAME@...
        LIFETIME@5617..5619 "\'a"
      COLON@5619..5620 ":"
matklad (Dec 10 2020 at 15:22, on Zulip):

(which would require distinguising between label definitions and label usages...

matklad (Dec 10 2020 at 15:22, on Zulip):

(ah, we already do that)

matklad (Dec 10 2020 at 15:23, on Zulip):
matklad (Dec 10 2020 at 15:23, on Zulip):

That is, adding something like NameRefClass::classify_lifetime in addition to just classify

Jonas Schievink [he/him] (Dec 10 2020 at 15:25, on Zulip):

Labels are hygienic by the way, so Name{Ref} seems appropriate

matklad (Dec 10 2020 at 15:25, on Zulip):

Without thinking too much about this, I'd keep lifetimes as a separate syntactic category from names, but treat them the same at the NameClass layer

matklad (Dec 10 2020 at 15:26, on Zulip):

But @Jonas Schievink 's point is a good one, yeah...

matklad (Dec 10 2020 at 15:26, on Zulip):

The intuiton behind NameClass/Ref is "which you can fine-refernces from/goto definition to"

matklad (Dec 10 2020 at 15:26, on Zulip):

So there's a bit of type-erasure happening there.

matklad (Dec 10 2020 at 15:27, on Zulip):

I guess, even if we go with ast change, we still need NameClass change, so it makes sense to start with the second approach

matklad (Dec 10 2020 at 15:28, on Zulip):

I have doubts about ast::Name for lifetimes because, with inline lifetimes (propsed feature) like fn foo(r: &'a str) -> 'a we loose syntactic distinciton between names and name refs for lifetimes anyway.

Lukas Wirth (Dec 10 2020 at 15:30, on Zulip):

So for now I should just go with the Name{Ref}Class::classify_lifetime approach then? It seems better for now in general to me as well as if the ast::Name{Ref} could as you said be done later on still

matklad (Dec 10 2020 at 15:31, on Zulip):

Yup!

matklad (Dec 10 2020 at 15:31, on Zulip):

You'll need to add those two functions

matklad (Dec 10 2020 at 15:31, on Zulip):

and you need to modify reference search code in places where it does match_ast! { Name | NameRef} to also handle Lifetime

Lukas Wirth (Dec 10 2020 at 15:32, on Zulip):

Just to make sure I got it right, NameClass would be used for(in this case) label/lifetime definitions and NameRefClass for those that well reference labels/lifetimes?

matklad (Dec 10 2020 at 15:33, on Zulip):

Exactly!

Lukas Wirth (Dec 10 2020 at 15:33, on Zulip):

Aight :thumbs_up:

matklad (Dec 10 2020 at 15:34, on Zulip):

(and in the future with inline lifetimes, we'd have a funky NameClass which stores a bunch of lifetime references)

matklad (Dec 10 2020 at 15:35, on Zulip):

(we actually should have the same today for patterns like (Some(x), None) | (None, Some(x)). Here, x is a NameClass which is repred by two ast::Names

matklad (Dec 10 2020 at 15:35, on Zulip):

the fact that we don't do this is a bug, )

Lukas Wirth (Dec 10 2020 at 15:42, on Zulip):

Oh interesting I never thought about the fact that Or patterns actually introduce a name with multiple definition locations

Lukas Wirth (Dec 10 2020 at 18:36, on Zulip):

Hm so if I see this right this requires me to add lifetime/label to ide_db::Definition as NameClass has a function to always return such a Definition, would that in turn require me to represent lifetimes in the hir::code_model?

Lukas Wirth (Dec 10 2020 at 18:39, on Zulip):

or rather would it be a good idea/is it even possible to represent lifetimes and labels in the code_model there

matklad (Dec 10 2020 at 18:46, on Zulip):

Yes, you'd need to add Definition

matklad (Dec 10 2020 at 18:47, on Zulip):

And adding lifetimes to code model is also something which we should do long-term

Lukas Wirth (Dec 10 2020 at 19:55, on Zulip):

Then I'll first dig around and see if I can try adding lifetimes to the code model first, touching the hir sounds like a fun task in general as I haven't worked much with it yet. Will follow up with my original PR afterwards then

matklad (Dec 10 2020 at 19:57, on Zulip):

:thumbs_up:

Lukas Wirth (Dec 11 2020 at 09:57, on Zulip):

What exactly is the different between hir::Ty and hir::TypeRef? Trying to figure out how to I should model Lifetimes in hir by looking at how the rest is modeled but that those two confuse me a bit

matklad (Dec 11 2020 at 10:00, on Zulip):

The main difference is that it's ty::Ty and def::TypeRef.

matklad (Dec 11 2020 at 10:01, on Zulip):

Ty is a type. TypeRef is more like a type "syntax", it is undersolved

matklad (Dec 11 2020 at 10:02, on Zulip):

That is, in

mod a {
    struct Foo;
    fn f(_: Foo) {}
}

mod b {
    struct Foo;
    fn f(_: Foo) {}
}
matklad (Dec 11 2020 at 10:02, on Zulip):

The Tys of two paramers are different

matklad (Dec 11 2020 at 10:02, on Zulip):

But TypeRef's are the same

matklad (Dec 11 2020 at 10:03, on Zulip):

I guess, a short way to say that is:

Ty is a fully-resolved type, TypeRef is an AST for a type syntax node.

Florian Diebold (Dec 11 2020 at 10:14, on Zulip):

Yeah. Also, Ty can represent types that can only come up during type inference and can't be written in Rust syntax, e.g. inference variables, function types and lots more

Lukas Wirth (Dec 11 2020 at 10:19, on Zulip):

Ah I see now, thanks

Florian Diebold (Dec 11 2020 at 10:21, on Zulip):

Ty will at some point be completely replaced by Chalk's Ty, though we may have another intermediate form that's like a TypeRef but with resolved names. For lifetimes, I guess we will need to have a LifetimeRef that's just a name, and then a resolved Lifetime that's either Static or Parameter(LifetimeParamId)

Florian Diebold (Dec 11 2020 at 10:23, on Zulip):

or maybe we don't need the unresolved form yet

Florian Diebold (Dec 11 2020 at 10:24, on Zulip):

we don't need to worry about the type-system / borrow-checker level representation since we won't get into that before completely moving to the Chalk representation for that

Lukas Wirth (Dec 11 2020 at 10:26, on Zulip):

As in integrating lifetimes into Ty?

Florian Diebold (Dec 11 2020 at 10:28, on Zulip):

yeah

Lukas Wirth (Dec 11 2020 at 10:29, on Zulip):

Alright

Lukas Wirth (Dec 11 2020 at 10:58, on Zulip):

Florian Diebold said:

we don't need to worry about the type-system / borrow-checker level representation since we won't get into that before completely moving to the Chalk representation for that

Lukas Wirth (Dec 11 2020 at 10:58, on Zulip):

I guess this also refer to the TypeBounds and such, as in T: 'a?

Florian Diebold (Dec 11 2020 at 11:08, on Zulip):

Yeah

Florian Diebold (Dec 11 2020 at 11:10, on Zulip):

well actually TypeBound is the equivalent of TypeRef, so we'll want to have lifetimes in there at some point, I don't know if it's necessary for your purposes though

Florian Diebold (Dec 11 2020 at 11:20, on Zulip):

if we do it similarly to types, we'll probably want a resolve_lifetime method on Resolver that can then be directly used by the code model stuff

Florian Diebold (Dec 11 2020 at 11:24, on Zulip):

so, the main thing to do actually seems to be adding lifetimes to GenericParams, giving them IDs, and implementing resolution for them in Resolver

Lukas Wirth (Dec 11 2020 at 13:17, on Zulip):

Alright, pushed a PR with the changes I have for now https://github.com/rust-analyzer/rust-analyzer/pull/6818
This doesn't contain Resolver stuff yet as I'll have to see how I'd use that here first

Lukas Wirth (Dec 13 2020 at 16:54, on Zulip):

So I need to change the SourceMap definition that that GenericDefId returns for the HasChildSource trait, which currently is

type SourceMap = ArenaMap<LocalTypeParamId, Either<ast::Trait, ast::TypeParam>>;

I need this to also support Lifetimes now, but Im unsure how I can manage that since I cannot have a key type for the ArenaMap that represents GenericParameters overall

Lukas Wirth (Dec 13 2020 at 16:54, on Zulip):

at least not the way they are handled currently

Lukas Wirth (Dec 13 2020 at 16:55, on Zulip):

Since lifetime and type parameters are stored seperately in two different arenas

matklad (Dec 13 2020 at 16:55, on Zulip):

type SourceMap = (ArenaMap<...>, ArenaMap<...>)?

matklad (Dec 13 2020 at 16:55, on Zulip):

probably needs a name as well :D

Lukas Wirth (Dec 13 2020 at 16:55, on Zulip):

HasChildSource requires its return type to be a single ArenaMap unfortunately

Lukas Wirth (Dec 13 2020 at 16:56, on Zulip):

which is the problem Im running into :big_smile:

Lukas Wirth (Dec 13 2020 at 16:56, on Zulip):
pub trait HasChildSource {
    type ChildId;
    type Value;
    fn child_source(&self, db: &dyn DefDatabase) -> InFile<ArenaMap<Self::ChildId, Self::Value>>;
}
matklad (Dec 13 2020 at 16:57, on Zulip):

I think this is a non-essential trait

Lukas Wirth (Dec 13 2020 at 16:58, on Zulip):

Ye Im looking through its usage right now I don't think this impl is really required to exist here

matklad (Dec 13 2020 at 16:58, on Zulip):

in a sense that ChildBySource is the one you need to impl

Lukas Wirth (Dec 13 2020 at 16:58, on Zulip):

For GenericDefId its sole usage seems to be the ChildBySource yep

matklad (Dec 13 2020 at 16:58, on Zulip):

and HasChildSource should be only a shortcut for that

Lukas Wirth (Dec 13 2020 at 16:59, on Zulip):

Alright, guess that answers the question :thumbs_up:

matklad (Dec 13 2020 at 16:59, on Zulip):

(tbh, I don't remember how that child source infra is setup exactly. I feel like it could be simplified considerably)

matklad (Dec 13 2020 at 17:00, on Zulip):

But this is also one of the more fun bits of rust-analyzer. The ultimate target of that infra is source_to_def.rs module, and that's the magic that binds IDE and compiler together

Lukas Wirth (Dec 13 2020 at 21:27, on Zulip):

Aight opened one PR for some hir additions https://github.com/rust-analyzer/rust-analyzer/pull/6862
HasChildSource is actually being used by one other thing which is the HasSource impl for TypeParams, so I replaced it with a normal method instead. I've annotated that part in the PR as a review as I don't think its the best option.

matklad (Dec 14 2020 at 14:08, on Zulip):

@Lukas Wirth re HasChildSource, would chaging ChildId from assoc type to a trait's type paramter fix the problem?

matklad (Dec 14 2020 at 14:09, on Zulip):

but the current impl also looks OK enough

Lukas Wirth (Dec 14 2020 at 14:58, on Zulip):

That might work, I can try testing that out after the PR, don't wanna change too much now.

matklad (Dec 14 2020 at 14:59, on Zulip):

sgtm!

matklad (Dec 14 2020 at 15:00, on Zulip):

r=me with FIXMEs added for the follow up items (chaing HasChildSource and using ast::Lifetime)

Lukas Wirth (Dec 14 2020 at 16:38, on Zulip):

Moving ChildId to a generic param worked flawlessly :thumbs_up:
Regarding typing lifetimes in the ast, I wonder if making it a node would be a nice idea? In that case we could have a Lifetime node and a Labelnode right?

matklad (Dec 14 2020 at 16:40, on Zulip):

I think we need a node here, and not a token, yeah

matklad (Dec 14 2020 at 16:40, on Zulip):

Sort of how ast::Name wraps ident token, ast::LIfetime would wrap Lifetime token

matklad (Dec 14 2020 at 16:41, on Zulip):

But we probably don't need to distinguish between Names and NameRefs for lifetimes (due to that future feature with implicit introduction of lifetimes)

Lukas Wirth (Dec 14 2020 at 17:01, on Zulip):

Oh right, Label already exists as a node, though break and continue don't use the node but the lifetime token only.
What should we do about the SyntaxKind though, because both the node and token would want the ident LIFETIME in this case right? :thinking:

matklad (Dec 14 2020 at 17:02, on Zulip):

But lable includes :, no?

matklad (Dec 14 2020 at 17:03, on Zulip):

I guess we can rename token to lifetime_ident

matklad (Dec 14 2020 at 17:03, on Zulip):

because it is a kind of ident

Lukas Wirth (Dec 14 2020 at 17:03, on Zulip):

https://github.com/rust-analyzer/ungrammar/blob/master/rust.ungram#L457-L460

Label =
  'lifetime'

doesn't seem to be the case

matklad (Dec 14 2020 at 17:04, on Zulip):

that's a bug I think

matklad (Dec 14 2020 at 17:04, on Zulip):
LOOP_EXPR@840..882
  LABEL@840..843
    LIFETIME@840..842 "\'a"
    COLON@842..843 ":"
  WHITESPACE@843..844 " "
  LOOP_KW@844..848 "loop"
  WHITESPACE@848..849 " "
  BLOCK_EXPR@849..882
    L_CURLY@849..850 "{"
    WHITESPACE@850..863 "\n            "
    EXPR_STMT@863..872
      BREAK_EXPR@863..871
        BREAK_KW@863..868 "break"
        WHITESPACE@868..869 " "
        LIFETIME@869..871 "\'a"
      SEMICOLON@871..872 ";"
    WHITESPACE@872..881 "\n        "
    R_CURLY@881..882 "}"
Lukas Wirth (Dec 14 2020 at 17:04, on Zulip):

Ye I just realized the grammar doesn't specify the colon at all for loop labels

matklad (Dec 14 2020 at 17:05, on Zulip):

Feel free to send a PR for that (bumping minor in Cargo.toml to auto-publish)

Lukas Wirth (Dec 14 2020 at 17:14, on Zulip):

for the colon or the colon as well as lifetime nodes?

matklad (Dec 14 2020 at 17:14, on Zulip):

For the colon definitelly. And for the lifetime node, once you are happy with the naming scheme

Lukas Wirth (Dec 14 2020 at 17:18, on Zulip):

lifetime_ident seems good to me, fits well together with ident, https://github.com/rust-analyzer/ungrammar/pull/15

Last update: Jul 27 2021 at 21:30UTC