Stream: t-compiler/wg-rls-2.0

Topic: issue 2736


divma (Jan 20 2020 at 18:09, on Zulip):

Hi, I would like to work on this issue, so I'm looking for some help

divma (Jan 20 2020 at 18:11, on Zulip):

For now I am looking for the place where ItemScope gets populated to start understangin what it does/ how it is used

Florian Diebold (Jan 20 2020 at 20:02, on Zulip):

ItemScope gets populated by the DefCollector (in collector.rs), in particular resolved imports get added here

divma (Jan 21 2020 at 16:31, on Zulip):

Hi @Florian Diebold , thanks for the suggestion. Trying to better understand the issue I took the test code here ran `

divma (Jan 21 2020 at 16:33, on Zulip):

then ran cargo xtask install --server and made the setup in my editor (nvim) but what I get from what I see is that the name that gets resolved is the right one

divma (Jan 21 2020 at 16:33, on Zulip):

maybe there is something that I am not understanding correctly but I can't see it

matklad (Jan 21 2020 at 16:37, on Zulip):

I think this might be because we use approximate name-based matching for goto def, if we fail to infer type

matklad (Jan 21 2020 at 16:37, on Zulip):

Basically, as long as you have a single function named foo in the project, all calls to .foo() will resolve to it.

matklad (Jan 21 2020 at 16:38, on Zulip):

But, for example, asking for the type of the expression in the editor should yield unknown

divma (Jan 21 2020 at 16:44, on Zulip):

ok! if I hover Bar it gives the trait and not the struct
good to finally seeing it, thanks!

divma (Jan 22 2020 at 00:32, on Zulip):

checking this line it seems that by then the alias of a use x; and use x as _; are both None, so there is no way to distinguish them there. I think the next step would be to make it a Some(Name(Text("_"))) again (or add a new variant?) in order to identify it? does this make sense?

matklad (Jan 22 2020 at 08:59, on Zulip):

Yep, we need to explicitelly handle an alias which is _ case there

divma (Jan 22 2020 at 15:35, on Zulip):

i'm still trying to find where are import.aliasgenerated to prevent use x as _ to have it as None. Any hint? rn I'm looking at raw_items in ModCollector but I still need to find where those come from

Florian Diebold (Jan 22 2020 at 15:44, on Zulip):

raw imports are created here

Florian Diebold (Jan 22 2020 at 15:46, on Zulip):

and here's where the alias is taken from the syntax; I suspect in the as _ case, tree.alias() will be Some, but a.name() will be None

divma (Jan 22 2020 at 15:49, on Zulip):

Thanks, those look promising

divma (Jan 26 2020 at 16:54, on Zulip):

Do you think this is related?

Florian Diebold (Jan 26 2020 at 21:40, on Zulip):

that's the parsing code for aliases, but I don't think you need to change anything there. in the case of use Foo as _, this means tree.alias() will be there, but there will be no name. I think it's fine to just check whether there's no name; that means that we'll treat use Foo as; the same, but I think that's reasonable

divma (Jan 26 2020 at 22:57, on Zulip):

You were right here

and here's where the alias is taken from the syntax; I suspect in the as _ case, tree.alias() will be Some, but a.name() will be None

but I had a harder time that what was due to actually check it
pasted image

divma (Jan 27 2020 at 00:30, on Zulip):

btw, I think rls has a somewhat similar issue

divma (Jan 31 2020 at 16:13, on Zulip):

Should I make update and update_recursive have in resolutions a bool to signal a named import vs an unnamed one? Or would it be better to have another function to handle adding the unnamed_traits to ItemScope?

divma (Jan 31 2020 at 16:17, on Zulip):

btw just to be clear, only Traits should be added right? if Foo is a struct or enum /something else use Foo as _; has no other effect besides an unused importwarning (to the best of my understanding)

std::Veetaha (Jan 31 2020 at 16:17, on Zulip):

To my mind, a bool is viable if it adds not more than one or two simple if statements in the body of the function. Otherwise it means that we have totally different logic going on for different values of that boolean and that means you should explicitly create two functions to handle two totally different cases. It's up to you as an engineer to estimate what case you are currently in.

divma (Jan 31 2020 at 16:19, on Zulip):

Well, I would go with a simple bool but I prefer to align with whatever you guys prefer. Specially since I don't fully get what most of the code does yet

std::Veetaha (Jan 31 2020 at 16:24, on Zulip):

By the way, I like using a so-called named boolean technique, that I remember @matklad mentioned in one of his Rust eductational videos
It means that you convert a boolean to an enum of two values:

enum ImportNamedNess { Named, Unnamed }
// or much more laconic one:
struct ImportIsNamed(boolean) ;

Though I think you should choose whatever is appropriate for the case and leave this discussion for the code review.

std::Veetaha (Jan 31 2020 at 16:27, on Zulip):

By the way, I've just come up with that newtype for a boolean and I now like this idea very much

std::Veetaha (Jan 31 2020 at 16:33, on Zulip):

Hmm, it looks like we can invent a syntax for named parameters via newtypes:

struct ModuleId(LocalModuleId);
struct TargetVisibility(Visibility);
struct Resolutions<'a>(&'a [(Name, PerNs)]);
struct Depth(usize);

fn update_recursive(
        &mut self,
        module_id: ModuleId,
        resolutions: Resolutions,
        // All resolutions are imported with this visibility; the visibilies in
        // the `PerNs` values are ignored and overwritten
        vis: TargetVisibility,
        depth: Depty,
    );
// then call
update_recursive(
      ModuleId(some_local_module_id),
      Resolutions(some_array_of_resolutions),
      TargetVisibility(some_visibility),
      Depth(442)
);
Florian Diebold (Feb 01 2020 at 13:31, on Zulip):

to get back to the original questions, it seems to make sense to me to make resolutions a [(Option<Name>, PerNs)]...

Florian Diebold (Feb 01 2020 at 13:32, on Zulip):

btw just to be clear, only Traits should be added right?

if it makes things simpler, sure, but I think it'd be fine to not check this

Last update: Jun 07 2020 at 10:35UTC