Remove proc-macro back-compat hack for rental #106060
Conversation
|
r? @nagisa (rustbot has picked a reviewer for you, use r? to override) |
This comment was marked as outdated.
This comment was marked as outdated.
rustbot
added
S-waiting-on-review
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
cc @rust-lang/compiler This change further reduces the scope of a longstanding hack in the procedural macro infrastructure (following up on #94063). Currently, we special-case the This is a horrible hack that we should get rid of. Unfortunately, the allsorts-rental crate still lacks a point release that works without the hack. However, a fixed version of The previous 'tightening' of this hack in #94063 landed in 1.66.0 with zero reported regressions on the issue tracker. This lint has been showing up in I've left in the |
Aaron1011
added
S-waiting-on-team
Let's not forget that crater is a source of signal, not the entirety of Rust code out there.
Can we make it so that any error caused by this hack removal would be explicit about what the user can do to get to working code? What is what users would see otherwise? Because it seems like it might be the following which isn't very illuminating. Instead of removing the hack entirely, can we turn the lint into a hard error for a few releases instead as an intermediary step?
Should we make sure that there are dot releases for every published major version of these two crates? That way anyone depending on allsorts-rental 0.4.* would still have an update path. |
compiler/rustc_expand/src/base.rs
Outdated
| "using an old version of `rental`", | ||
| BuiltinLintDiagnostics::ProcMacroBackCompat( | ||
| "older versions of the `rental` crate will stop compiling in future versions of Rust; \ | ||
| please update to `rental` v0.5.6, or switch to one of the `rental` alternatives".to_string() |
There was a problem hiding this comment.
Can we list at least some of the alternatives?
There was a problem hiding this comment.
I've added a mention of ouroboros, and adjusted the error message to specifically refer to allsorts-rental.
That's true - however, the total lack of reported issues gives me more confidence. Compare this to the various incremental-compilation issues, which resulted in a large number of reported issues from affected projects (despite being non-deterministic and difficult to trigger).
That
We could, but I don't think it would be very useful. In practice, I think all of the breakage is going to be caused by users upgrading directly from a very old compiler (which has no warnings at all around this) to a future compiler (with the hack removed entirely). Making it a hard error for a little while won't really change this situation, since users might still upgrade directly to a future version where the hard error logic has been removed.
|
Unfortunately, this isn't possible. With the hack removed, old versions of the |
|
This was probably discussed before, but why not:
So if cargo found that blacklisted crates, it suggest user to update (assuming, that update available), or live in the doomed world they have created, without digging which part broken exactly. |
|
@klensy: That's an interesting idea. I would prefer to remove all special-casing based on crate names, but having a check in cargo does seem less hacky than having rustc alter its behavior like it does now. |
|
r? rust-lang/compiler |
Aaron1011
force-pushed
the
remove-rental-hack
branch
from
fa95173
to
c8bd7f3
Compare
This leaves the hack in place for allsorts-rental - it still relies on this backc-ompat hack to compile, and some crates on crates.io are still using it. We've been emitting future-incompat report warnings for rental for a while now, and fixing a broken build only requires bumping rental to the latest point release.
Aaron1011
force-pushed
the
remove-rental-hack
branch
from
c8bd7f3
to
8dcea83
Compare
I like the theme of this proposal. I'm curious about people's thoughts about overall timing. Namely: Should we actually block removing the hack from the compiler until after the cargo change has been made? (Or, to rephrase: If the hack were removed from the compiler today, with no preceding change to cargo, would we have any motivation to subsequently put the warning/error into cargo later on? Or is it something where the warning/error only makes sense as a kind of future-incompat warning, and once the hack is removed and the breakage hits compiler's users, will they still benefit from the higher level cargo perspective?) The reason I ask is that I'm trying to figure out if makes sense to try to contribute the cargo change, in order to unblock this, or if we can just go ahead and land this without worrying if the cargo change has been implemented yet. |
|
Adding in a cargo warning afterwards could still be beneficial. If users update directly from an old compiler to the latest stable or nightly (containing the cargo change), they'll have an improved experience if they're using a crate broken by the upgrade |
|
Okay. I think for the time being we can land this as is
Before this branch: log of the current error message you get if you force rental 0.5.5 via `rental = "=0.5.5"` in Cargo.toml dependenciesAfter: log of this branch's error message you get if you force rental 0.5.5 via `rental = "=0.5.5"` in Cargo.toml dependenciesHaving said that, we have had the future-incompat message in there for quite a long time; it was restricted to focus on Longer term, I would like us to look into shifting these messages into |
|
What is the cost to promote the existing lint to a hard error? It doesn't seem to be adding too big of a maintenance burden, does it? I fear people trying to build a 3 year old project a year from now and just getting (seemingly) gibberish.
I believe that to handle cases like these, where a proc-macro crate can produce invalid code that has been deprecated and fixed in subsequent versions, the ideal DX would be for cargo to have a database of such issues, and pass it down to rustc along with crate name and version. That way the compatibility error would only be shown on the appropriate error, and if never hit because of how the user used the crate then nothing is emitted. For what is worth, I've been wanting cargo to pass down the crate names and versions for a while now, to better handle the "expected Foo, found Foo" category of errors. Having said that, I think that we can have cargo hold a list of "unconditionally warn/error if this crate-version is used if rustc --version > NN ". The maintenance burden of such a list should be low, and having it be a separate file (or even downloadable!) could allow distros to keep that list around and give better errors for the times someone inevitably tries to build a project off github in 2029 when running today's Debian. If we are set on completely removing this logic for rental, I'd like it if we could coordinate to have something on cargo before this reaches users. |
|
We discussed this issue in today's T-compiler triage meeting |
Sorry, got interrupted before I finished my thought there.
Changing this PR to S-waiting-on-author to reflect our request for a more specific error message. (Or, if you would rather just close this PR for now and wait for the cargo work, I think that would be fine too, based on how I read the room in the meeting today.) @rustbot label: -S-waiting-on-team +S-waiting-on-author |
pnkfelix
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #111925) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Aaron1011 |
|
I think the benefit of an external database that cargo pulls is that users can find out about these situations now and prepare. This avoids the problem of a user having a bad dependency on an old toolchain and then skipping over any of the transitional toolchains when they do their next upgrade. Instead, once the toolchain with support for the external database is their old version, they can find out about these dependencies then and upgrade that dependency before upgrading their toolchain. We do have a database today: yanked crates. I don't know how crates.io feels about shortcutting the maintainer for yanking these "bad" versions. And I wish we could attach a yanked reason to it (rust-lang/cargo#2608). There is a PR open to allow forcing the use of a yanked version. rustsec also has a database. Their focus is on security but that bleeds over a little when it comes to "unmaintained crates" (e.g. its a small step to say "suggest structopt users switch to clap despite structopt being 'maintained'"). It is also all through third-party subcommands atm. Whether its hard coded or not, the cargo team is relatively cautious when it comes to warnings until we have rust-lang/cargo#12235. However, I think a case like this is likely warranted for an unconditional warning until then. |
|
Sorry, @epage , just to clarify: How should I translate your comment for the specific case of rental 0.5.5? As far as I can tell, rental 0.5.5 is not yanked, and I'm as clueless as you are about the policy of crates.io about non-owners trying to arrange for "bad" crates to get yanked. So maybe I should interpret your comment as saying "See if you can get rustsec to add rental 0.5.5 to their database" ? Or the very end of your comment mentioned "whether its hard-coded or not" -- so maybe you're saying, for the medium term, the cargo team might be willing to accept a PR that includes special case handling of rental 0.5.5, rather than trying to do the upfront design of the separate database that would store these cases? |
|
I would first recommend checking with crates.io (CC @Turbo87) to see if their policy allows or can be adapted to allow these broken-but-working-on-old version crates to be yanked by them. If that doesn't work out, we can raise it to the cargo team about putting in a short-term hack for this. I believe we already have one or two. I don't mention rustsec because, while that has almost what we want, the intention of marking these crates bad doesn't align with them. Longer term, I think cargo and crates.io should collaborate on how to maintain a rustsec-like database to track these things on the server so the status is accessible by all versions of cargo that can read this database, rather than just gets hard coded in new versions. My main concern about a short-term cargo hack is that its just easy enough that there won't be motivation for anyone to move forward with the database. |
The hack is a now blocker for #125174, which is a blocker for #124141, which is on the path to fixing the longstanding bug in #67062. This PR was about reducing the scope of the hack but I need the hack to be removed completely to make progress on that chain of work. #93275 was created to (2.25 years ago) to remove the hack entirely, so my comment is perhaps more relevant there, but this PR has more recent discussion and so seems like a good place to talk about it. It's now been four years since the hack was added (#73345). That's a long time. Can we remove it? Can we remove it without also requiring some bigger change to Cargo and/or crates.io that hasn't progressed beyond the "here's an idea" stage and seems, realistically speaking, unlikely to be implemented any time soon? |
|
@nnethercote I think that the fastest, least intrusive course of action is executing on
and removing this hack. Even if the change is detecting the specific |
|
@estebank: Ok, I think making it an unconditional error would be enough to unblock me. When I do that, what level of consultation do you think the PR will need? Does it need a crater run? Does it need t-compiler discussion? Or are the previous crater runs and discussions in the various PRs (plus the passing of another year or two) enough, do you think? |
|
#125596 has code to convert the lint to an unconditional error. |
|
Gonna do a crater run simply to sanity check, but beyond that I'm happy with the solution. |
|
I will close this PR. Removing the hack for allsorts-rental but not rental doesn't gain much, and this PR is superseded by #125596. |
This leaves the hack in place for allsorts-rental - it still relies on this backc-ompat hack to compile, and some crates on crates.io are still using it.
We've been emitting future-incompat report warnings for rental for a while now, and fixing a broken build only requires bumping rental to the latest point release.
Background (in reverse chronological order):
rentalandallsorts-rentalcrates back in PR Only applyProceduralMasqueradehack to older versions ofrental#94063proc_macro_back_compatwas extended to any use of aprocedural-masqueradecrate in PR Extendproc_macro_back_compatlint toprocedural-masquerade#83168proc_macro_back_comatlint was introduced in PR Introduceproc_macro_back_compatlint, and emit fortime-macros-impl#83127 (rollup PR Rollup of 10 pull requests #83149)