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

Remove pretty-print compat hack for all crates #93275

Closed
wants to merge 1 commit into from

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jan 24, 2022

This PR removes the pretty-printing hack that changed how we printed types named ProceduralMasqueradeDummyType. This was added in #73345, to avoid causing widespread ecosystem breakage in the ecosystem while making a change to proc-macro invocation,

It's been around a year and a half, and all of the affected crates have new versions out which no longer require this hack to compile. The crater run in #93275 (comment) (https://crater-reports.s3.amazonaws.com/pr-93275/index.html) shows three sources of regressions:

  • The rental crate - while this crate is now unmaintained, its final versions 0.5.6 has a fix that allows us to compile without this hack. All affected crates should be able to bump their Cargo.lock to the latest version.
  • The cssparser crate - this crate is still maintained, and all recent versions no longer depend on procedural-masquerade (which is what panicked without this workaround).
  • allsorts-rental - this appears to be a fork of rental created by the allsorts projects (Make allsorts run on no_std yeslogic/allsorts#42). Confusingly, it has a version called 0.5.6 which is not the same as rental 0.5.6. However, the allsorts project no longer depends on rental since yeslogic/allsorts@7b51ea8, so I think this crate is essentially a legacy from crates that haven't yet updated their Cargo.lock

Unfortunately, there are still 231 crates that transitively depend on one of these 3 crates. The vast majority of these crates appear to be small personal projects with their Cargo.lock committed. At this point, I think that waiting longer won't decrease the amount of breakage - the authors of these crates are probably not running the latest Rust, and will therefore not see the future-incompat-report message.

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 24, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2022
@Aaron1011
Copy link
Member Author

r? @ghost

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 24, 2022
@bors
Copy link
Contributor

bors commented Jan 24, 2022

⌛ Trying commit 9f93af4f89973b09bf98a5f48fbb15f9466d5eab with merge 91c0623519feeb52387fd0c3677ece4d24642b73...

@bors
Copy link
Contributor

bors commented Jan 24, 2022

☀️ Try build successful - checks-actions
Build commit: 91c0623519feeb52387fd0c3677ece4d24642b73 (91c0623519feeb52387fd0c3677ece4d24642b73)

@rust-timer
Copy link
Collaborator

Queued 91c0623519feeb52387fd0c3677ece4d24642b73 with parent 51126be, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (91c0623519feeb52387fd0c3677ece4d24642b73): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 24, 2022
@Aaron1011
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-93275 created and queued.
🤖 Automatically detected try build 91c0623519feeb52387fd0c3677ece4d24642b73
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-93275 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-93275 is completed!
📊 231 regressed and 15 fixed (208162 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 29, 2022
@Aaron1011 Aaron1011 changed the title [DO NOT MERGE] Remove pretty-print compat hack for all crates Remove pretty-print compat hack for all crates Jan 29, 2022
@Aaron1011
Copy link
Member Author

cc @rust-lang/lang @petrochenkov

I propose that we remove this hack. The description of this PR contains a description of what this does, and what the expected fallout from removing this is.

If the amount of fallout is still judged to be too high, then I think we could 'partially' remove this - disable the hack on nightly, but revert it on beta. This should help catch any actively maintained projects that are depending on this (and are ignoring the cargo future-incompat-report message), while keeping our stable releases 'stable' for a little while longer.

@scottmcm
Copy link
Member

scottmcm commented Jan 29, 2022

Over a year seems like long enough to keep a "we were being nice" hack to me; let's see what everyone else thinks.

EDIT: Oh, this is marked compiler, despite the cc. Who should decide on it?

@Aaron1011
Copy link
Member Author

I think changes to the proc-macro infrastructure are generally handled by the compiler team. However, this will result in some amount of ecosystem breakage, so I wanted to see if the lang team had any objections.

@Mark-Simulacrum
Copy link
Member

The future compatibility lint support will only land in 1.59 in Cargo, right? So stable users aren't seeing those warnings yet?

Can you confirm that in all cases (as found by Crater) we expect a cargo update -p some-crate to fix the issue? Can we change to a hard error, but with that recommendation for a cycle or so? In any case, it would be great to have the PR description contain the instructions for each case in a short snippet we can copy into release notes.

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2022
@JohnCSimon JohnCSimon removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 20, 2022
@bors
Copy link
Contributor

bors commented Oct 29, 2022

☀️ Try build successful - checks-actions
Build commit: ea0b8b54c698a32b6a15d1474117ead6958cef0e (ea0b8b54c698a32b6a15d1474117ead6958cef0e)

@Aaron1011
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-93275 created and queued.
🤖 Automatically detected try build ea0b8b54c698a32b6a15d1474117ead6958cef0e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 29, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-93275 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-93275 is completed!
📊 187 regressed and 12 fixed (246739 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 30, 2022
@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2022
@jackh726
Copy link
Member

jackh726 commented Nov 6, 2022

@Aaron1011 I assume you'll triage the crater run and figure out if we can land this

@Aaron1011
Copy link
Member Author

@craterbot
Copy link
Collaborator

🚨 Error: missing desired crates: {"https://crater-reports.s3.amazonaws.com/pr-93275/retry-regressed-list.txt"}

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Aaron1011
Copy link
Member Author

@craterbot
Copy link
Collaborator

👌 Experiment pr-93275-1 created and queued.
🤖 Automatically detected try build ea0b8b54c698a32b6a15d1474117ead6958cef0e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-93275-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-93275-1 is completed!
📊 129 regressed and 0 fixed (187 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 9, 2022
@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2022
@ForrestOfBarnes
Copy link
Contributor

ForrestOfBarnes commented Jun 11, 2023

Any progress on this? Removing the pretty-print hack would remove one of the last two instances of Token::Interpolated.

@Dylan-DPC
Copy link
Member

Closing this as the crater run still needs to be triäged and it's inactive for a while now

@Dylan-DPC Dylan-DPC closed this Oct 5, 2023
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.