Stream: t-compiler/wg-rls-2.0

Topic: Shadowing imports


Paul Faria (Jun 24 2020 at 13:03, on Zulip):

@matklad now that https://github.com/rust-analyzer/rust-analyzer/pull/5015 is going to be merged, should I submit another PR just to remove this comment https://github.com/rust-analyzer/rust-analyzer/blob/4029628f15c612182ad9da1c652078f9df62f5cf/crates/ra_hir_def/src/item_scope.rs#L54 or is there more work to do?

matklad (Jun 24 2020 at 13:04, on Zulip):

I think so, though there are other shadowing problems elsewhere: https://github.com/rust-analyzer/rust-analyzer/pull/4716

Paul Faria (Jun 24 2020 at 13:09, on Zulip):

Well, I just got curious and realized my change only fixes depending on the order of imports. So now it's still broken if the glob comes later in the imports

Paul Faria (Jun 24 2020 at 13:10, on Zulip):

e.g., swapping the lines use foo::*; use bar::baz; changes which module is imported

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

yes, that needs some more handling (I actually was surprised that we don't do that already)

Paul Faria (Jun 24 2020 at 13:20, on Zulip):

I think I can solve it in collector by partitioning the imports by glob, then resolving the glob imports first, followed by the non-glob so they can override the glob imports.

Florian Diebold (Jun 24 2020 at 13:32, on Zulip):

I don't think that will work, because you can't resolve all glob imports (in the crate) before all named imports. I think the right way to handle it would be to just keep track of whether an import comes from a glob, and allow them to override accordingly

Paul Faria (Jun 24 2020 at 13:32, on Zulip):

In that case let me close https://github.com/rust-analyzer/rust-analyzer/pull/5033 and I'll work on this some more :)

Paul Faria (Jun 24 2020 at 13:34, on Zulip):

Or rather, let me expand on it

Florian Diebold (Jun 24 2020 at 14:18, on Zulip):

here's an example:

mod a { pub struct X; }
mod b { pub use super::a::X; }
mod c { pub struct X; }
mod d {
    use super::c::X;
    use super::b::*;
}

if the glob import gets handled first, when the import in b gets resolved later, the new name in B is 'pushed' through the glob import so it shows up in d, but it shouldn't shadow the already existing X in d

Florian Diebold (Jun 24 2020 at 14:19, on Zulip):

(that's what's happening here)

Paul Faria (Jun 25 2020 at 00:11, on Zulip):

ok, I'll use that as a test and try to work on it a bit tonight and tomorrow morning.

Paul Faria (Jun 25 2020 at 01:44, on Zulip):

ok, I just pushed an update. Out of curiosity, I tried doing this:

pub mod foo {
    pub mod baz {
        pub struct Foo;
    }
}

pub mod bar {
    pub mod baz {
        pub struct Foo;
    }
}

use foo::*;
use bar::*;
use baz::Foo;

mostly because I didn't actually know what to expect from rustc. It turns out rustc considers the baz::Foo to be an error on the baz portion because it's ambiguous. Should I add that kind of tracking in my current PR, or as a follow-up?

Florian Diebold (Jun 25 2020 at 07:27, on Zulip):

I'd do it as a follow-up (we don't even have diagnostics for unresolved imports yet, I don't know if we should add them for ambiguous imports)

Paul Faria (Jun 25 2020 at 16:35, on Zulip):

@Florian Diebold I'm working on your comments during my lunch break. I have a solution for the memory usage you brought up that I think works: I extracted the tracking of the ImportType to an FxHashMap<Name, ImportType> that belongs to the DefCollector. The one possible issue I've come across is that there's also tracking being done in ExprCollector in lower.rs. Do I need to ensure the two are somehow connected? Or can they be tracked completely independently?

Florian Diebold (Jun 25 2020 at 16:37, on Zulip):

body lowering doesn't do any import resolution yet, I don't think that should be a problem :thinking:

Florian Diebold (Jun 25 2020 at 16:38, on Zulip):

it can't just be the Name though, can it, since the DefCollector is concerned about the whole crate, right?

Florian Diebold (Jun 25 2020 at 16:38, on Zulip):

i.e. you would need (LocalModuleId, Name)

Florian Diebold (Jun 25 2020 at 16:39, on Zulip):

and actually, maybe you could just make it a set? from_glob_import: FxHashSet<(LocalModuleId, Name)>

Florian Diebold (Jun 25 2020 at 16:41, on Zulip):

as for the per-namespace thing, I think something like

mod a { pub fn foo() {} }
mod b { pub fn foo() {} }
mod c {
    use super::a::*;
    macro_rules! foo {}
    use super::b::foo;
}

would be a problem if we don't track the globness per namespace

Paul Faria (Jun 25 2020 at 17:07, on Zulip):

That's really helpful, thank you. I'll tackle this more after work

Paul Faria (Jun 26 2020 at 01:33, on Zulip):

I'm going to push a broken commit (and amend later) just to get some help on it. For some reason, the macros are never set with your example. Also, in case anyone else needs it (and because it took me an hour to figure out...) here's the lldb invocation I used to get smolstr to show the string value in the watch window: type summary add "&ra_hir_expand::name::Name" -o "value = valobj.GetChildMemberWithName('0').GetChildMemberWithName('0').GetChildMemberWithName('0'); l = value.GetChildMemberWithName('len').GetValueAsUnsigned(0); s = value.GetChildMemberWithName('buf').AddressOf(); return '"%s"'% valobj.process.ReadMemory(s.GetValueAsUnsigned(0),l,lldb.SBError());"

Paul Faria (Jun 26 2020 at 01:36, on Zulip):

What I'm seeing in the new test I added, glob_shadowed_per_ns, is that push_res_with_import is never seeing a macro def. I'm not sure why that's the case.

Florian Diebold (Jun 26 2020 at 08:42, on Zulip):

hmm macro name resolution is weird, the example is probably wrong. You can achieve the same problem with a struct foo {} and a fn foo() {} though, I think

Paul Faria (Jun 26 2020 at 13:45, on Zulip):

I'm having a hard time figuring out how to validate this with the current test infrastructure. I can see that using the struct would definitely break the old design (track once per all ns), but the testing infrastructure doesn't let me validate where the items come from. Previously I had been using the modules and validating differently named structs within the submodules to validate the right module had been loaded. However, doing that bypasses the test I'm trying to write here.

Paul Faria (Jun 26 2020 at 13:59, on Zulip):

Would an enum work? I'll give that a try

Florian Diebold (Jun 26 2020 at 13:59, on Zulip):

yeah, I don't see how you could test this with the current nameres tests, since they just test that there is a name in a certain module, not which one

Florian Diebold (Jun 26 2020 at 14:01, on Zulip):

it would be possible to test it e.g. with a type inference test instead (though it would be nicer to test it in the same crate of course, but that would require implementing some new kind of test there)

Paul Faria (Jun 26 2020 at 14:58, on Zulip):

I also had to change the test data a bit. This setup triggers the case:

mod a { pub fn foo() {} pub struct foo; }
mod b { pub fn foo () {} }
mod c { pub struct foo; }
mod d {
    pub use super::a::*;
    pub use super::c::foo;
    pub use super::b::foo;
}

The previous one was collecting the inline module definitions first and so the check for overlapping globs was never been hit.

Paul Faria (Jun 26 2020 at 14:59, on Zulip):

Where do the type inference tests live btw?

Paul Faria (Jun 26 2020 at 14:59, on Zulip):

The ones in ra_hir_ty?

Florian Diebold (Jun 26 2020 at 15:01, on Zulip):

yes

Florian Diebold (Jun 26 2020 at 15:02, on Zulip):

the easiest way to test it in type inference would be to make the two foo functions return different types, and then see what type foo() gets

Paul Faria (Jun 26 2020 at 15:06, on Zulip):

Ok good, I was doing just that. I was also adding a field to the structs with the same name but different types and asserting that the field type matches what's expected.

Paul Faria (Jun 26 2020 at 15:06, on Zulip):

Just about to do red/green testing to make sure it actually works :sweat_smile:

Paul Faria (Jun 26 2020 at 15:12, on Zulip):

ok, just pushed latest. Should be good for review again. Thanks again for your help @Florian Diebold !

Last update: Sep 27 2020 at 13:15UTC