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

Reject ?Trait bounds in various places where we unconditionally warned since 1.0 #135841

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

Reject ?Trait bounds in various places where we unconditionally warned since 1.0 #135841

wants to merge 1 commit into from
+86 −62

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jan 21, 2025 •
edited
Loading

fixes #135730
fixes #135809

Also a breaking change, so let's see what crater says.

This has been an unconditional warning since before 1.0

@rustbot
Copy link
Collaborator

rustbot commented Jan 21, 2025

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 Jan 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 21, 2025

HIR ty lowering was modified

cc @fmease

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 21, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2025
@bors
Auto merge of rust-lang#135841 - oli-obk:push-qxlnokwrkkym, r=<try>
173780a 
Reject `impl Trait` within `?Trait` generics or assoc ty bounds

fixes rust-lang#135730

Also a breaking change, so let's see what crater says.

This has been an unconditional warning since *before* 1.0
@bors
Copy link
Contributor

bors commented Jan 21, 2025

⌛ Trying commit 757c75f with merge 173780a...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 21, 2025

☀️ Try build successful - checks-actions
Build commit: 173780a (173780a87744406b7489b28349c0176f14bb3881)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 22, 2025

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-135841 created and queued.
🤖 Automatically detected try build 173780a
🔍 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 22, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-135841 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-135841 is completed!
📊 5 regressed and 4 fixed (571795 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 Jan 25, 2025
@oli-obk oli-obk changed the title Reject impl Trait within ?Trait generics or assoc ty bounds Reject ?Trait bounds in various places where we unconditionally warned since 1.0 Jan 30, 2025
@oli-obk oli-obk added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 30, 2025
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 30, 2025

Hi @rust-lang/types, it turns out we've accepted ?Trait bounds in various places since 1.0 and just emitted an unconditional warning. I have been unable to find the reason for it, but since it doesn't do anything and emitted a warning, it seems no one actually wrote this (crater shows no regressions when turning it into a hard error). Therefor I propose to just turn it into a hard error and subsequently make our lives easier because we don't need to patch situations everywhere in the compiler.

One common source of problems with such bounds is that they can have generics or associated types/consts and all of these just get dropped on the floor and never evaluated, normalized, or otherwise processed in the type system. This leads to fun bugs like not evaluating constants or just ICEing because opaque types never got defined.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 30, 2025 •
edited by BoxyUwU
Loading

Team member @oli-obk 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!

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 Jan 30, 2025
@lcnr
Copy link
Contributor

lcnr commented Jan 30, 2025

cc @rust-lang/lang for info

I think this is fine as a pure types team FCP. Allowing these bounds is causing bugs without any benefit.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

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

rfcbot commented Jan 30, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@compiler-errors compiler-errors

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types 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.

relaxing a non-Sized bounds is a hard-warning ICE could not unify ! with revealed type
9 participants
@oli-obk @rustbot @bors @rust-log-analyzer @craterbot @rfcbot @lcnr @nikomatsakis @compiler-errors