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.

Sign up for GitHub

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

Jump to bottom

discard default auto trait impls if explicit ones exist #85048

Closed
wants to merge 2 commits into from
Closed

discard default auto trait impls if explicit ones exist #85048

wants to merge 2 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 7, 2021 •
edited
Loading

fixes #84857

definitely still far from being UwU

When implementing an auto trait for &'a SomeType that auto trait is now not implemented for &&T which is unfortunate xx

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lcnr
Copy link
Contributor Author

lcnr commented Jul 8, 2021

don't have the capacity to work on this rn

@lcnr lcnr closed this Jul 8, 2021
@lcnr lcnr mentioned this pull request Jul 8, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2021
@bors
Auto merge of rust-lang#86986 - lcnr:simplify_type, r=nikomatsakis,ol…
69ac533 
…i-obk

extend `simplify_type`

might cause a slight perf inprovement and imo more accurately represents what types there are.

considering that I was going to use this in rust-lang#85048 it seems like we might need this in the future anyways 🤷
@lcnr lcnr reopened this Dec 22, 2021
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 22, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lcnr lcnr force-pushed the auto-trait-fun branch 4 times, most recently from f3d243d to a477613 Compare January 6, 2022 11:49
@lcnr
Copy link
Contributor Author

lcnr commented Jan 6, 2022

@craterbot check

@lcnr
Copy link
Contributor Author

lcnr commented Jan 6, 2022

@craterbot ping

@craterbot
Copy link
Collaborator

🏓 Pong!

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

@lcnr
Copy link
Contributor Author

lcnr commented Jan 6, 2022

@craterbot check

@craterbot
Copy link
Collaborator

🚨 Error: missing start toolchain

🆘 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

@lcnr
Copy link
Contributor Author

lcnr commented Jan 6, 2022

woops

@bors try

@bors
Copy link
Contributor

bors commented Jan 6, 2022

⌛ Trying commit a4776138bb4707af42c3f3a5c56d7ae3005ee406 with merge dc0340a3a9e4e041da9a5966db8733ccb7ac87e0...

@bors
Copy link
Contributor

bors commented Jan 6, 2022

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

@lcnr
Copy link
Contributor Author

lcnr commented Jan 6, 2022

@craterbot check

@craterbot
Copy link
Collaborator

🚨 Error: experiment 'pr-85048-2' not found

🆘 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

@lcnr
Copy link
Contributor Author

lcnr commented Jan 20, 2022

@craterbot run name=pr-85048-2 start=stable end=try#a0aab45a34400a5afa9e3246429a7455540b55d2 mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-85048/retry-regressed-list.txt

@craterbot
Copy link
Collaborator

👌 Experiment pr-85048-2 created and queued.
🔍 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 20, 2022
@nikomatsakis
Copy link
Contributor

@lcnr and I discussed this today. Our conclusion was that while we do believe this PR should land, and it fixes a bug in the auto trait implementation that gave it unexpected semantics, we don't feel good about breaking the Sync impl in https://crates.io/crates/imagequant/4.0.0-beta.2 (link) without a good workaround.

Two possible ways to resolve this:

Either would permit a second impl like impl<T: Send> Send for Cow<'_, T>.

@lcnr talked about modifying the PR to land as a future compatibility warning in the meantime.

@Mark-Simulacrum
Copy link
Member

@craterbot edit p=5

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 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

@craterbot
Copy link
Collaborator

🚧 Experiment pr-85048-2 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-85048-2 is completed!
📊 5 regressed and 2 fixed (47 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 22, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Jan 24, 2022

alright, so we're left with only the expected auto trait fallout. Going to write a lint for that in a different pr for now.

@lcnr lcnr added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2022
This was referenced Jan 24, 2022
Merged
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 31, 2022
@matthiaskrgr
Rollup merge of rust-lang#93267 - lcnr:auto-trait-lint, r=nikomatsakis
ed845d6 
implement a lint for suspicious auto trait impls

cc rust-lang#85048 (comment)

r? `@nikomatsakis`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 31, 2022
@matthiaskrgr
Rollup merge of rust-lang#93267 - lcnr:auto-trait-lint, r=nikomatsakis
478698b 
implement a lint for suspicious auto trait impls

cc rust-lang#85048 (comment)

r? ``@nikomatsakis``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 1, 2022
@matthiaskrgr
Rollup merge of rust-lang#93267 - lcnr:auto-trait-lint, r=nikomatsakis
2707e49 
implement a lint for suspicious auto trait impls

cc rust-lang#85048 (comment)

r? `@nikomatsakis`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 1, 2022
@matthiaskrgr
Rollup merge of rust-lang#93267 - lcnr:auto-trait-lint, r=nikomatsakis
eb01fe8 
implement a lint for suspicious auto trait impls

cc rust-lang#85048 (comment)

r? ``@nikomatsakis``
@nikomatsakis
Copy link
Contributor

Now that #93267 is merged, @lcnr, shall we close this?

@lcnr
Copy link
Contributor Author

lcnr commented Feb 14, 2022

we could also just merge the breakage in the same version as the lint ✨ would prefer that tbh :(

@lcnr lcnr closed this Feb 14, 2022
@lcnr lcnr mentioned this pull request Mar 6, 2023
Closed
@Ddystopia Ddystopia mentioned this pull request Jul 4, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2023
@bors
Auto merge of rust-lang#113312 - Ddystopia:auto-trait-fun, r=lcnr
e4c98ca 
discard default auto trait impls if explicit ones exist (rebase of rust-lang#85048)

Rebase of rust-lang#85048
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Oct 23, 2023
@bors
Auto merge of rust-lang#113312 - Ddystopia:auto-trait-fun, r=lcnr
4eeecae 
discard default auto trait impls if explicit ones exist (rebase of rust-lang#85048)

Rebase of rust-lang#85048
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

auto trait candidate selection is unsound
8 participants
@lcnr @rust-log-analyzer @craterbot @bors @Mark-Simulacrum @nikomatsakis @rust-highfive @rustbot