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

decl_macro expansion in 2018 context cannot refer to external crates at the root #55668

Closed
jebrosen opened this issue Nov 4, 2018 · 3 comments
Assignees
Labels
A-macros-2.0 Area: Declarative macros 2.0 (#39412) A-resolve Area: Path resolution

Comments

@jebrosen
Copy link
Contributor

jebrosen commented Nov 4, 2018

decl_macros that are invoked in a 2018 edition crate can fail depending on how the macro's paths are defined.

Minimized example:

#![feature(decl_macro)]

macro test {
    () => {
        ::std::vec::Vec::new()
    }
}

fn main() {
    let x: Vec<i32> = test!();
}

In the 2018 edition, this fails to compile:

error[E0433]: failed to resolve. Could not find `std` in `{{root}}`
  --> src/main.rs:5:11
   |
5  |         ::std::vec::Vec::new()
   |           ^^^ Could not find `std` in `{{root}}`
...
10 |     let x: Vec<i32> = test!();
   |                       ------- in this macro invocation

error: aborting due to previous error

The edition at the expansion location is what matters in this case, not the edition in which the macro is defined. Other crates show the same error message, not just std.

The workarounds that I have found so far are to start the path with std (no leading ::), or to add extern crate std; into the macro itself. The former is not a usable option when the decl_macro is generated by a proc_macro, because the crate std could be shadowed by a module named std at the "real" decl_macro definition site.

My bisect of the more complicated example pointed to #54658 -- cc @petrochenkov

cc @SergioBenitez

@SergioBenitez
Copy link
Contributor

I believe this is actually the expected behavior. Since decl_macro's are supposed to be fully hygienic, their expansion should not be able to refer to crates (or any item) that isn't declared in the expansion itself or a parent expansion thereof. That being said, it's long been desired that the prelude be made available to decl_macros which would resolve this case in particular.

The bug, however, is that hygiene seems to depend on the edition. The following, for instance, compiles just fine in the 2015 edition but fails in the 2018 edition:

#![feature(decl_macro)]

macro test() {
    ::std::vec::Vec::new()
}

fn main() {
    let x: Vec<usize> = test!();
}

@petrochenkov petrochenkov self-assigned this Nov 4, 2018
@petrochenkov petrochenkov added A-resolve Area: Path resolution A-macros-2.0 Area: Declarative macros 2.0 (#39412) labels Nov 4, 2018
@petrochenkov
Copy link
Contributor

@SergioBenitez

I believe this is actually the expected behavior. Since decl_macro's are supposed to be fully hygienic, their expansion should not be able to refer to crates (or any item) that isn't declared in the expansion itself or a parent expansion thereof.

The real scheme is a bit more complex, when resolving i-th path segment with i > 0 hygienic context is "adjusted to the module" of the prefix 0 ... i - 1, so things like

#![feature(decl_macro)]

struct S;

mod m {
    macro m() {
        let s = crate::S; // OK
    }
    
    fn check() {
        m!();
    }
}

fn main() {}

work despite S in crate::S not having the same context as S in struct S.

So, ::my_crate should indeed work, and #54658 introduced a regression by forgetting to adjust std's context in ::std to the "crate universe module".

That being said, it's long been desired that the prelude be made available to decl_macros

Preludes are already kinda work with decl_macros

#![feature(decl_macro)]

macro m() {
    let v = Vec::<u8>::new(); // OK
}

fn main() {
    m!();
}

, but how exactly they work with non-built-in names is an open question, and their hygiene is broken in cross-crate scenarios - prelude of the use-site crate is always used.

bors added a commit that referenced this issue Nov 14, 2018
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost
bors added a commit that referenced this issue Nov 18, 2018
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost
@petrochenkov
Copy link
Contributor

Fixed in #55884

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros-2.0 Area: Declarative macros 2.0 (#39412) A-resolve Area: Path resolution
Projects
None yet
Development

No branches or pull requests

3 participants