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

Crates can't migrate to 2018 edition when they invoke coupled procedural macros #54647

Closed
alexcrichton opened this issue Sep 28, 2018 · 20 comments
Assignees
Labels
A-rust-2018-preview Area: The 2018 edition preview WG-epoch Working group: Epoch (2018) management

Comments

@alexcrichton
Copy link
Member

Procedural macros are often coupled with "runtime crates" which support the various macros that the procedural macro exports. These runtime crates, however, sometimes also want to invoke the procedural macro itself (think panic! and libstd, which exports it for users and also defines it itself).

Let's say our runtime crate is called foo. The macro today generally expands to paths that look like ::foo::MyType. This doesn't work by default in the crate foo itself, so foo typically includes a module that looks like:

mod foo { pub use super::*; }

And the enables the crate foo to use its own macros internally.

In the 2018 edition, however, the ::foo::MyType path unconditionally requires foo to be a crate, which isn't the case when we're compiling foo! As a result, these sorts of runtime crates don't have a great path forward when migrating to the 2018 edition.

@alexcrichton alexcrichton added WG-epoch Working group: Epoch (2018) management A-rust-2018-preview Area: The 2018 edition preview labels Sep 28, 2018
@alexcrichton
Copy link
Member Author

This was first reported at dtolnay/syn#507

@eddyb
Copy link
Member

eddyb commented Sep 28, 2018

cc @rust-lang/lang @petrochenkov

@joshtriplett
Copy link
Member

Shouldn't such macros use $self::MyType or similar instead? (Does that not work for procedural macros, for some reason?)

@dtolnay
Copy link
Member

dtolnay commented Sep 28, 2018

@joshtriplett the procedural macro and the runtime support live in separate crates, think serde_derive vs serde. You would need some way to specify what crate $self would refer to, because in code generated by serde_derive all the types are defined by serde not serde_derive.

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 28, 2018

Hack: provide a version of macro with an extra argument for internal use and replace ::foo with crate if it's used.

@petrochenkov
Copy link
Contributor

Hack 2: --extern foo=self that adds foo into extern prelude as an alias to crate.

@petrochenkov
Copy link
Contributor

Or, in general, some way to add a name that's associated with the current crate into extern prelude.

@joshtriplett
Copy link
Member

@dtolnay Ah, I misunderstood the phrase "the crate foo to use its own macros internally".

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 28, 2018

Perhaps even #![crate_name = "foo"] and --crate-name NAME can be re-purposed for that.
In general, if the crate knows its name (which is not always true), we can add that name into extern prelude.

EDIT: Nah, perhaps this need is too special, my_crate_name being generally available as an alias to crate in all crates built by Cargo would probably mostly cause confusion.

@petrochenkov
Copy link
Contributor

extern crate foo = PATH; 😄

@eddyb
Copy link
Member

eddyb commented Sep 28, 2018

@petrochenkov wouldn't that create a bit of a "side-effect" in the system?
Or would you be required to place it in the root of another crate?

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 28, 2018

@eddyb
Do you mean extern crate foo?
If by side effect you mean that the extern prelude set becomes dynamic and can change in the process of expansion, then yes.
It's not so bad, it's the same thing as #[macro_use] extern crate ... adding names into the macro prelude, so it's trivially implementable in the existing infrastructure.
It breaks the uniform import path scheme with canaries though, so it's one more reason to move the implementation to the proper search in scope sooner (EDIT: or perhaps not and we'll just need more canaries?).

@petrochenkov
Copy link
Contributor

#54658 implements a possible prerequisite (extern crate items adding things into extern prelude).

@stephaneyfx
Copy link
Contributor

This workaround using CARGO_PKG_NAME seems to work, but that does not feel like an elegant solution:

In crate foo:

pub trait Foo {
    fn foo(&self) -> String;
}

In crate foo-derive, the path used in the generated impl block is either ::foo::Foo or crate::Foo based on the value of the CARGO_PKG_NAME environment variable. Tested by using the derive(Foo) attribute within the foo crate and in a separate crate.

#[proc_macro_derive(Foo)]
pub fn derive_foo(input: TokenStream) -> TokenStream {
    let ast = syn::parse::<DeriveInput>(input).unwrap();
    let ty = &ast.ident;
    let ty_name = ty.to_string();
    let in_self = env::var("CARGO_PKG_NAME").unwrap() == "foo";
    let trait_path = if in_self {
        Path {
            leading_colon: None,
            segments: ["crate", "Foo"].iter()
                .map(|&s| Ident::new(s, Span::call_site()))
                .map(PathSegment::from)
                .collect(),
        }
    } else {
        Path {
            leading_colon: Some(Default::default()),
            segments: ["foo", "Foo"].iter()
                .map(|&s| Ident::new(s, Span::call_site()))
                .map(PathSegment::from)
                .collect(),
        }
    };
    let tokens = quote! {
        impl #trait_path for #ty {
            fn foo(&self) -> String {
                format!("I'm a {}", #ty_name)
            }
        }
    };
    tokens.into()
}

@petrochenkov
Copy link
Contributor

#55275 provides a possible solution.

bors added a commit that referenced this issue Oct 24, 2018
Add `extern crate` items to extern prelude

With this patch each `extern crate orig_name as name` item adds name `name` into the extern prelude, as if it was passed with `--extern`.

What changes this causes in practice?
Almost none! After all, `--extern` passed from Cargo was supposed to replace `extern crate` items in source, so if some code has `extern crate` item (or had it on 2015 edition), then it most likely uses `--extern` as well...

... with exception of a few important cases.

- Crates using `proc_macro`. `proc_macro` is not passed with `--extern` right now and is therefore not in extern prelude.
Together with 2018 edition import behavior this causes problems like #54418, e.g.
    ```rust
    extern crate proc_macro;
    use proc_macro::TokenStream;
    ```
    doesn't work.
It starts working after this patch.

- `#[no_std]` crates using `std` conditionally, like @aturon described in #53166 (comment), and still wanting to write `std` instead of `crate::std`. This PR covers that case as well.
This allows us to revert placing `std` into the extern prelude unconditionally, which was, I think, a [bad idea](#53166 (comment)).

- Later `extern crate` syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:
- Only `extern crate` items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
This means you can opt out from the prelude additions with something like
    ```rust
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
    ```
- I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the `extern crate` item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
bors added a commit that referenced this issue Oct 24, 2018
Add `extern crate` items to extern prelude

With this patch each `extern crate orig_name as name` item adds name `name` into the extern prelude, as if it was passed with `--extern`.

What changes this causes in practice?
Almost none! After all, `--extern` passed from Cargo was supposed to replace `extern crate` items in source, so if some code has `extern crate` item (or had it on 2015 edition), then it most likely uses `--extern` as well...

... with exception of a few important cases.

- Crates using `proc_macro`. `proc_macro` is not passed with `--extern` right now and is therefore not in extern prelude.
Together with 2018 edition import behavior this causes problems like #54418, e.g.
    ```rust
    extern crate proc_macro;
    use proc_macro::TokenStream;
    ```
    doesn't work.
It starts working after this patch.

- `#[no_std]` crates using `std` conditionally, like @aturon described in #53166 (comment), and still wanting to write `std` instead of `crate::std`. This PR covers that case as well.
This allows us to revert placing `std` into the extern prelude unconditionally, which was, I think, a [bad idea](#53166 (comment)).

- Later `extern crate` syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:
- Only `extern crate` items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
This means you can opt out from the prelude additions with something like
    ```rust
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
    ```
- I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the `extern crate` item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
bors added a commit that referenced this issue Oct 25, 2018
Add `extern crate` items to extern prelude

With this patch each `extern crate orig_name as name` item adds name `name` into the extern prelude, as if it was passed with `--extern`.

What changes this causes in practice?
Almost none! After all, `--extern` passed from Cargo was supposed to replace `extern crate` items in source, so if some code has `extern crate` item (or had it on 2015 edition), then it most likely uses `--extern` as well...

... with exception of a few important cases.

- Crates using `proc_macro`. `proc_macro` is not passed with `--extern` right now and is therefore not in extern prelude.
Together with 2018 edition import behavior this causes problems like #54418, e.g.
    ```rust
    extern crate proc_macro;
    use proc_macro::TokenStream;
    ```
    doesn't work.
It starts working after this patch.

- `#[no_std]` crates using `std` conditionally, like @aturon described in #53166 (comment), and still wanting to write `std` instead of `crate::std`. This PR covers that case as well.
This allows us to revert placing `std` into the extern prelude unconditionally, which was, I think, a [bad idea](#53166 (comment)).

- Later `extern crate` syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:
- Only `extern crate` items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
This means you can opt out from the prelude additions with something like
    ```rust
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
    ```
- I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the `extern crate` item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
@dhardy
Copy link
Contributor

dhardy commented Oct 28, 2018

@stephaneyfx's solution seems to work nicely, but can be simplified:

    let c = if env::var("CARGO_PKG_NAME").unwrap() == "mycrate" {
        quote!( crate )
    } else {
        quote!( mycrate )
    };

Then just use #c inside quote! instead of $crate.

Edit: I realise this will only work where the crate using the macro is using Edition 2018.

@petrochenkov
Copy link
Contributor

@dhardy
::crate::foo with leading :: doesn't currently work (#53347), so there may be issues with disambiguated paths?

@dhardy
Copy link
Contributor

dhardy commented Oct 30, 2018

I think I found a problem with this workaround (even using Edition 2018): rustdoc examples. Since the error message is pretty poor I'm not certain, but I think it's using crate within the doc example.

@glandium
Copy link
Contributor

Something I noticed in a 2018 edition crate using a 2015 crate/crate-derive pair, is that the trait needs to be used in the 2018 edition crate, while it wasn't necessary in 2015 code.

@withoutboats
Copy link
Contributor

withoutboats commented Nov 27, 2018

Something I noticed in a 2018 edition crate using a 2015 crate/crate-derive pair, is that the trait needs to be used in the 2018 edition crate, while it wasn't necessary in 2015 code.

That's expected. Derives were imported with #[macro_use] extern cratein 2015, whereas in 2018 they are imported the same way as any other name would be.

Is this going to be fixed before release or will these kinds of crates be incompatible across editions?

I don't know what the status of this is, but this issue doesn't refer to a crate being "incompatible across editions." That's not possible: there's no way to define a crate so that its "incompatible" with crates using a different edition. Crates using this pattern (which relies on an ambiguity in 2015 edition namespacing) just have to themselves continue to use the 2015 edition or one of the other workarounds discussed in this thread.

bors added a commit that referenced this issue Dec 1, 2018
experiment: Support aliasing local crate root in extern prelude

This PR provides some minimally invasive solution for the 2018 edition migration issue described in #54647 and affecting proc macro crates.

`extern crate NAME as RENAME;` now accepts `NAME`=`self` and interprets it as referring to the local crate.
As with other `extern crate` items, `RENAME` in this case gets into extern prelude in accordance with #54658, thus resolving #54647.
```rust
extern crate self as serde; // Adds local crate to extern prelude as `serde`
```
This solution doesn't introduce any new syntax and has minimal maintenance cost, so it can be easily deprecated if something better is found in the future.

Closes #54647
leo60228 added a commit to leo60228/celeste.rs that referenced this issue Mar 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust-2018-preview Area: The 2018 edition preview WG-epoch Working group: Epoch (2018) management
Projects
None yet
Development

No branches or pull requests

9 participants