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

ignore implied bounds with placeholders #112422

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

aliemjay
Copy link
Member

@aliemjay aliemjay commented Jun 8, 2023

given the following code:

trait Trait {
    type Ty<'a> where Self: 'a;
}

impl<T> Trait for T {
    type Ty<'a> = () where Self: 'a;
}

struct Foo<T: Trait>(T)
where
    for<'x> T::Ty<'x>: Sized;

when computing the implied bounds from Foo<X> we incorrectly get the bound X: !x from the normalization of for<'x> <X as Trait>::Ty::<'x>: Sized. This is a a known bug! we shouldn't use the constraints that arise from normalization as implied bounds. See #109628.

Ignore these bounds for now. This should prevent later ICEs.

Fixes #112250
Fixes #107409

@rustbot
Copy link
Collaborator

rustbot commented Jun 8, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@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 Jun 8, 2023
@aliemjay
Copy link
Member Author

aliemjay commented Jun 8, 2023

cc @lcnr (because it involves #109628)
r? types

@rustbot rustbot assigned jackh726 and unassigned WaffleLapkin Jun 8, 2023
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

❤️

r=me after nits

Comment on lines +323 to +326
// Because of #109628, we may have unexpected placeholders. Ignore them!
// FIXME(#109628): panic in this case once the issue is fixed.
let bounds = bounds.into_iter().filter(|bound| !bound.has_placeholders());
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move that into the TypeOp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moving this into the TypeOp has no benefit as we would have to duplicate the logic in QueryTypeOp::{perform_query,perform_locally_in_new_solver} as well. It is also more difficult to handle canonicalized query responses - there is no easy way to detect placeholders for example.

Doing it in the query itself is difficult as we need to do OpportunisticRegionResolution before checking for placeholders. Using TypeVisitableExt::has_placeholders to check for erroneus placeholders there does not play well with #109388 as it would yield many false positives.

@BoxyUwU BoxyUwU assigned lcnr and unassigned jackh726 Jun 8, 2023
@lcnr lcnr 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 Jun 8, 2023
@Dylan-DPC
Copy link
Member

@aliemjay any updates on this? thanks

@bors
Copy link
Contributor

bors commented Oct 26, 2023

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

@chrysn
Copy link
Contributor

chrysn commented Nov 14, 2023

I think I have a case similar to #112250 in code at https://gitlab.com/chrysn/coap-message-demos branch embedded-nal-coap-clientserver when run as cargo +nightly run --example std_embedded_nal_coap --features example-std_embedded_nal_coap. Before I start minimizing this for hours I'd like to try this PR, but the code widely depends on newly stabilized features. Would you consider updating?

@rust-cloud-vms rust-cloud-vms bot force-pushed the implied-bounds-placeholders branch from 02f72b3 to af79fd1 Compare November 16, 2023 14:26
@aliemjay
Copy link
Member Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2023
@lcnr
Copy link
Contributor

lcnr commented Nov 17, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Nov 17, 2023

📌 Commit af79fd1 has been approved by lcnr

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 Nov 17, 2023
@bors
Copy link
Contributor

bors commented Nov 17, 2023

⌛ Testing commit af79fd1 with merge 4d7f952...

@bors
Copy link
Contributor

bors commented Nov 17, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 4d7f952 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 17, 2023
@bors bors merged commit 4d7f952 into rust-lang:master Nov 17, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 17, 2023
@aliemjay aliemjay deleted the implied-bounds-placeholders branch November 17, 2023 15:15
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4d7f952): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

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.5% [1.1%, 1.8%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
-2.0% [-3.4%, -1.0%] 6
All ❌✅ (primary) 0.6% [-1.0%, 1.8%] 3

Cycles

Results

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)
-0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

Binary size

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

Bootstrap: 676.982s -> 676.352s (-0.09%)
Artifact size: 313.61 MiB -> 313.63 MiB (0.00%)

@lqd
Copy link
Member

lqd commented Nov 17, 2023

(deep-vector is currently being noisy)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
10 participants