Stream: t-compiler/wg-rls-2.0

Topic: rename functions

Jean Mertz (Mar 04 2019 at 22:12, on Zulip):

I was wondering if there's a reason why function renaming isn't supported yet? Is it just a matter of priority? If so, I was looking into what it'd take to implement it, and one of the things I noticed is that find_node_at_offset returns None if I add a test for function renaming. Looking at the AST, the function name is of type NAME, and that check expects NAME_REF. Any pointers on what to do to make this work?

vipentti (Mar 05 2019 at 06:00, on Zulip):

I think one problem is that we don't currently handle project wide references, including functions. Meaning you cannot use find all references on a function currently, which is used when renaming non-modules.

vipentti (Mar 05 2019 at 06:45, on Zulip):

Tracking issue for project wide references can be found in rust-analyzer#200

Jean Mertz (Mar 05 2019 at 07:38, on Zulip):

Thanks for the pointer. Yeah, that seems like a big one to tackle.

Jean Mertz (Mar 05 2019 at 07:39, on Zulip):

Still though, I'm curious if you have any feedback on my question of NAME_REF vs NAME, what their differences are, and why the function signature returns NAME (which incidentally doesn't match in the link I gave above)?

matklad (Mar 05 2019 at 07:56, on Zulip):

@Jean Mertz there should be two cases for renaming:

renaming the declaration (where we do find references and then renaming)

renaming the reference (where we first need to to goto definition, and than find usages&rename)

matklad (Mar 05 2019 at 07:57, on Zulip):

I think that bit of code doesn't handle the first case, while it should

matklad (Mar 05 2019 at 07:57, on Zulip):

The test case would be something like this:

fn main() {
    let foo<|> = 92;
    foo + foo;
Jean Mertz (Mar 05 2019 at 09:40, on Zulip):

Ah. That makes sense. Thanks. I’ll look at this after work.

vipentti (Mar 05 2019 at 09:55, on Zulip):

I think you may be able to use find_node_at_offset::<ast::BindPat> directly in the non-NameRef case. and combining it with source_binder::function_from_child_node

vipentti (Mar 05 2019 at 10:00, on Zulip):

Or perhaps you first have to check that the node is Name before getting the bind pattern

Jean Mertz (Mar 05 2019 at 12:05, on Zulip):

I just took a quick peek, and that's already what's happening here. And indeed, I added a test and it passes, so this already works, it just doesn't work for functions yet (which is then – I suspect – related to project wide references not being a thing yet).

Jeremy Kolb (Mar 05 2019 at 14:45, on Zulip):

Yep it's because of project wide references. Note that renaming is broken into two steps at the LSP layer: a prepare step where we should do validation (and we currently don't really do any) and the actual rename operation

vipentti (Mar 05 2019 at 15:32, on Zulip):

What kind of validation should be done?

Jeremy Kolb (Mar 06 2019 at 13:22, on Zulip):

probably things like: is the input blank, is it a valid identifier, can we actually rename the thing we are on etc.

Last update: Jan 21 2020 at 12:10UTC