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 ptr_cast_add_auto_to_object lint into hard error #136764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

traviscross
Copy link
Contributor

@traviscross traviscross commented Feb 9, 2025

In Rust 1.81, we added a FCW lint (including linting in dependencies) against pointer casts that add an auto trait to dyn bounds. This was part of work making casts of pointers involving trait objects stricter, and was part of the work needed to restabilize trait upcasting.

We considered just making this a hard error, but opted against it at that time due to breakage found by crater. This breakage was mostly due to the anymap crate which has been a persistent problem for us.

It's now a year later, and the fact that this is not yet a hard error is giving us pause about stabilizing arbitrary self types and derive(CoercePointee). So let's see about making a hard error of this.

r? ghost

cc @adetaylor @Darksonn @BoxyUwU @RalfJung @compiler-errors @oli-obk @WaffleLapkin

Related:

Tracking:

@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 Feb 9, 2025
@traviscross traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 9, 2025
@rust-log-analyzer

This comment was marked as resolved.

@traviscross traviscross force-pushed the TC/make-ptr_cast_add_auto_to_object-hard-error branch from 7719195 to d5f82f6 Compare February 9, 2025 07:27
@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross force-pushed the TC/make-ptr_cast_add_auto_to_object-hard-error branch from d5f82f6 to 866c3cf Compare February 9, 2025 07:45
@traviscross
Copy link
Contributor Author

@bors try

...in preparation for a crater run.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2025
…o_to_object-hard-error, r=<try>

Make `ptr_cast_add_auto_to_object` lint into hard error

In Rust 1.81, we added a FCW lint (including linting in dependencies) against pointer casts that add an auto trait to dyn bounds.  This was part of work making casts of pointers involving trait objects stricter, and was part of the work needed to restabilize trait upcasting.

We considered just making this a hard error, but opted against it at that time due to breakage found by crater.  This breakage was mostly due to the `anymap` crate which has been a persistent problem for us.

It's now a year later, and the fact that this is not yet a hard error is giving us pause about stabilizing arbitrary self types and `derive(CoercePointee)`.  So let's see about making a hard error of this.

r? ghost

cc `@adetaylor` `@Darksonn` `@BoxyUwU` `@RalfJung` `@compiler-errors` `@oli-obk` `@WaffleLapkin`

Related:

- rust-lang#135881
- rust-lang#136702

Tracking:

- rust-lang#127323
- rust-lang#44874
- rust-lang#123430
@bors
Copy link
Contributor

bors commented Feb 9, 2025

⌛ Trying commit 866c3cf with merge 8cf44c8...

@bors
Copy link
Contributor

bors commented Feb 9, 2025

☀️ Try build successful - checks-actions
Build commit: 8cf44c8 (8cf44c88fc5805dc578c5c987397615b47d2e2b2)

@traviscross
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-136764 created and queued.
🤖 Automatically detected try build 8cf44c8
🔍 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 Feb 9, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-136764 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-136764 is completed!
📊 95 regressed and 2 fixed (580506 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ 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 Feb 10, 2025
@traviscross traviscross added the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 10, 2025
@traviscross
Copy link
Contributor Author

traviscross commented Feb 10, 2025

@rustbot labels +I-lang-nominated

As expected, all the real regressions are due to anymap. Let's discuss what we want to do here.

cc @rust-lang/lang

@traviscross
Copy link
Contributor Author

We talked about this in triage today. The feeling is that the FCW has been out there long enough, so we're within our rights to do this.

@rfcbot fcp merge

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 12, 2025
@traviscross traviscross removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 12, 2025
@traviscross

This comment was marked as resolved.

@rfcbot

This comment was marked as resolved.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 12, 2025
@traviscross
Copy link
Contributor Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 12, 2025

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 12, 2025
@tmandry
Copy link
Member

tmandry commented Feb 12, 2025

@rfcbot reviewed

@rust-lang rust-lang deleted a comment from rfcbot Feb 12, 2025
@traviscross traviscross marked this pull request as ready for review February 12, 2025 16:47
@rustbot
Copy link
Collaborator

rustbot commented Feb 12, 2025

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

The time has come.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 12, 2025
@rfcbot
Copy link

rfcbot commented Feb 12, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 12, 2025
@traviscross traviscross force-pushed the TC/make-ptr_cast_add_auto_to_object-hard-error branch from 866c3cf to 7c2c4ff Compare February 18, 2025 02:25
@bors
Copy link
Contributor

bors commented Feb 19, 2025

☔ The latest upstream changes (presumably #137248) made this pull request unmergeable. Please resolve the merge conflicts.

@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 Feb 22, 2025
@rfcbot
Copy link

rfcbot commented Feb 22, 2025

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.

In Rust 1.81, we added a FCW lint (including linting in dependencies)
against pointer casts that add an auto trait to dyn bounds.  This was
part of work making casts of pointers involving trait objects stricter
which was needed to restabilize trait upcasting.

We considered just making this a hard error at the time, but opted
against it due to breakage found by crater.  This breakage was mostly
due to the `anymap` crate which has been a persistent problem for us.

It's now a year later, and the fact that this is not yet a hard error
is giving us pause about stabilizing arbitrary self types and
`derive(CoercePointee)`.  So let's now make a hard error of this.
@traviscross traviscross force-pushed the TC/make-ptr_cast_add_auto_to_object-hard-error branch from 7c2c4ff to ef337a6 Compare February 22, 2025 23:05
@traviscross
Copy link
Contributor Author

r? @oli-obk

cc @compiler-errors

Comment on lines +1 to +4
# Removed lints

This directory contains tests to confirm that lints that have been
removed do not cause errors and produce the appropriate warnings.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would that ever prevent a problem?

As-in, if you forgot to remove the lint properly surely you'd also forget to add a test? And it seems very unlikely that any compiler change would break removed lints...

Comment on lines -7 to +8
//~^ warning: adding an auto trait `Send` to a trait object in a pointer cast may cause UB later on
//~| warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
//~^ ERROR cannot add auto trait `Send` to dyn bound via pointer cast
//~| NOTE unsupported cast
//~| NOTE this could allow UB elsewhere
//~| HELP use `transmute` if you're sure this is sound
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(curiosity) Any specific reason you are using ERROR instead of error:?

I prefer the latter cause it's closer to the actual compiler output (error[CODE]: message or error: message for code-less errors) and because in my opinion it looks/reads nicer. (but there is no accepted style, so feel free to use whatever of course)

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. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language 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.