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

make unsupported_calling_conventions a hard error #129935

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 3, 2024

This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were not many crates hitting this even when the lint was originally added.

This should get cratered, and I assume then it needs a t-compiler FCP. (t-compiler because this looks entirely like an implementation oversight -- for the vast majority of ABIs, we already have a hard error, but some were initially missed, and we are finally fixing that.)

Fixes #87678

@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2024

@bors try

@RalfJung RalfJung force-pushed the unsupported_calling_conventions branch from d933cf5 to 0c5a9b3 Compare September 3, 2024 16:56
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
…ions, r=

make unsupported_calling_conventions a hard error

This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang#86231 (comment)) even when the PR was landed.

This should get cratered, and I assume then it needs a t-compiler FCP.

Fixes rust-lang#88397
@bors
Copy link
Contributor

bors commented Sep 3, 2024

⌛ Testing commit 0c5a9b3 with merge 498fce2...

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2024

Damn, this needs a change in the reference.^^

@ehuss
Copy link
Contributor

ehuss commented Sep 3, 2024

Damn, this needs a change in the reference.^^

Waiting 24 hours will also unblock, since beta week ends.

@bors
Copy link
Contributor

bors commented Sep 3, 2024

☀️ Try build successful - checks-actions
Build commit: 498fce2 (498fce24f39c1ecbb0abe08824e72162da853341)

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2024

@craterbot check
The fact that crater is Linux-only is actually good here; on Windows the ABI keeps being accepted on all targets, only on other OSes does it get rejected.

@craterbot
Copy link
Collaborator

👌 Experiment pr-129935 created and queued.
🤖 Automatically detected try build 498fce2
🔍 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 Sep 3, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-129935 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129935 is completed!
📊 29 regressed and 3 fixed (508635 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

@RalfJung
Copy link
Member Author

RalfJung commented Sep 9, 2024

6 crates on crates.io are affected, those all look like genuine regressions. They are all rarely used (the one with most downloads has 12k total downloads, that one also has not seen any updates in 8 years).

I have filed issues for the 4 crates that saw updates in the last 5 years, see the backlinks above.

@RalfJung RalfJung force-pushed the unsupported_calling_conventions branch from 0c5a9b3 to 2620cb2 Compare September 9, 2024 06:20
@RalfJung
Copy link
Member Author

RalfJung commented Sep 9, 2024

@fee1-dead I think this is then ready for review.

Does it need a t-compiler FCP because it is a breaking change?

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the unsupported_calling_conventions branch from c3cca9a to 4de549d Compare October 19, 2024 07:42
@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the unsupported_calling_conventions branch from 4de549d to e9bec19 Compare October 19, 2024 08:31
@RalfJung RalfJung force-pushed the unsupported_calling_conventions branch from e9bec19 to de3cbf3 Compare October 20, 2024 13:23
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 21, 2024
@rfcbot
Copy link

rfcbot commented Oct 21, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 22, 2024

📌 Commit de3cbf3 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 22, 2024
fmease added a commit to fmease/rust that referenced this pull request Oct 22, 2024
…ntions, r=compiler-errors

make unsupported_calling_conventions a hard error

This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang#86231 (comment)) even when the lint was originally added.

This should get cratered, and I assume then it needs a t-compiler FCP. (t-compiler because this looks entirely like an implementation oversight -- for the vast majority of ABIs, we already have a hard error, but some were initially missed, and we are finally fixing that.)

Fixes rust-lang#87678
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#129935 (make unsupported_calling_conventions a hard error)
 - rust-lang#130432 (rust_for_linux: -Zregparm=<N> commandline flag for X86 (rust-lang#116972))
 - rust-lang#131697 (`rt::Argument`: elide lifetimes)
 - rust-lang#131954 (shave 150ms off bootstrap)
 - rust-lang#131982 (Represent `hir::TraitBoundModifiers` as distinct parts in HIR)
 - rust-lang#132017 (Update triagebot.toml)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Oct 22, 2024

⌛ Testing commit de3cbf3 with merge 1de57a5...

@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #129935!

Tested on commit 1de57a5.
Direct link to PR: #129935

💔 reference on linux: test-pass → test-fail (cc @Havvy @matthewjasper @ehuss).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Oct 22, 2024
Tested on commit rust-lang/rust@1de57a5.
Direct link to PR: <rust-lang/rust#129935>

💔 reference on linux: test-pass → test-fail (cc @Havvy @matthewjasper @ehuss).
@bors
Copy link
Contributor

bors commented Oct 22, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 1de57a5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 22, 2024
@bors bors merged commit 1de57a5 into rust-lang:master Oct 22, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 22, 2024
@RalfJung
Copy link
Member Author

Reference should be fixed by rust-lang/reference#1600

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1de57a5): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Max RSS (memory usage)

Results (primary 1.7%, secondary -0.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.7% [1.7%, 1.7%] 1
Regressions ❌
(secondary)
1.6% [1.6%, 1.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) 1.7% [1.7%, 1.7%] 1

Cycles

Results (primary -2.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 781.081s -> 779.742s (-0.17%)
Artifact size: 333.69 MiB -> 333.70 MiB (0.00%)

@RalfJung RalfJung deleted the unsupported_calling_conventions branch October 22, 2024 07:23
ehuss added a commit to ehuss/reference that referenced this pull request Oct 22, 2024
rust-lang/rust#129935 made
`unsupported_calling_conventions` a hard-error, which in turn makes this
test fail.
@ehuss
Copy link
Contributor

ehuss commented Oct 22, 2024

@RalfJung There is an additional test that needs to be fixed. I posted rust-lang/reference#1659 to fix that.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 7, 2024
…ions, r=compiler-errors

make unsupported_calling_conventions a hard error

This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang#86231 (comment)) even when the lint was originally added.

This should get cratered, and I assume then it needs a t-compiler FCP. (t-compiler because this looks entirely like an implementation oversight -- for the vast majority of ABIs, we already have a hard error, but some were initially missed, and we are finally fixing that.)

Fixes rust-lang#87678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for future-incompatibility lint unsupported_calling_conventions