discard default auto trait impls if explicit ones exist (rebase of #85048) by Dd...

Contributor

Author

r? @lcnr

Ddystopia

changed the title ~~auto trait candidate selection is unsound (rebase of #84857)~~

auto trait candidate selection is unsound (rebase of #85048)

Ddystopia

changed the title ~~auto trait candidate selection is unsound (rebase of #85048)~~

discard default auto trait impls if explicit ones exist (rebase of #85048)

Contributor

thanks for looking into this lets start with crater

@bors try

Contributor

Trying commit cb2716b with merge 262225c...

This comment has been minimized.

Contributor

💔 Test failed - checks-actions

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

compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs

Contributor

oh, this PR is now breaking tokio-util that is unexpected and feels wrong

Contributor

error[E0277]: `impl Future<Output = Result<(), tokio::sync::mpsc::error::SendError<_>>>` cannot be sent between threads safely
Error:   --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-util-0.6.9/src/sync/mpsc.rs:43:43
   |
43 |             inner: ReusableBoxFuture::new(make_future(None)),
   |                    ---------------------- ^^^^^^^^^^^^^^^^^ `impl Future<Output = Result<(), tokio::sync::mpsc::error::SendError<_>>>` cannot be sent between threads safely
   |                    |
   |                    required by a bound introduced by this call
   |
   = help: the trait `Send` is not implemented for `impl Future<Output = Result<(), tokio::sync::mpsc::error::SendError<_>>>`
note: required by a bound in `ReusableBoxFuture::<T>::new`
  --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-util-0.6.9/src/sync/reusable_box.rs:21:33
   |
19 |     pub fn new<F>(future: F) -> Self
   |            --- required by a bound in this associated function
20 |     where
21 |         F: Future<Output = T> + Send + 'static,
   |                                 ^^^^ required by this bound in `ReusableBoxFuture::<T>::new`

		if !has_impl {
		candidates.vec.push(AutoImplCandidate)
		}
		}

Contributor

ah, this is wrong for opaque types we need to always emit consider AutoImplCandidates for them if they don't also have a ProjectionCandidate. This is a bit of a mess in the old solver.

The issue is that we consider all impls to potentially apply for opaque types, but we should instead actually leak their underlying type and check whether it implements the auto trait

cc @compiler-errors given that we've recently talked about auto traits and opaques :>

Ddystopia reacted with heart emoji

Contributor

@bors try

Contributor

Trying commit 2cc873f with merge d48499c...

Contributor

Try build successful - checks-actions
Build commit: d48499c (d48499c087dc9ccf49e58dd584b2df4a2c08657c)

1 similar comment

Contributor

@craterbot check

Collaborator

Experiment pr-113312 created and queued.
Automatically detected try build d48499c
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

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

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

Jul 18, 2023

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

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

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.

Contributor

can you change the description of SUSPICIOUS_AUTO_TRAIT_IMPLS to "the rules governing auto traits have recently changed resulting in potential breakage". I think we then want to remove this lint after 1 or 2 more versions. Also change the description to talk about "previously unsound" and "have been recently modified". "This change caused".

r=me after that

@bors rollup=never
@bors delegate+

Contributor

@Ddystopia, you can now approve this pull request!

If @lcnr told you to "r=me" after making some further change, please make that change, then do @bors r=@lcnr

Contributor

Author

lcnr reacted with thumbs up emoji

Contributor

📌 Commit 3715934 has been approved by lcnr

It is now in the queue for this repository.

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

Contributor

Testing commit 3715934 with merge e4c98ca...

Contributor

Test successful - checks-actions
Approved by: lcnr
Pushing e4c98ca to master...

added the merged-by-bors This PR was explicitly merged by bors label

merged commit e4c98ca into

rust-lang:master