Skip to content

Conversation

@Veykril
Copy link
Member

@Veykril Veykril commented Dec 9, 2020

This required a lot of code unfortunately, mainly because it requires a lot of special casing grabbing specific syntax children depending on what kind of Node is being dealt with. I hope the comments will aid a bit in understanding/reviewing.

With this everything should be supported in reference search now I believe.

Lifetime renaming will be enabled in a follow up PR.

@kjeremy
Copy link
Contributor

kjeremy commented Dec 9, 2020

Does this have an effect on rename?

@Veykril
Copy link
Member Author

Veykril commented Dec 9, 2020

I wanted to address the rename stuff in a separate PR but I think this might already have an effect on it, I'll check in a bit if it does.

@Veykril
Copy link
Member Author

Veykril commented Dec 9, 2020

Renaming actually also just works as is. Except for the check whether the new name is a lifetime. Changing that is not quite as straight forward as I thought though so I will put that up for a follow up PR after all as to not make this PR even larger than it already is.

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

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.

In particular, I think we should start with making goto definition work for labels&lifetimes in this example:

fn t<'a>(xs: &'a i32) {
    'b: loop {
        break 'b;
    }
}

once goto definition works, reference search we should get automatically.

ast::ContinueExpr(_it) => find_all_label_references(position, lifetime_token, &parent),
_ => None,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

given that we don't make use of well-typed is, we might just match parent.kind() and fold some args together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, seems like this can be factored a bit better?

enum LifetimeClass {
    LifetimeParam, Label
}

impl LifetimeClass {
  fn classify(lt: ast::Lifetime) -> LifeTimeClass. 
}

@Veykril
Copy link
Member Author

Veykril commented Dec 13, 2020

Gonna reopen the rewritten PR in a bit so I'll close this here

@Veykril Veykril closed this Dec 13, 2020
bors bot added a commit that referenced this pull request Dec 17, 2020
6907: Lifetime reference search  r=matklad a=Veykril

PR #6787 but rewritten to make use of the HIR now. This only applies to Lifetimes, not labels. Also Higher-Ranked Trait Bounds aren't supported yet, but I feel like this PR is big enough as is which is why I left them out after noticing I forgot about them.

Supporting renaming required slight changes in the renaming module as lifetime names aren't allowed for anything but lifetimes(and labels) and vice versa for normal names.

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
@Veykril Veykril deleted the ref-search-lt branch March 4, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants