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

Upcoming rustc breakage wrt macro re-exporting #95

Closed
therealprof opened this issue May 5, 2018 · 12 comments
Closed

Upcoming rustc breakage wrt macro re-exporting #95

therealprof opened this issue May 5, 2018 · 12 comments
Labels
feb-2019-cleanup These issues are proposed to be closed, as part of a cleanup of issues in February 2019

Comments

@therealprof
Copy link
Contributor

In recent the next stable rust version, macro re-exporting will work differently hence insta-breaking all current and future crates generated by svd2rust and at least some (if not all) hal implementations and potentially even more.

Apart from fixing svd2rust we should probably prepare for this situation with some helpful advisories as part of the "embedded rust goes stable" story.

1 | # ! [ cfg_attr ( feature = "rt" , feature ( global_asm ) ) ] # ! [ cfg_attr ( feature = "rt" , feature ( macro_reexport ) ) ] # ! [ cfg_attr ( feature = "rt" , feature ( used ) ) ] # ! [ doc = "Peripheral access API for NRF51 microcontrollers (generated using svd2rust v0.12.0)\n\nYou can find an overview of the API [here].\n\n[here]: https://docs.rs/svd2rust/0.12.0/svd2rust/#peripheral-api" ] # ! [ allow ( private_no_mangle_statics ) ] # ! [ deny ( missing_docs ) ] # ! [ allow ( non_camel_case_types ) ] # ! [ feature ( const_fn ) ] # ! [ no_std ]
  |                                                                                                          ^^^^^^^^^^^^^^
  |
note: subsumed by `#![feature(use_extern_macros)]` and `pub use`
@therealprof
Copy link
Contributor Author

I've opened rust-embedded/svd2rust#209 about the breakage of the generated code.

pftbest added a commit to pftbest/cortex-m-rt that referenced this issue May 5, 2018
@japaric
Copy link
Member

japaric commented May 6, 2018

In recent the next stable rust version, macro re-exporting will work

I don't understand why the word "stable" appears here. There has never been a stable mechanism to reexport macros. #[macro_reexport] was always an unstable feature; that's why removing it was not a breaking change.

Apart from fixing svd2rust we should probably prepare for this situation with some helpful advisories as part of the "embedded rust goes stable" story.

The quasi-stable versions of cortex-m-rt and svd2rust (which already have PRs up) don't make use of #[macro_reexport]. If you need a macro from cortex-m-rt you do #[macro_use] extern crate cortex_m_rt;. What kind of advisories do you have in mind?

@therealprof
Copy link
Contributor Author

I don't understand why the word "stable" appears here. There has never been a stable mechanism to reexport macros.

Sure. However the previous unstable mechanism to re-export macros has been used in embbeded and will not work in the upcoming stable version which is what we're aiming for, no?

#[macro_reexport] was always an unstable feature; that's why removing it was not a breaking change.

😏 That's what a language lawyer would say not how it is perceived by a existing or potential user. Over night this change "broke" (for the lack of a better word that would satisfy a language lawyer) a lot (if not even all) close-to-metal rust crates, so many maintainers will need to take action and in addition it might shy away people trying rust for embedded because they hit a wall they may not be able to work around.

The quasi-stable versions of cortex-m-rt and svd2rust (which already have PRs up) don't make use of #[macro_reexport].

Great to hear. Didn't see them.

If you need a macro from cortex-m-rt you do #[macro_use] extern crate cortex_m_rt;.

Does that re-export the macro? Didn't know that.

I was following the hints in the error message, i.e. turn #![cfg_attr(feature = "rt", feature(macro_reexport))] into #![cfg_attr(feature = "rt", feature(use_extern_macros))] and #[macro_reexport(default_handler, exception)] into pub use cortex_m_rt::{default_handler, exception};

What kind of advisories do you have in mind?

Exactly like the one above: How to switch from the previous unstable mechanism to the new stable mechanism.

@japaric
Copy link
Member

japaric commented May 6, 2018

Sure. However the previous unstable mechanism to re-export macros has been used in embbeded and will not work in the upcoming stable version which is what we're aiming for, no?

I don't see what's the problem here. How macros are re-exported is an implementation detail of device crates. Users of those device crates can continue to use #[macro_use] to import the macros regardless of whether the device crate is using #[macro_reexport] or the use_extern_macros feature.

Re-exporting macros is not possible on stable today. If that's still not the case for the edition release (*) then we simply won't use this feature on stable embedded Rust. The only impact of doing this is that instead of writing #[macro_use(exception)] extern crate stm32f103xx; you'll have to write #[macro_use(exception)] extern crate cortex_m_rt; as stm32f103xx won't re-export the exception! macro defined in cortex_m_rt.

(*) Though I think the plan is to stabilize use_extern_macros by the edition release.

That's what a language lawyer would say not how it is perceived by a existing or potential user.

Users of nightly are aware that their code can break over night. If they don't want any breakage they should use the stable channel. The ETA for embedded Rust on stable is 1.28; if one doesn't want to deal with breakage they can wait until then.

If you need a macro from cortex-m-rt you do #[macro_use] extern crate cortex_m_rt;.

Does that re-export the macro? Didn't know that.

No, it doesn't. It only imports the macro.

I was following the hints in the error message

That describes how to migrate macro re-exports from macro_reexport to use_extern_macros. Importing macros with #[macro_use] still works the same -- it's a stable feature after all.

How to switch from the previous unstable mechanism to the new stable mechanism.

There's no stable mechanism. If you meant "how to deal with the breakage". The steps are:

  1. Switch to a device crate generated using svd2rust v0.12.1. As a user this may only involve updating the Cargo.lock file.
  2. There's no step 2. No need to modify your code. All your uses of #[macro_use] continue to work.

@therealprof
Copy link
Contributor Author

I don't see what's the problem here. How macros are re-exported is an implementation detail of device crates.

Funny, you said you don't see the problem and then described it in the very next sentence yourself: Pretty much all device crates are broken and will need to be fixed otherwise they can't be used anymore.

# cargo build --release --examples
    Updating git repository `https://github.com/japaric/stm32f103xx-hal`
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling num-traits v0.2.0
   Compiling semver-parser v0.7.0
   Compiling libc v0.2.36
   Compiling cortex-m v0.3.1
   Compiling vcell v0.1.0
   Compiling cortex-m-rt v0.4.0
   Compiling bare-metal v0.1.1
   Compiling aligned v0.1.1
   Compiling cortex-m v0.4.3
   Compiling r0 v0.2.2
   Compiling untagged-option v0.1.1
   Compiling stm32f103xx-hal v0.1.0 (https://github.com/japaric/stm32f103xx-hal#330c9044)
   Compiling nb v0.1.1
   Compiling cast v0.2.2
   Compiling numtoa v0.0.7
   Compiling static-ref v0.2.1
   Compiling volatile-register v0.2.0
   Compiling embedded-hal v0.1.2
   Compiling time v0.1.39
   Compiling semver v0.9.0
   Compiling stm32f103xx v0.9.1
   Compiling num-integer v0.1.36
   Compiling num-iter v0.1.35
   Compiling num v0.1.42
   Compiling chrono v0.4.0
   Compiling rustc_version v0.2.2
   Compiling cortex-m-rt v0.3.13
   Compiling stm32f103xx v0.8.0
error[E0557]: feature has been removed
 --> /Users/egger/.cargo/registry/src/github.com-1ecc6299db9ec823/stm32f103xx-0.8.0/src/lib.rs:1:106
  |
1 | # ! [ cfg_attr ( feature = "rt" , feature ( global_asm ) ) ] # ! [ cfg_attr ( feature = "rt" , feature ( macro_reexport ) ) ] # ! [ cfg_attr ( feature = "rt" , feature ( used ) ) ] # ! [ doc = "Peripheral access API for STM32F103XX microcontrollers (generated using svd2rust v0.12.0)\n\nYou can find an overview of the API [here].\n\n[here]: https://docs.rs/svd2rust/0.12.0/svd2rust/#peripheral-api" ] # ! [ allow ( private_no_mangle_statics ) ] # ! [ deny ( missing_docs ) ] # ! [ deny ( warnings ) ] # ! [ allow ( non_camel_case_types ) ] # ! [ feature ( const_fn ) ] # ! [ no_std ]
  |                                                                                                          ^^^^^^^^^^^^^^
  |
note: subsumed by `#![feature(use_extern_macros)]` and `pub use`
 --> /Users/egger/.cargo/registry/src/github.com-1ecc6299db9ec823/stm32f103xx-0.8.0/src/lib.rs:1:106
  |
1 | # ! [ cfg_attr ( feature = "rt" , feature ( global_asm ) ) ] # ! [ cfg_attr ( feature = "rt" , feature ( macro_reexport ) ) ] # ! [ cfg_attr ( feature = "rt" , feature ( used ) ) ] # ! [ doc = "Peripheral access API for STM32F103XX microcontrollers (generated using svd2rust v0.12.0)\n\nYou can find an overview of the API [here].\n\n[here]: https://docs.rs/svd2rust/0.12.0/svd2rust/#peripheral-api" ] # ! [ allow ( private_no_mangle_statics ) ] # ! [ deny ( missing_docs ) ] # ! [ deny ( warnings ) ] # ! [ allow ( non_camel_case_types ) ] # ! [ feature ( const_fn ) ] # ! [ no_std ]
  |                                                                                                          ^^^^^^^^^^^^^^

error[E0658]: The attribute `macro_reexport` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
 --> /Users/egger/.cargo/registry/src/github.com-1ecc6299db9ec823/stm32f103xx-0.8.0/src/lib.rs:4:1
  |
4 | #[macro_reexport(default_handler, exception)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: add #![feature(custom_attribute)] to the crate attributes to enable

error: aborting due to 2 previous errors

Some errors occurred: E0557, E0658.
For more information about an error, try `rustc --explain E0557`.
error: Could not compile `stm32f103xx`.
warning: build failed, waiting for other jobs to finish...
error: build failed

Users of nightly are aware that their code can break over night. If they don't want any breakage they should use the stable channel. The ETA for embedded Rust on stable is 1.28; if one doesn't want to deal with breakage they can wait until then.

This is a horrible stance to take and I'm not sure why you're saying that. We want people to use Rust on embedded, to join in, help, create code and expand the ecosystem, no? I think it is our obligation to make that as seamless and easy as possible, especially since some details are not stable yet.

That describes how to migrate macro re-exports from macro_reexport to use_extern_macros. Importing macros with #[macro_use] still works the same -- it's a stable feature after all.

As the title implies we're only talking about re-exporting here.

There's no stable mechanism. If you meant "how to deal with the breakage". The steps are:

Switch to a device crate generated using svd2rust v0.12.1. As a user this may only involve updating the Cargo.lock file.

There're different actions for different cases:

  • A peripheral crate needs to be recreated with svd2rust
  • A HAL implementation that re-exports macros (as quite a few happen to do) need to replace the re-exports with the new mechanism
  • Same as BSP crates that use re-exports

There's no step 2. No need to modify your code. All your uses of #[macro_use] continue to work.

No. If the dependency crates are not fixed then you won't even get to the point of compiling code that does the #[macro_use] because the compiler will bomb out earlier (as seen above).

@japaric
Copy link
Member

japaric commented May 6, 2018

OK, so the facts here are:

  • If you are the author of a crate that directly uses the unstable macro_reexport feature then (obviously) it's your job to fix it. The error message about macro_reexport being removed tells you how (though including an example of how to fix things in the error message would have been nice). Basically you use "#![feature(use_extern_macros)] and pub use" as follows:
-#![feature(macro_reexport)]
+#![feature(use_extern_macros)]

-#[macro_reexport(foo)]
 extern crate bar;

+pub use bar::foo;
  • If you are the author of a crate that depends on a crate that uses macro_reexport then you will (hopefully) report the problem to the author of the dependency and switch to an older nightly to avoid the problem. Once the dependency has been fixed (in a new patch version as the fix is non-breaking) you can switch to it (by updating your Cargo.lock file) and switch to a recent nightly. That's it; you do not need to change anything in your code; importing macros via #[macro_use] will continue to work regardless of whether macro_reexport or use_extern_macros is being used in the dependency.

  • The bulk of the crates that need to be fixed are device crates generated using svd2rust. The fix
    consists of simply re-generating the crate using svd2rust v0.12.1.

Anyone has a concrete proposal to do anything about this issue? I still don't understand what @therealprof meant by 'Upcoming rustc breakage', 'the next stable rust version' and 'part of the "embedded rust goes stable" story'. The process of dealing with the macro_reexport breakage is no different than the one for any other unstable feature changing and breaking crates; and macro reexports have no bearing on the embedded Rust on stable story.

@therealprof
Copy link
Contributor Author

OK, so the facts here are:

Correct. I don't think I've ever written anything else.

Anyone has a concrete proposal to do anything about this issue?

I'm in the process of writing a blog article about this. This surprising breakage did cost me some time, prevented me from doing some investigation I wanted to do instead of working around this (you probably guessed by now that this investigation involves a recent compiler version) and I'm very much afraid that this will cause a lot of frustration for newbies so crate maintainers should be reminded that there might be an issue and it would be cool if they could check if there is and fix it rather sooner than later.

I still don't understand what @therealprof meant by 'Upcoming rustc breakage', 'the next stable rust version'

That's just my (potentially inaccurate) way of saying that as of a few days ago it is broken. I was not aware at that point that it was an unstable feature.

and 'part of the "embedded rust goes stable" story'.

You can be pretty certain that if we don't do anything about this now it won't be addressed in all crates by the time embedded rust will be available in stable.

and macro reexports have no bearing on the embedded Rust on stable story.

I don't see how that statement can be correct, unless you're telling that the plan is to get rid of the re-exports entirely, e.g. in svd2rust generated code...

@RandomInsano
Copy link

Hey guys. Lots of frustration in this thread. I can appreciate that it was difficult but anger isn’t productive in this scenario.

If there are breakages, let’s find a way to hunt them down for people and make a strike force to make it awesome as part of the stabilization effort. I wouldn’t mind some mindless PR work on random libraries to change pace a little bit.

Crates.io links to source, and I know the compiler team regularly rebuilds a bunch of things from there to confirm all is well. Let’s document to avoid future paper cuts and hunt the existing stuff down instead of letting this discussion get the better of us.

@japaric
Copy link
Member

japaric commented May 6, 2018

I don't see how that statement can be correct, unless you're telling that the plan is to get rid of the re-exports entirely, e.g. in svd2rust generated code...

Yes, unstable features have no place in stable embedded Rust so we are dropping the re-exports. Like I mentioned above the re-exports are just a convenience; you can get e.g. the exception! macro directly from cortex-m-rt.

If use_extern_macro gets stabilized in 1.28 or 1.29 then we can add back the re-exports, but we are removing them in svd2rust 0.13.0.

You can be pretty certain that if we don't do anything about this now it won't be addressed in all crates by the time embedded rust will be available in stable.

We are removing all unstable features, including use_extern_macros, from all the Cortex-M related crates in their next minor version release. Which is going to happen soon so there'll be plenty of time for the crates.io ecosystem to adapt (i.e. follow suit) before the edition release.

@therealprof
Copy link
Contributor Author

My blog post about this can be found here: https://www.eggers-club.de/blog/2018/05/06/changes-in-rust-macro-re-exporting/

@jamesmunns
Copy link
Member

I believe the items in this issue have been addressed. I am nominating this issue to be closed. Please let me know if you feel otherwise.

@jamesmunns jamesmunns added the feb-2019-cleanup These issues are proposed to be closed, as part of a cleanup of issues in February 2019 label Feb 4, 2019
@jamesmunns
Copy link
Member

I am closing this issue, please feel free to open another issue if you would like this discussed further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feb-2019-cleanup These issues are proposed to be closed, as part of a cleanup of issues in February 2019
Projects
None yet
Development

No branches or pull requests

4 participants