Skip to content

Fix #2443 #2444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix #2443 #2444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/ra_hir_def/src/nameres.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ pub struct ModuleData {
pub definition: Option<FileId>,

pub impls: Vec<ImplId>,

/// alias information (Name => Alias)
pub aliases: FxHashMap<Name, Name>,
Copy link
Member

Choose a reason for hiding this comment

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

This info is only need during name-resolutrion process, not afterweards, so this field should be a part of ModCollector

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agreed.

}

#[derive(Default, Debug, PartialEq, Eq)]
Expand Down
20 changes: 19 additions & 1 deletion crates/ra_hir_def/src/nameres/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,13 @@ where

let resolution = Resolution { def, import: Some(import_id) };
self.update(module_id, Some(import_id), &[(name, resolution)]);

// insert alias information to module
if let Some(alias) = &import.alias {
self.def_map.modules[module_id]
.aliases
.insert(last_segment.name.clone(), alias.clone());
}
}
None => tested_by!(bogus_paths),
}
Expand All @@ -407,7 +414,18 @@ where
import: Option<LocalImportId>,
resolutions: &[(Name, Resolution)],
) {
self.update_recursive(module_id, import, resolutions, 0)
self.update_recursive(module_id, import, resolutions, 0);

// update alias
let alias_resolutions: Vec<_> = resolutions
.iter()
.filter_map(|(name, res)| {
let alias = self.def_map.modules[module_id].aliases.get(name)?;
Some((alias.clone(), res.clone()))
})
.collect();

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I am feeling uneasy about this approach. Seems like Name -> Name mapping errases essential information (like from what import the name initially came). I imagine use foo::a as b; use bar::a as b might actually be legal if a's live in different namespaces?

And I think we actually have all the required infrastructure in place to fix this properly? Basically, an use foo as bar import should be marked unresolved, and, when we add foo to the module namespace, on the next iteration of the loop { } we should also add bar. It seems like we need to fix some bug in existing infra, rather than add something specific for aliases tracking.

Copy link
Member Author

@edwin0cheng edwin0cheng Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

And I think we actually have all the required infrastructure in place to fix this properly? Basically, an use foo as bar import should be marked unresolved.

Um.. imagine this example:

mod m {
    macro_rules! _foo {
        ($x:ident) => { type $x = u64; }
    }
    pub(crate) use _foo as foo;
}

macro_rules! foo {
    () => {}
}

m::foo!(foo);
use foo as bar;   // <-----

The order of resolving will be:

mod m
macro_rules _foo 
use _foo as foo
macro_rules foo
use foo as bar  // foo is macro_rules! foo
m:foo!(foo)
type foo = u64

Copy link
Member Author

@edwin0cheng edwin0cheng Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I am feeling uneasy about this approach. Seems like Name -> Name mapping errases essential information (like from what import the name initially came). I imagine use foo::a as b; use bar::a as b might actually be legal if a's live in different namespaces?

Yes, I agree, I didn't think of this problem. Can I fix it by adding more information to alias map ? Or do you feel there is a better approach to solve it ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so the problem is that we end up with bar only as macro, and miss bar as a type, right?

So looks like the problem is that we remove use foo as bar from the set of unresolved imports when it resolves in any namespace, while we should only do this if it resolves in all namespaces. I wonder if we construct a test case here which doens't involve any macros? something like this

type T = ();

use self::T as  Y;

use m::*;

mod m {
  pub const T: () = ();
}

fn main() { let _: Y = Y; }

Looks like rust-analyzer indeed fails to see that Y is a value here:

image

Copy link
Member

Choose a reason for hiding this comment

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

yeah, one doesn't need aliases to construct a similar problem:

image

image

Copy link
Member Author

@edwin0cheng edwin0cheng Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes indeed. The problem here is the use alias. My approach to fix it is mark the alias and rewrite to it. OTOH, we can mark it to unresolved, but the problem is, how can we know the resolve loop should be ended ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, one doesn't need aliases to construct a similar problem:

Okay, my alias fix won't work in your case. :)

Copy link
Member

Choose a reason for hiding this comment

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

Just checked that IntelliJ also doesn't handle the above example 😩

self.update_recursive(module_id, None, &alias_resolutions, 0);
}

fn update_recursive(
Expand Down
31 changes: 31 additions & 0 deletions crates/ra_hir_ty/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4956,3 +4956,34 @@ fn main() {
"###
);
}

#[test]
fn infer_type_value_non_legacy_macro_use_as() {
assert_snapshot!(
infer(r#"
mod m {
macro_rules! _foo {
($x:ident) => { type $x = u64; }
}
pub(crate) use _foo as foo;
}

m::foo!(foo);
use foo as bar;
fn f() -> bar { 0 }

fn main() {
let _a = f();
}

"#),
@r###"
[159; 164) '{ 0 }': u64
[161; 162) '0': u64
[176; 200) '{ ...f(); }': ()
[188; 190) '_a': u64
[194; 195) 'f': fn f() -> u64
[194; 197) 'f()': u64
"###
);
}