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

allow opaques to be defined by trait queries, again #113690

Merged
merged 1 commit into from
Jul 19, 2023
Merged

allow opaques to be defined by trait queries, again #113690

merged 1 commit into from
Jul 19, 2023

Conversation

aliemjay
Copy link
Member

This basically reverts #112963.

Moreover, all call-sites of enter_canonical_trait_query can now define opaque types, see the ui test defined-by-user-annotation.rs.

Fixes #113689

r? @compiler-errors @oli-obk

@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 Jul 14, 2023
@aliemjay aliemjay mentioned this pull request Jul 14, 2023
Merged
compiler-errors
compiler-errors reviewed Jul 16, 2023
View reviewed changes
tests/ui/impl-trait/defined-by-trait-resolution.rs
@@ -0,0 +1,12 @@
//! The trait query `foo: Fn() -> u8` is a valid defining use of RPIT.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add revision to this test and the other one demonstrating what it does in the new solver?

// revisions: current next
//[next] compile-flags: -Ztrait-solver=next

Copy link
Member

Choose a reason for hiding this comment

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

(I expect it to pass in the new solver too)

@compiler-errors
Copy link
Member

compiler-errors commented Jul 16, 2023 •
edited
Loading

This looks fine to me, but also cc @lcnr as they may have thoughts about opaques and their behavior esp wrt the transition to next solver. This does add a new DefiningAnchor::Bubble call to the compiler, which I dislike, but it's limited to the old* solver, so it's nbd.

I'll give this a few days for people to respond, and then will r+ as it's reverting a PR that we thought had no side-effects, and fixes an ICE if we hear nothing.

@rustbot blocked

@rustbot rustbot added S-blocked Status: 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 Jul 16, 2023
@lcnr
Copy link
Contributor

lcnr commented Jul 17, 2023

I agree with using propagating opaque constraints from trait queries. While I am unhappy with Bubble, the fix is imo to pass the defining scope as an argument to the trait queries, similar to the old solver. fine with this change 😁

This does add a new DefiningAnchor::Bubble call to the compiler, which I dislike, but it's limited to the new solver, so it's nbd.

s/new/old 😁

@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 18, 2023
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2023

📌 Commit 281c227 has been approved by 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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 18, 2023
@bors
Copy link
Contributor

bors commented Jul 19, 2023

⌛ Testing commit 281c227 with merge b657dc5...

@bors
Copy link
Contributor

bors commented Jul 19, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Jul 19, 2023

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

@bors bors added merged-by-bors This PR was explicitly merged by bors. labels Jul 19, 2023
@bors bors merged commit b657dc5 into rust-lang:master Jul 19, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b657dc5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

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

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

Binary size

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

Bootstrap: 647.107s -> 647.114s (0.00%)

@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 20, 2023
@aliemjay aliemjay deleted the opaque-defined-by-trait branch July 21, 2023 11:55
rust-cloud-vms bot pushed a commit to aliemjay/rust that referenced this pull request Jul 21, 2023
@aliemjay
add tests for next solver in rust-lang#113690
10362df
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 22, 2023
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.73.0, 1.72.0 Jul 22, 2023
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 22, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2023
@bors
Auto merge of rust-lang#113938 - Mark-Simulacrum:beta-backport, r=Mar…
a47f796 
…k-Simulacrum

[beta] backport

This PR backports:

- rust-lang#113690: allow opaques to be defined by trait queries, again
- rust-lang#113631: make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors

It also includes a bump of bootstrap to the released stable.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@compiler-errors compiler-errors

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.

ICE when trait resolution defines opaque types
9 participants
@aliemjay @compiler-errors @lcnr @bors @rust-timer @apiraino @oli-obk @Mark-Simulacrum @rustbot