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

Allow reexporting proc macro output in the 2018 edition #1359

Merged
merged 2 commits into from
Mar 26, 2019

Conversation

konstin
Copy link
Contributor

@konstin konstin commented Mar 18, 2019

Trying to use a proc macro from a 2018 edition crate in a 2018 edition crate that reexports wasm bindgen's output failed before this commit with "could not find wasm_bindgen in {{root}}".

This commit was made with

rg " ::wasm_bindgen::" --files-with-matches | xargs sed -i 's/::wasm_bindgen::/wasm_bindgen::/g'

Trying to use a proc macro from a 2018 edition crate in a 2018 edition crate that reexports wasm bindgen's output failed before this commit with "could not find `wasm_bindgen` in `{{root}}`".

This commit was made with 

rg " ::wasm_bindgen::" --files-with-matches | xargs sed -i 's/::wasm_bindgen::/wasm_bindgen::/g'
@alexcrichton
Copy link
Contributor

Thanks for the PR! Can you elaborate a bit though on where this failed in the wild? The leading :: is required for 2015 compatibility, but this should also definitely be compatible with 2018 crates!

@konstin
Copy link
Contributor Author

konstin commented Mar 18, 2019

It's not really in the wild, but in a crate of mine which contains a proc macro that re-exports the code generated by wasm-bindgen. In my test crate, which uses the re-exporting crate through a proc macro attribute, the version without leading :: worked with both the 2015 and 2018 edition. Afaik this would only be a problem if the re-exporting crate used the 2015 edition.

@alexcrichton
Copy link
Contributor

Oh I think I understand what you mean, sorry I think I didn't fully get what was being reexported before.

I'm actually pretty surprised that this works at all, if the leading :: is removed do the imports actually work? Doesn't that still, in 2018, rely on either a local wasm_bindgen name in the module or the wasm_bindgen crate being linked as a dependency? (which I suspect with reexporting you want to avoid). Or in other words, how does your reexported version work without the leading ::?

@konstin
Copy link
Contributor Author

konstin commented Mar 20, 2019

Doesn't that still, in 2018, rely on either a local wasm_bindgen name in the module or the wasm_bindgen crate being linked as a dependency? (which I suspect with reexporting you want to avoid). Or in other words, how does your reexported version work without the leading ::?

Yes, wasm-bindgen still needs to be in scope, but now I can hide it in a use foobar::prelude::*, i.e. the user (theoretically) doesn't even need to know wasm-bindgen exists.

I've made a minimal reproduction of my architecture in https://github.com/konstin/wasm-bindgen-1359. The structure essentially is the following:

lib-backend:

extern crate proc_macro;

use crate::proc_macro::TokenStream;

#[proc_macro_attribute]
pub fn some_attribute(attr: TokenStream, input: TokenStream) -> TokenStream { 
    wasm_bindgen_macro_support::expand(attr.into(), input.into()).unwrap().into()
}

lib-frontend:

pub mod prelude {
    pub use wasm_bindgen;
    pub use lib_backend::some_attribute;
}

user-crate:

extern crate lib_frontend;

use lib_frontend::prelude::*;

#[some_attribute]
struct FooBar {
    _x: usize
}

user-crate/Cargo.toml:

[package]
name = "user-crate"
version = "0.1.0"
authors = ["konstin <konstin@mailbox.org>"]
edition = "2018"

[dependencies]
lib-frontend = { path = "../lib-frontend" }

#[patch.crates-io]
#wasm-bindgen-macro-support = { git = "https://github.com/konstin/wasm-bindgen", branch = "reexporting_in_2018" }
#wasm-bindgen-backend = { git = "https://github.com/konstin/wasm-bindgen", branch = "reexporting_in_2018" }

With the last three lines commented out it works neither in the 2015 nor in the 2018 edition, while it works in both editions if the # are removed.

@alexcrichton
Copy link
Contributor

Ok that makes sense, thanks for the clarification!

Unfortunately though I'm not sure there's a great way to solve this. Although it's intended that you do use wasm_bindgen::prelude::* (or some similar prelude) it's not the only way to invoke a macro, as users could always do something like #[wasm_bindgen::prelude::wasm_bindgen] (or via some other path that was hooked up via pub use). In that sense the macro can't necessarily assume the name wasm_bindgen is in scope.

Similarly though it can't really assume that wasm_bindgen the crate is linked! I'm not really sure what the best way to tackle this would be. It seems like there's no great solution for us now :(

FWIW on a somewhat unrelated note I wouldn't recommend using the wasm_bindgen_macro_support crate if you can avoid it, it's best to lower to syntax that uses #[wasm_bindgen] itself and then that'll get expanded off to the side. (the API of wasm_bindgen_macro_support has no backcompat guarantees)

@konstin
Copy link
Contributor Author

konstin commented Mar 23, 2019

I made this change because I thought it would be an easy win that supports that special macro usage. So if we can't gaurantee this doesn't break anyone else's code, I think I can find another workaround for this problem.

In that sense the macro can't necessarily assume the name wasm_bindgen is in scope.

It seems to currently already assume that. For example the following 2018 crate fails to compile:

[dependencies]
other_name = { package = "wasm-bindgen", version = "=0.2.40" }
#[other_name::prelude::wasm_bindgen]
pub struct Bar {
    pub x: usize
}


#[other_name::prelude::wasm_bindgen]
impl Bar {
    fn get_x(&self) -> usize {
        self.x
    }
}

This results in a bunch of errors like:

error[E0658]: The attribute `__wasm_bindgen_class_marker` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
 --> src/lib.rs:9:1
  |
9 | #[other_name::prelude::wasm_bindgen]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: add #![feature(custom_attribute)] to the crate attributes to enable

error[E0433]: failed to resolve: use of undeclared type or module `wasm_bindgen`
 --> src/lib.rs:3:1
  |
3 | #[other_name::prelude::wasm_bindgen]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of undeclared type or module `wasm_bindgen`

error[E0432]: unresolved import
 --> src/lib.rs:3:1
  |
3 | #[other_name::prelude::wasm_bindgen]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0433]: failed to resolve: could not find `wasm_bindgen` in `{{root}}`
 --> src/lib.rs:3:1
  |
3 | #[other_name::prelude::wasm_bindgen]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `wasm_bindgen` in `{{root}}`

(Sidenote: I think that __wasm_bindgen_class_marker from #1208 now in parser.rs#L899 should get some wasm_bindgen:: prefix because it breaks #[wasm_bindgen::prelude::wasm_bindgen] pub struct Foo {} #[wasm_bindgen::prelude::wasm_bindgen] impl Foo { fn get_42(&self) {} } without renaming - Should I open a separate issue for that?)

As far as I can tell there is no proper solution to this problem because rust doesn't have any way to link proc macro code to cargo dependencies.

FWIW on a somewhat unrelated note I wouldn't recommend using the wasm_bindgen_macro_support crate if you can avoid it, it's best to lower to syntax that uses #[wasm_bindgen] itself and then that'll get expanded off to the side. (the API of wasm_bindgen_macro_support has no backcompat guarantees)

I'm aware of that and I'll change that before the crate leaves the pet stage. Currently I simply pin wasm-bindgen.

@alexcrichton
Copy link
Contributor

Hm ok, so given all that, I think I'm tempted to agree and say that we should remove all the leading :: here. To make sure I understand, what that means is:

  • In the 2018 edition, this resolves to either a local wasm_bindgen name in the module (which can be used for reexportation) or the list of external crates (which typically happens)
  • In the 2015 edition this will probably break entirely, unless you say use wasm_bindgen in the module that you invoke the macro in.

I wonder if we could get use wasm_bindgen::prelude::* to bring in wasm_bindgen, a module, that reexports the crate root? That way we can keep the 2015 edition working?

@konstin
Copy link
Contributor Author

konstin commented Mar 26, 2019

I tried some combinations, and I'm now 90% sure that the edition of the generated code is the edition of the declaring crate, i.e. always the 2018 edition. Unfortunately, there's nothing about the proc macro / edition interaction in the docs or in the sources for syn and proc_macro. I've also asked about that in discord but I didn't get any response. However if that is universally true, we simply can't break the 2015 edition.

@alexcrichton
Copy link
Contributor

Interesting! I think what may actually be happening is that the new name resolution rules have been applied backwards compatibly to the 2015 edition, and I agree it's working!

In that case seems like a universally good change to me, thanks again for this!

@alexcrichton alexcrichton merged commit c5d2b2d into rustwasm:master Mar 26, 2019
@konstin konstin deleted the reexporting_in_2018 branch March 26, 2019 18:54
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.

None yet

2 participants