Stream: t-compiler/wg-rls-2.0

Topic: foo::* support for merge assist


Piotr Szpetkowski (Mar 27 2020 at 12:13, on Zulip):

Hi, first time posting here, please be gentle.

I'm working on the https://github.com/rust-analyzer/rust-analyzer/issues/3728 issue and I've managed to gain some level of understanding of how things work on the side of RA (at the moment I also have a lot of trouble with comprehending rowan).

Now, I've an idea for the fix of the above issue, but I'm not sure if the approach is correct, so I'd be grateful if somebody could verify it. I think that the problem is that in the split_prefix function the UseTreeList instance doesn't have the STAR in its syntax. I suppose it might be related to https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_parser/src/grammar/items/use_item.rs#L22

This does not handle cases such as use some::path::*

A quick way to resolve the problem is as @matklad suggested on GH to simply translate use foo::bar::* to use foo::bar::{*}, so my idea here is to update the split_prefix function to take last_child_or_token of a given UseTree.syntax() and if the found NodeOrToken is of STAR kind I'd put the syntax into a string format, replace * with {*} and make an AST object -- a bit similarly to what make.rs::use_tree does.

I'm not sure if it's a correct approach e.g. maybe there is a simpler way to replace * with {*}? I'd be grateful if somebody could give me a push here to proper direction as I'm still very new to this codebase.

matklad (Mar 27 2020 at 12:36, on Zulip):

I don't think there's a need to change split_prefix function's signature. For use foo::bar::* case, the prefix argument would be foo::bar, and that should be enough to figure out that the desired result is use foo::bar::{*}

matklad (Mar 27 2020 at 12:37, on Zulip):

Specifically, I think None => return self.clone() branch should be change to look if there's self.has_star()

Piotr Szpetkowski (Mar 27 2020 at 13:11, on Zulip):

As far as I can tell, right now there isn't a scenario where split_path_prefix would return None, so imo it's not possible to reach the None => return self.clone() branch. Also the prefix is actually result of common_prefix call (in case of the merge_imports assist), so for use foo::bar::*, prefix passed to split_prefix is not necessarily foo::bar.

For example for a simple case of merging use std::foo::* and use std::bar it goes like this. Here's the prefix:

PATH@[5; 8)
  PATH_SEGMENT@[5; 8)
    NAME_REF@[5; 8)
      IDENT@[5; 8) "std"

And here is the suffix that will be returned by split_path_prefix (for the std::foo::* tree):

PATH@[0; 4)
  PATH_SEGMENT@[0; 4)
    NAME_REF@[0; 4)
      IDENT@[0; 4) "foo"
matklad (Mar 27 2020 at 13:19, on Zulip):

It returs None in this case: let parent = prefix.parent_path()?;

matklad (Mar 27 2020 at 13:19, on Zulip):

The ?

Piotr Szpetkowski (Mar 27 2020 at 13:27, on Zulip):

Ohh, sorry, missed the ? symbol while skimming through the code.

However in the above case of merging use std::foo::* and use std::bar the parent will have a value (at least that's what debugger has told me):

PATH@[5; 14)
  PATH@[5; 8)
    PATH_SEGMENT@[5; 8)
      NAME_REF@[5; 8)
        IDENT@[5; 8) "std"
  COLONCOLON@[8; 10) "::"
  PATH_SEGMENT@[10; 14)
    NAME_REF@[10; 14)
      IDENT@[10; 14) "foo"
matklad (Mar 27 2020 at 13:31, on Zulip):

Yep, that I think is a slightly different case from when the prefix is equal to the whole path.

I think for this case the fix should be applied around here:

let use_tree = make::use_tree(suffix.clone(), self.use_tree_list(), self.alias());

Here, we also need to preserve self.has_star() (which probably involves adding a boolean parameter to make::use_tree)

Piotr Szpetkowski (Mar 27 2020 at 13:40, on Zulip):

I see, makes sense. One last question for now - The way UseTree.has_star function works is that it simply fetches children of the given UseTree node and then searches for a T![*]? So if I'd have following tree:

USE_TREE@[5; 17)
  PATH@[5; 14)
    PATH@[5; 8)
      PATH_SEGMENT@[5; 8)
        NAME_REF@[5; 8)
          IDENT@[5; 8) "std"
    COLONCOLON@[8; 10) "::"
    PATH_SEGMENT@[10; 14)
      NAME_REF@[10; 14)
        IDENT@[10; 14) "cell"
  COLONCOLON@[14; 16) "::"
  STAR@[16; 17) "*"

it'd only search the set of direct descendants of the USE_TREE@[5; 17)? ( which here would bePATH@[5; 14), COLONCOLON@[14; 16) "::", STAR@[16; 17) "*")

matklad (Mar 27 2020 at 13:42, on Zulip):

Indeed, it will look only among direct children (and there can be at most one star among direct children)

Piotr Szpetkowski (Mar 27 2020 at 13:44, on Zulip):

Great, I finally understand something :D Thanks, I have everything I need for now, I might get back with more questions later on, but I'll do my best to solve what I can on my own.

matklad (Mar 27 2020 at 13:46, on Zulip):

:thumbs_up:

The workflow I'd recommend is

Piotr Szpetkowski (Mar 27 2020 at 13:50, on Zulip):

Well, that's basically what I do, I also use the vscode-lldb to trace the code flow as I'm learning what goes where, but so far I haven't found a way to get a format repr of a tree during debug session. Maybe it's just not possible atm.

Last update: Sep 30 2020 at 16:15UTC