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

Bubble up opaque <eq> opaque operations instead of picking an order #114586

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Aug 7, 2023

In case we are in Bubble mode (meaning every opaque type that is defined in the current crate is treated as if it were in its defining scope), we don't try to register an opaque type as the hidden type of another opaque type, but instead bubble up an obligation to equate them at the query caller site. Usually that means we have a DefiningAnchor::Bind and thus can reliably figure out whether an opaque type is in its defining scope. Where we can't, we'll error out, so the default is sound.

With this change we start using AliasTyEq predicates in the old solver, too.

fixes #108498

But also regresses tests/ui/impl-trait/anon_scope_creep.rs. Our use of Bubble for check_opaque_type_well_formed is going to keep biting us.

r? @lcnr @compiler-errors

@rustbot
Copy link
Collaborator

rustbot commented Aug 7, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Aug 7, 2023
@compiler-errors
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Trying commit d64fc80 with merge cde356bd79f103d9897ada6234862d1d3e511791...

@bors
Copy link
Contributor

bors commented Aug 7, 2023

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 7, 2023

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-114586 created and queued.
🤖 Automatically detected try build cde356bd79f103d9897ada6234862d1d3e511791
🔍 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 Aug 7, 2023
//! query, we attempt to actually check the defining anchor, but now we
//! have a situation where the RPIT gets constrained outside its anchor.

// check-pass
Copy link
Member

Choose a reason for hiding this comment

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

Given that this works in the new solver already:

Suggested change
// check-pass
// revisions: current next
//[next] compile-flags: -Ztrait-solver=next
// check-pass

@craterbot
Copy link
Collaborator

🚧 Experiment pr-114586 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114586 is completed!
📊 37 regressed and 3 fixed (343590 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 Aug 11, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 30, 2023

All of the regressions are spurious (disk failures or panics in the metadata encoder, probably also due to disk failures).

@lcnr
Copy link
Contributor

lcnr commented Sep 7, 2023

r=me unless @compiler-errors also wants to take another look

@oli-obk oli-obk force-pushed the patch_tait_rpit_order_check branch from d64fc80 to 930affa Compare September 11, 2023 16:54
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 11, 2023

@bors r=lcnr,compiler-errors

@bors
Copy link
Contributor

bors commented Sep 11, 2023

📌 Commit 930affa has been approved by lcnr,compiler-errors

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 Sep 11, 2023
@bors
Copy link
Contributor

bors commented Sep 11, 2023

⌛ Testing commit 930affa with merge e2b3676...

@bors
Copy link
Contributor

bors commented Sep 11, 2023

☀️ Test successful - checks-actions
Approved by: lcnr,compiler-errors
Pushing e2b3676 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2023
@bors bors merged commit e2b3676 into rust-lang:master Sep 11, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 11, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e2b3676): 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)
-0.4% [-0.4%, -0.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)
- - 0
Regressions ❌
(secondary)
4.8% [4.2%, 5.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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

Binary size

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

Bootstrap: 630.454s -> 633.716s (0.52%)
Artifact size: 317.84 MiB -> 317.88 MiB (0.02%)

@oli-obk oli-obk deleted the patch_tait_rpit_order_check branch September 12, 2023 06:03
.gitignore Show resolved Hide resolved
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 16, 2023
don't globally ignore rustc-ice files

Reverts a change that happened in rust-lang#114586 but is unrelated to that PR and wasn't discussed.

ICE files appearing is somewhat of a nuisance, but I'd rather clean them up than have them accumulate in my source folder. `@oli-obk` if you want to ignore them you can add them to your local `.git/info/exclude`.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2023
Rollup merge of rust-lang#115885 - RalfJung:dont-ignore-ice, r=oli-obk

don't globally ignore rustc-ice files

Reverts a change that happened in rust-lang#114586 but is unrelated to that PR and wasn't discussed.

ICE files appearing is somewhat of a nuisance, but I'd rather clean them up than have them accumulate in my source folder. `@oli-obk` if you want to ignore them you can add them to your local `.git/info/exclude`.
@aliemjay
Copy link
Member

aliemjay commented Oct 8, 2023

This does not bubble up the AliasRelate obligation to the query caller. Instead, the AliasRelate obligation does not make progress, forcing the query to be ambiguous and unfortunately we swallow ambiguities silently in MIR typeck instead of ICEing as expected.

Here is an example that regressed under the PR:

#![feature(type_alias_impl_trait)]
type Opaque = impl Sized; //~ ERROR unconstrained opaque type
fn get_rpit() -> impl Sized {}
fn query(_: impl FnOnce() -> Opaque) {}
fn test(_: Opaque) {
    query(get_rpit); // although it is constrained here
}

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 9, 2023
…r-errors

delay a bug when encountering an ambiguity in MIR typeck

We shouldn't have any trait selection ambiguities in MIR typeck.

See rust-lang#114586 (comment)

r? `@oli-obk` `@compiler-errors` `@lcnr`
aliemjay added a commit to aliemjay/rust that referenced this pull request Oct 11, 2023
…ler-errors

delay a bug when encountering an ambiguity in MIR typeck

We shouldn't have any trait selection ambiguities in MIR typeck.

See rust-lang#114586 (comment)

r? `@oli-obk` `@compiler-errors` `@lcnr`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2023
Rollup merge of rust-lang#116530 - aliemjay:ice-on-ambiguity, r=compiler-errors

delay a bug when encountering an ambiguity in MIR typeck

We shouldn't have any trait selection ambiguities in MIR typeck.

See rust-lang#114586 (comment)

r? `@oli-obk` `@compiler-errors` `@lcnr`
aliemjay added a commit to aliemjay/rust that referenced this pull request Oct 18, 2023
aliemjay added a commit to aliemjay/rust that referenced this pull request Oct 18, 2023
…li-obk

revert rust-lang#114586

Reverts rust-lang#114586.

cc rust-lang#116877 (not closing until this gets a beta backport)
fixes rust-lang#116684
fixes rust-lang#114586 (comment)

r? `@oli-obk` or `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 18, 2023
Rollup of 5 pull requests

Successful merges:

 - rust-lang#116812 (Disable missing_copy_implementations lint on non_exhaustive types)
 - rust-lang#116856 (Disable effects in libcore again)
 - rust-lang#116865 (Suggest constraining assoc types in more cases)
 - rust-lang#116870 (Don't compare host param by name)
 - rust-lang#116879 (revert rust-lang#114586)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 18, 2023
Rollup merge of rust-lang#116879 - aliemjay:revert-opaque-bubble, r=oli-obk

revert rust-lang#114586

Reverts rust-lang#114586.

cc rust-lang#116877 (not closing until this gets a beta backport)
fixes rust-lang#116684
fixes rust-lang#114586 (comment)

r? `@oli-obk` or `@lcnr`
cuviper pushed a commit to cuviper/rust that referenced this pull request Oct 21, 2023
(cherry picked from commit a1e274f)
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2023
[beta] backports and stage0 bump

- Bump stage0 to released stable compiler
- Hide host effect params from docs rust-lang#116670
- Fix a performance regression in obligation deduplication. rust-lang#116826
- Make `#[repr(Rust)]` and `#[repr(C)]` incompatible with one another rust-lang#116829
- Update to LLVM 17.0.3 rust-lang#116840
- Disable effects in libcore again rust-lang#116856
- revert rust-lang#114586 rust-lang#116879

r? cuviper
xobs pushed a commit to betrusted-io/rust that referenced this pull request Nov 7, 2023
(cherry picked from commit a1e274f)
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
Development

Successfully merging this pull request may close these issues.

ICE when equating TAIT and RPIT in canonical queries
9 participants