Skip to content
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.

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

handle extern crate in name resolution #523

Closed
killercup opened this issue Jan 13, 2019 · 7 comments · Fixed by #742
Closed

handle extern crate in name resolution #523

killercup opened this issue Jan 13, 2019 · 7 comments · Fixed by #742
Labels
E-has-instructions Issue has some instructions and pointers to code to get started E-medium

Comments

@killercup
Copy link
Member

Currently, std::sync::Arc cannot be resolved.

In #522 (comment), @flodiebold speculates:

std::sync gets properly resolved, but it reexports Arc from alloc_crate, which is an extern crate rename: extern crate alloc as alloc_crate. I guess that's the problem.


Here's a simple test case:

diff --git a/crates/ra_ide_api/src/hover.rs b/crates/ra_ide_api/src/hover.rs
index 2968b807..2ca1d6d9 100644
--- a/crates/ra_ide_api/src/hover.rs
+++ b/crates/ra_ide_api/src/hover.rs
@@ -230,6 +230,20 @@ mod tests {
         assert_eq!("u32", &type_name);
     }
 
+    #[test]
+    fn test_type_of_arc() {
+        let (analysis, position) = single_file_with_position(
+            r"
+            use std::sync::A<|>rc;
+
+            fn main() {}
+            ",
+        );
+
+        let hover = analysis.hover(position).unwrap().unwrap();
+        assert!(!hover.info.contains("Failed to exactly resolve"));
+    }
+
     // FIXME: improve type_of to make this work
     #[test]
     fn test_type_of_for_expr_1() {
@matklad matklad changed the title cannot resolve std::sync::Arc handle extern crate in name resolution Jan 26, 2019
@matklad
Copy link
Member

matklad commented Feb 3, 2019

@matklad matklad added the E-has-instructions Issue has some instructions and pointers to code to get started label Feb 3, 2019
@flodiebold
Copy link
Member

@matklad I've got a branch which does handle extern crate (by lowering them to imports with absolute paths), but it doesn't fix this problem, because it turns out we also need to fall back to resolving imports in the crate root. In this particular case, it looks like this:
lib.rs:

extern crate alloc as alloc_crate;
mod sync;

sync.rs:

pub use alloc_crate::sync::Arc;

In general, even in the 2018 edition, plain paths can be resolved either from the current scope or from the crate root.

@matklad
Copy link
Member

matklad commented Feb 3, 2019

In general, even in the 2018 edition, plain paths can be resolved either from the current scope or from the crate root.

Just checked locally, and it seems that on 2018 ::foo or use foo don't resolve from the crate root. Could it be that as in extern crate acts as as renamer of the whole crate?

@flodiebold
Copy link
Member

Hm, you're right, it just works for extern crate apparently. But extern crate foo as bar doesn't just rename the crate in the extern prelude, because 1. it only has an effect if it's in the crate root or the current module, and 2. the original name is still usable. Back to reading rustc, I guess...

@flodiebold
Copy link
Member

flodiebold commented Feb 3, 2019

I think extern crates in the crate root are special-cased to insert entries in the extern prelude, if I read this correctly:
https://github.com/rust-lang/rust/blob/fc6e9a2845e8bb4560811ed21136483a596505bb/src/librustc_resolve/build_reduced_graph.rs#L390-L408

@bjorn3
Copy link
Member

bjorn3 commented Feb 3, 2019

Yup, rust-lang/rust#54658

@flodiebold
Copy link
Member

Though std isn't actually a case of this, since it's still edition 2015, but I guess if we implemented it that way we could still avoid implementing 2015 name resolution...

@flodiebold flodiebold mentioned this issue Feb 4, 2019
bors bot added a commit that referenced this issue Feb 5, 2019
742: Extern crate r=matklad a=flodiebold

This implements `extern crate` declarations by lowering them to (absolute) imports, and adds support for absolute paths. It also extracts the extern prelude from the per-module item map, and handles the special case of extern crates in the crate root adding to the extern prelude.

This means we finally resolve `Arc`, so it fixes #523 😄 

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
@bors bors bot closed this as completed in #742 Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-has-instructions Issue has some instructions and pointers to code to get started E-medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants