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

Tracking Issue for proc_macro_back_compat #83125

Open
Aaron1011 opened this issue Mar 14, 2021 · 23 comments
Open

Tracking Issue for proc_macro_back_compat #83125

Aaron1011 opened this issue Mar 14, 2021 · 23 comments
Assignees
Labels
A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros A-proc-macros Area: Procedural macros C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-needs-to-bake Status: The implementation is "complete" but it needs time to bake. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Mar 14, 2021

What is this issue?

If you're a crate author who's been linked here, your crate (indirectly) depends on a version of a procedural macro crate that will stop compiling in the future. All affected crates have updated versions published, so you should update to the latest version of the crate mentioned by the compiler warning you saw.

Background

In the past, the Rust compiler has had several bugs in the way that it passes a TokenStream to procedural macros ('proc-macro's). Fixing these bugs resulted in changes the input TokenStream, which some proc-macros were not prepared to handle.

To avoid breaking large portions of the ecosystem, several hardcoded hacks were added to the compiler. When specific macros from certain crates (e.g. time-macros-impl) are invoked, we invoke the macro with a modified TokenStream (what would have been used before the bugfix), which keeps the project compiling.

However, hard-coding specific crate and macro names in the compiler is a huge hack, which we'd like to remove as soon as possible. This issue tracks our progress towards removing all of these hacks.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

TODO

Unresolved Questions

Implementation history

@Aaron1011 Aaron1011 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. A-proc-macros Area: Procedural macros A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros labels Mar 14, 2021
@Aaron1011 Aaron1011 self-assigned this Mar 14, 2021
@est31
Copy link
Member

est31 commented Apr 20, 2021

I'm hitting this issue in glium/glium#1922 but there seems to be no use of the procedural-masquerade crate at all. What am I doing wrong?

@est31
Copy link
Member

est31 commented Apr 20, 2021

OH apparently it's a bug in the warning. The PR that added it didn't check for the crate but a dummy type. This dummy type is also present in rental. As rental still has thousands of downloads per day, maybe PR #83168 should be reverted until rental is fixed?

@Aaron1011
Copy link
Member Author

Reverting that PR seems like overkill. I'll work on a fix for the false positive.

@est31
Copy link
Member

est31 commented Apr 20, 2021

@Aaron1011 that would do it as well, thanks!

@Aaron1011
Copy link
Member Author

Aaron1011 commented Apr 20, 2021

Interestingly enough, this is actually not a false-positive. rental copied some string parsing logic from procedural-masquerade, and suffers from the bug that caused us to add the hack in the first place.

When I disabled the pretty-printing hack, I got this panic:

thread 'rustc' panicked at 'expected prefix "stringify!" not found in "#[allow(unused)] enum ProceduralMasqueradeDummyType\n{ Input = (0, stringify ! (32)) . 0 }"', rental-impl/src/lib.rs:25:9

which is the same type of issue that procedural-masquerade has.

This means that rental will actually stop compiling when we remove the hack (though the warning message could be modified to mention rental specifically.

The fact that rental is unmaintained (and the repository is archived) makes this tricky.

cc @petrochenkov

@est31
Copy link
Member

est31 commented Apr 22, 2021

@Aaron1011 I've filed a dedicated issue for it: #84428

@Stargateur
Copy link
Contributor

Stargateur commented Apr 29, 2021

If I resume, we didn't make the breaking when it was still early to do it so we did a hack and now time have passed and more people used the bugged behavior. Isn't the worst possible thing to do ?

Better now than never I guess.

@andr0s
Copy link

andr0s commented Jun 12, 2021

Tried to compile Firefox and got TONS of such errors...

0:05.92 warning: using procedural-masquerade crate
0:05.92 --> servo/ports/geckolib/error_reporter.rs:412:28
0:05.92 |
0:05.92 412 | )) => (cstr!("PEColorNotColor"), Action::Nothing),
0:05.92 | ^^^^^^^^^^^^^^^^^^^^^^^^
0:05.92 |
0:05.92 = warning: this was previously accepted by the compiler but is being phased out; it will become a hard
error in a future release!
0:05.92 = note: for more information, see issue #83125 #83125

@Aaron1011
Copy link
Member Author

@andr0s Newer versions of the cstr crate no longer depend on procedural-masquerade. Updating to the latest version of cstr should fix those warnings.

@est31
Copy link
Member

est31 commented Jun 12, 2021

Firefox versions since 85 should not depend on offending cstr versions: https://bugzilla.mozilla.org/show_bug.cgi?id=1661961

@andr0s might be compiling the ESR version of Firefox. Latest ESR is 78.11.0. I've downloaded the source tarball and it still depends on cstr 0.1.

According to the wiki, ESR 78.0 is still under maintenance until its last release on October 05. I think until then there should be no breakage of the build, and maybe one should wait a couple of weeks after that.

Also I wonder if Firefox should maybe be integrated into crater. There is an issue for servo but none for Firefox proper or ESR, although it is an important product that builds on Rust.

@KeyboardDanni
Copy link

cc @jpernst

I am using alto which uses al-sys and rental, both crates that throw this warning. What's my best course of action here? I need maintained OpenAL bindings and I am only a single dev with limited time and resources.

@Stargateur
Copy link
Contributor

Stargateur commented Feb 15, 2022

@cosmicchipsocket look like alto is archived so not much.

You can use a fixed rustc version where you know your project compile but this is a no update solution, you can of course fork alto, rental can be replace with ouroboros

I don't know much about openAl, I found ears but that all.

@KeyboardDanni
Copy link

KeyboardDanni commented Feb 15, 2022

My project is a public crate so I can't just freeze the Rust version. ears isn't going to work for me because it's an engine rather than just a wrapper around OpenAL. It also relies on external (possibly OS-specific) C libraries to load sound formats, which I am trying to avoid.

I'm most likely just going to end up forking alto. But my big worry is that dependency-breaking changes are going to continue to be a thing in Rust going forward. Today, it's alto/rental. Tomorrow it's png. Then suddenly a Rust change breaks a crate that tons of people rely on like winapi or rand.

To reiterate, I am a single developer. Every unmaintained crate that is broken by removal of backward compatibility is yet another crate I have to maintain myself just so I can continue compiling my code. Dependencies cascade - the only logical end to this is that I have to take upon the sole burden of hundreds of crates, repeatedly updating them to keep up with the latest breakage in Rust. I would be spending more time fixing broken code in unfamiliar dependencies than actually being productive in my own work.

Rust already has multiple strategies to try and avoid an "npm left-pad" situation, including keeping existing crates on crates.io and employing the Rust Editions. This whole thing about "this code won't compile in a future rustc" runs counter to all that, and I really hope that the Rust team is willing to consider alternatives.

I have three questions:

  1. What is the burden to maintaining this particular instance of backward compatibility? Is there a timetable for when the warning becomes an error? All the Rust devs have to do is flip a switch and suddenly my life will become a lot harder, because I will end up becoming the sole maintainer of alto and its dependencies, and perhaps many more in the future. My hope is that the Rust developers aren't doing this just to make things "clean" and that they are actually weighing the negative effects on the ecosystem should they decide to flip that switch.
  2. Is there any chance that the user could be allowed to specify using the older macro logic in a specific dependency, somewhat like Rust Editions? This would at least relieve the burden of maintaining a list of exceptions from the compiler to the user.
  3. Is there or will there be in the future a group dedicated to updating unmaintained crates across the Rust ecosystem? I know there are examples of this in one or two sub-ecosystems, but nothing that covers the Rust ecosystem in its entirety, at least to my knowledge. And while there's a place to report orphaned crates, it's more for reporting security vulnerabilities and not to get those crates new maintainers.

@Aaron1011
Copy link
Member Author

Can you post the output you're getting from the compiler?

@KeyboardDanni
Copy link

The package `al-sys v0.6.1` currently triggers the following future incompatibility lints:
> warning: using `procedural-masquerade` crate
>    --> C:\Users\danni\.cargo\registry\src\github.com-1ecc6299db9ec823\al-sys-0.6.1\src\lib.rs:141:1
>     |
> 141 | / al_api! {
> 142 | |     alcCreateContext: unsafe extern "C" fn(device: *mut ALCdevice, attrlist: *const ALCint) -> *mut ALCcontext,
> 143 | |     alcMakeContextCurrent: unsafe extern "C" fn(context: *mut ALCcontext) -> ALCboolean,
> 144 | |     alcProcessContext: unsafe extern "C" fn(context: *mut ALCcontext) -> (),
> ...   |
> 235 | |     alGetBufferiv: unsafe extern "C" fn(buffer: ALuint, param: ALenum, values: *mut ALint) -> (),
> 236 | | }
>     | |_^
>     |
>     = note: `#[allow(proc_macro_back_compat)]` on by default
>     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>     = note: for more information, see issue #83125 <https://github.com/rust-lang/rust/issues/83125>
>     = note: The `procedural-masquerade` crate has been unnecessary since Rust 1.30.0. Versions of this crate below 0.1.7 will eventually stop compiling.
>     = note: this warning originates in the macro `rental` (in Nightly builds, run with -Z macro-backtrace for more info)
>

The package `rental v0.5.6` currently triggers the following future incompatibility lints:
> warning: using `procedural-masquerade` crate
>    --> C:\Users\danni\.cargo\registry\src\github.com-1ecc6299db9ec823\rental-0.5.6\src\lib.rs:94:8
>     |
> 94  |         enum ProceduralMasqueradeDummyType {
>     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ...
> 117 |     define_rental_traits!(32);
>     |     ------------------------- in this macro invocation
>     |
>     = note: `#[allow(proc_macro_back_compat)]` on by default
>     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>     = note: for more information, see issue #83125 <https://github.com/rust-lang/rust/issues/83125>
>     = note: The `procedural-masquerade` crate has been unnecessary since Rust 1.30.0. Versions of this crate below 0.1.7 will eventually stop compiling.
>     = note: this warning originates in the macro `define_rental_traits` (in Nightly builds, run with -Z macro-backtrace for more info)
>
> warning: using `procedural-masquerade` crate
>    --> C:\Users\danni\.cargo\registry\src\github.com-1ecc6299db9ec823\rental-0.5.6\src\lib.rs:258:9
>     |
> 258 |               enum ProceduralMasqueradeDummyType {
>     |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ...
> 285 | / rental! {
> 286 | |     /// Example types that demonstrate the API generated by the rental macro.
> 287 | |     pub mod examples {
> 288 | |         use std::sync;
> ...   |
> 345 | |     }
> 346 | | }
>     | |_- in this macro invocation
>     |
>     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>     = note: for more information, see issue #83125 <https://github.com/rust-lang/rust/issues/83125>
>     = note: The `procedural-masquerade` crate has been unnecessary since Rust 1.30.0. Versions of this crate below 0.1.7 will eventually stop compiling.
>     = note: this warning originates in the macro `rental` (in Nightly builds, run with -Z macro-backtrace for more info)
>
> warning: using `procedural-masquerade` crate
>    --> C:\Users\danni\.cargo\registry\src\github.com-1ecc6299db9ec823\rental-0.5.6\src\lib.rs:258:9
>     |
> 258 |               enum ProceduralMasqueradeDummyType {
>     |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ...
> 350 | / rental! {
> 351 | |     /// Premade types for the most common use cases.
> 352 | |     pub mod common {
> 353 | |         use std::ops::DerefMut;
> ...   |
> 484 | |     }
> 485 | | }
>     | |_- in this macro invocation
>     |
>     = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>     = note: for more information, see issue #83125 <https://github.com/rust-lang/rust/issues/83125>
>     = note: The `procedural-masquerade` crate has been unnecessary since Rust 1.30.0. Versions of this crate below 0.1.7 will eventually stop compiling.
>     = note: this warning originates in the macro `rental` (in Nightly builds, run with -Z macro-backtrace for more info)
>

@Aaron1011
Copy link
Member Author

Aaron1011 commented Feb 16, 2022

Thanks! It looks like all of these warnings stem from the rental crate. Fortunately, rental 0.5.6 (which you're using) includes a fix that will allow it to continue compiling, even when the proc_macro_back_compat hacks are removed. Specifically, you do not need to fork alto (or take any action, in fact) for your crate to continue compiling in future versions of Rust. However, the compiler is emitting the warning anyway - I'll work on a pull request to stop emitting the warning in this case.

But my big worry is that dependency-breaking changes are going to continue to be a thing in Rust going forward

As a member of the compiler team (and the person responsible for adding in several of the backward-compatibility hacks that we're now trying to remove), I recognize how disruptive these kinds of backwards-incompatible changes are to the ecosystem. We try to keep them to an absolute minimum, and features like cargo report future-incompat were created specifically to help lessen the impact that they have on the ecosystem.

To address your questions:

What is the burden to maintaining this particular instance of backward compatibility?

The proc_macro_back_compat is a particularly unusual case. While it's not particularly difficult to maintain, it represents a pretty egregious hack in the language. It was only added because the alternative was causing a very large amount of ecosystem breakage. Specifically, we are hard-coding the names of individual crates/types:

let name = item.ident.name;
if name == sym::ProceduralMasqueradeDummyType {
if let ast::ItemKind::Enum(enum_def, _) = &item.kind {
if let [variant] = &*enum_def.variants {
if variant.ident.name == sym::Input {

let time_macros_impl =
macro_name == sym::impl_macros && matches_prefix("time-macros-impl", "lib.rs");
let js_sys = macro_name == sym::arrays && matches_prefix("js-sys", "lib.rs");
if time_macros_impl || js_sys {
let snippet = source_map.span_to_snippet(orig_span);
if snippet.as_deref() == Ok("$name") {

if macro_name == sym::tuple_from_req && matches_prefix("actix-web", "extract.rs") {
let snippet = source_map.span_to_snippet(orig_span);

From a language perspective, this is quite bad - the compiler's behavior shouldn't change just based on the name of the type you've written, or the filename you're using.

Is there a timetable for when the warning becomes an error?

At the moment, there's no fixed timetable. The main reason that I'm trying to do this sooner rather than later is to prevent additional crates from beginning to rely on this behavior (which could make this even harder to remove in the future). The recent stabilization of cargo report future-incompat should hopefully serve to discourage people from depending on affected versions. However, experience has shown that as long as it's technically possible for people to rely on something (e.g. compilation succeeds when they use it), then some people will rely on it.

All the Rust devs have to do is flip a switch and suddenly my life will become a lot harder, because I will end up becoming the sole maintainer of alto and its dependencies, and perhaps many more in the future. My hope is that the Rust developers aren't doing this just to make things "clean" and that they are actually weighing the negative effects on the ecosystem should they decide to flip that switch.

I'm committed to ensuring that the impact on the ecosystem is as small as possible. In particular, I want to make sure that we avoid the situation that you've just described, where individual crate authors are forced to fork arbitrary (transitive) dependencies to keep their project compiling. Before disabling any of these back-compat measures, I'll be performing another Crater run to gauge ecosystem impact, and (when possible) reaching out to affected projects on Github to explain what changes need to be made.

Ideally, all affected downstream crates can simply run cargo update to avoid breakage, since all of the affected crates will have published updates that avoid relying on the back-compat logic. Fortunately, your crate falls into this category (though I need to update the compiler to avoid emitting the current false-positive warning).

Is there any chance that the user could be allowed to specify using the older macro logic in a specific dependency, somewhat like Rust Editions? This would at least relieve the burden of maintaining a list of exceptions from the compiler to the user.

Unfortunately, this would probably lead to a larger maintenance burden, since Cargo would need to be extended to allow passing this information along.

Is there or will there be in the future a group dedicated to updating unmaintained crates across the Rust ecosystem? I know there are examples of this in one or two sub-ecosystems, but nothing that covers the Rust ecosystem in its entirety, at least to my knowledge. And while there's a place to report orphaned crates, it's more for reporting security vulnerabilities and not to get those crates new maintainers.

I don't think there's currently any such group, but I think it would be a good idea to form one.

@KeyboardDanni
Copy link

KeyboardDanni commented Feb 16, 2022

Thank you for the response. I'm feeling a bit better about this now.

You mentioned that the warnings are only stemming from rental though, when the output specifically lists al-sys as well. Is it just because the warning is pointing to rental specifics within the al_api! macro?

@Aaron1011
Copy link
Member Author

You mentioned that the warnings are only stemming from rental though, when the output specifically lists al-sys as well. Is it just because the warning is pointing to rental specifics within the al_api! macro?

That's right - you can use RUSTFLAGS="-Z macro-backtrace" on nightly to see this

@Stargateur
Copy link
Contributor

@cosmicchipsocket you mix a lot of thing, true that breaking is disappointing, but archived repository is not rust compiler fault. with or without this you would end up with an unmaintained dependencies. So you are already in a situation with no upgrade cause of this dead crate.

@Aaron1011
Copy link
Member Author

with or without this you would end up with an unmaintained dependencies.

Rust's stability policy should ensure that an unmaintained crate will continue to compile with future versions of Rust (the crate itself may have bugs, but that's not caused by the compiler). This is a case where it's necessary for the compiler to violate that policy, but this kind of event should be very rare. In normal circumstances, users should be able to rely on the compiler not making breaking changes.

@Aaron1011
Copy link
Member Author

Aaron1011 commented Feb 17, 2022

@cosmicchipsocket I've opened #94063 to gauge the impact of starting to phase out one of these backwards-compatibility hacks (a hack that changes how we pretty-print certain types). It will still be applied to older versions of rental, but not anything else. If merged, that PR will suppress the spurious warnings that you're currently seeing.

Aaron1011 added a commit to Aaron1011/bracket-fluent that referenced this issue Nov 9, 2022
Your crate depends on an old version of `rental`, which relies on
a backwards-compatibiltiy hack in rustc. In future versions of rustc,
versions of `rental` below `0.5.6` will now compiling, breaking your
crate.

This PR updates your crate to `rental` 0.5.6, which ensures that it will
continue to compile with both older and newer version of rustc.

For more information, see rust-lang/rust#83125
@wesleywiser wesleywiser added the S-tracking-needs-to-bake Status: The implementation is "complete" but it needs time to bake. label Jan 26, 2024
@wesleywiser
Copy link
Member

Visited during compiler team tracking issue triage. Since #106060 is open and waiting on consensus to either flip the switch or land new tooling improvements, I'm marking this as S-tracking-needs-to-bake.

nnethercote added a commit to nnethercote/rust that referenced this issue May 27, 2024
We still check for the `rental`/`allsorts-rental` crates. But now if
they are detected we just emit a fatal error, instead of emitting a
warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added
in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative
behaviour is interfering with rust-lang#125174 and follow-on work.
nnethercote added a commit to nnethercote/rust that referenced this issue May 27, 2024
We still check for the `rental`/`allsorts-rental` crates. But now if
they are detected we just emit a fatal error, instead of emitting a
warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added
in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative
behaviour is interfering with rust-lang#125174 and follow-on work.
bors added a commit to rust-lang-ci/rust that referenced this issue May 27, 2024
Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? `@estebank`
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jun 4, 2024
…stebank

Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? `@estebank`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 4, 2024
…stebank

Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? ``@estebank``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 4, 2024
Rollup merge of rust-lang#125596 - nnethercote:rental-hard-error, r=estebank

Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? ``@estebank``
@nnethercote
Copy link
Contributor

@estebank: now that #125596 has merged and turned the proc_macro_back_compat lint into an unconditional error, do you think it is reasonable to close this tracking issue?

lcnr pushed a commit to lcnr/rust that referenced this issue Jun 12, 2024
We still check for the `rental`/`allsorts-rental` crates. But now if
they are detected we just emit a fatal error, instead of emitting a
warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added
in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative
behaviour is interfering with rust-lang#125174 and follow-on work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros A-proc-macros Area: Procedural macros C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-needs-to-bake Status: The implementation is "complete" but it needs time to bake. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants