Skip to content

Make macro scope a real name scope and fix some details#1795

Merged
bors[bot] merged 2 commits intorust-lang:masterfrom
oxalica:macro-in-ns
Sep 9, 2019
Merged

Make macro scope a real name scope and fix some details#1795
bors[bot] merged 2 commits intorust-lang:masterfrom
oxalica:macro-in-ns

Conversation

@oxalica
Copy link
Copy Markdown
Contributor

@oxalica oxalica commented Sep 9, 2019

This PR make macro's module scope a real name scope in PerNs, instead of handling Either<PerNs, MacroDef> everywhere.

In rustc, the macro scope behave exactly the same as type and value scope.
It is valid that macros, types and values having exact the same name, and a use statement will import all of them. This happened to module alloc::vec and macro alloc::vec!.
So Either is not suitable here.

There is a trap that not only does #[macro_use] import all #[macro_export] macro_rules, but also imports all macros used in the crate root.
In other words, it just imports all macros in the module scope of crate root. (Visibility of use doesn't matter.)

And it also happened to libstd which has use alloc_crate::vec; in crate root to re-export alloc::vec, which it both a module and a macro.
The current implementation of #[macro_use] extern crate doesn't work here, so that is why only macros directly from libstd like dbg! work, while vec! from liballoc doesn't.
This PR fixes this.

Another point is that, after some tests, I figure out that macro_rules does NOT define macro in current module scope at all.
It defines itself in legacy textual scope. And if #[macro_export] is given, it also is defined ONLY in module scope of crate root. (Then being macro_used, as mentioned above)
(Well, the nightly Declarative Macro 2.0 simply always define in current module scope only, just like normal items do. But it is not yet supported by us)

After this PR, in my test, all non-builtin macros are resolved now. (Hover text for documentation is available) So it fixes #1688 . Since compiler builtin macros are marked as #[rustc_doc_only_macro] instead of #[macro_export], we can simply tweak the condition to let it resolved, but it may cause expansion error.

Some critical notes are also given in doc-comments.

Screenshot_20190909_223859

Fix some details about module scoping
@matklad
Copy link
Copy Markdown
Contributor

matklad commented Sep 9, 2019

This PR make macro's module scope a real name scope in PerNs, instead of handling Either<PerNs, MacroDef> everywhere.

Hm, interesting! So, the original motivation for handing macros separately was that macros are only needed in the initial expansion phase. After that, the rest of the hir should assume a fully macro expanded code, and, for example, name resolution operations should be forbidden from returning a macro. However, I think that this reasoning is actually flawed: the main thing that tries to resolve paths is name resolution and macro expansion itself! Basically the rest of the hir should see paths already resolved.

Comment thread crates/ra_hir/src/nameres/per_ns.rs Outdated
Comment thread crates/ra_hir/src/nameres/per_ns.rs
@matklad
Copy link
Copy Markdown
Contributor

matklad commented Sep 9, 2019

bors r+

bors Bot added a commit that referenced this pull request Sep 9, 2019
1795: Make macro scope a real name scope and fix some details r=matklad a=uHOOCCOOHu

This PR make macro's module scope a real name scope in `PerNs`, instead of handling `Either<PerNs, MacroDef>` everywhere.

In `rustc`, the macro scope behave exactly the same as type and value scope.
It is valid that macros, types and values having exact the same name, and a `use` statement will import all of them. This happened to module `alloc::vec` and macro `alloc::vec!`.
So `Either` is not suitable here.

There is a trap that not only does `#[macro_use]` import all `#[macro_export] macro_rules`, but also imports all macros `use`d in the crate root.
In other words, it just _imports all macros in the module scope of crate root_. (Visibility of `use` doesn't matter.)

And it also happened to `libstd` which has `use alloc_crate::vec;` in crate root to re-export `alloc::vec`, which it both a module and a macro.
The current implementation of `#[macro_use] extern crate` doesn't work here, so that is why only macros directly from  `libstd` like `dbg!` work, while `vec!` from `liballoc` doesn't.
This PR fixes this.

Another point is that, after some tests, I figure out that _`macro_rules` does NOT define macro in current module scope at all_.
It defines itself in legacy textual scope. And if `#[macro_export]` is given, it also is defined ONLY in module scope of crate root. (Then being `macro_use`d, as mentioned above)
(Well, the nightly [Declarative Macro 2.0](rust-lang/rust#39412) simply always define in current module scope only, just like normal items do. But it is not yet supported by us)

After this PR, in my test, all non-builtin macros are resolved now. (Hover text for documentation is available) So it fixes #1688 . Since compiler builtin macros are marked as `#[rustc_doc_only_macro]` instead of `#[macro_export]`, we can simply tweak the condition to let it resolved, but it may cause expansion error.

Some critical notes are also given in doc-comments.

<img width="447" alt="Screenshot_20190909_223859" src="https://user-images.githubusercontent.com/14816024/64540366-ac1ef600-d352-11e9-804f-566ba7559206.png">


Co-authored-by: uHOOCCOOHu <hooccooh1896@gmail.com>
@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Sep 9, 2019

Build succeeded

@bors bors Bot merged commit 5f48ef3 into rust-lang:master Sep 9, 2019
@oxalica oxalica deleted the macro-in-ns branch September 10, 2019 05:29
bors Bot added a commit that referenced this pull request Sep 11, 2019
1796: Support completion for macros r=matklad a=uHOOCCOOHu

This is based on #1795 , and fixes #1727 

Also prettify hover text of macros.

Some screenshorts below:

Completion in item place.
<img width="416" alt="Screenshot_20190910_134056" src="https://user-images.githubusercontent.com/14816024/64587159-fa72da00-d3d0-11e9-86bb-c98f169ec08d.png">

After pressing `tab`.
<img width="313" alt="Screenshot_20190910_134111" src="https://user-images.githubusercontent.com/14816024/64587160-fa72da00-d3d0-11e9-9464-21e3f6957bd7.png">

Complete macros from `std`.
<img width="588" alt="Screenshot_20190910_134147" src="https://user-images.githubusercontent.com/14816024/64587161-fb0b7080-d3d0-11e9-866e-5161f0d1b546.png">

Hover text.
<img width="521" alt="Screenshot_20190910_134242" src="https://user-images.githubusercontent.com/14816024/64587162-fb0b7080-d3d0-11e9-8f09-ad17e3f6702a.png">



Co-authored-by: uHOOCCOOHu <hooccooh1896@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Most of macros from std are not resolved

2 participants