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

Eagerly instantiate closure/coroutine-like bounds with placeholders to deal with binders correctly #122267

Merged
merged 2 commits into from Apr 2, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 10, 2024

A follow-up to #119849, however it aims to fix a different set of issues. Currently, we have trouble confirming goals where built-in closure/fnptr/coroutine signatures are compared against higher-ranked goals.

Currently, we don't support goals like for<'a> fn(&'a ()): Fn(&'a ()) because we don't expect the self type of goal to reference any bound regions from the goal, because we don't really know how to deal with the double binder of predicate + self type. However, this definitely can be reached (#121653) -- and in fact, it results in post-mono errors in the case of #112347 where the builtin type (e.g. a coroutine) is hidden behind a TAIT.

The proper fix here is to instantiate the goal before trying to extract the signature from the self type. See final two commits.

r? lcnr

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 10, 2024
@bors
Copy link
Contributor

bors commented Mar 11, 2024

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

@compiler-errors compiler-errors force-pushed the closure-like-goals branch 2 times, most recently from d8d44a6 to 0d9269f Compare March 14, 2024 22:29
@compiler-errors
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 18, 2024

⌛ Trying commit 0d9269f with merge 05804f7...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2024
…r=<try>

Eagerly instantiate closure/coroutine-like bounds with placeholders to deal with binders correctly

A follow-up to rust-lang#119849, however it aims to fix a different set of issues. Currently, we have trouble confirming goals where built-in closure/fnptr/coroutine signatures are compared against higher-ranked goals.

Currently, we don't support goals like `for<'a> fn(&'a ()): Fn(&'a ())` because we don't expect the self type of goal to reference any bound regions from the goal, because we don't really know how to deal with the double binder of predicate + self type. However, this definitely can be reached (rust-lang#121653) -- and in fact, it results in post-mono errors in the case of rust-lang#112347 where the builtin type (e.g. a coroutine) is hidden behind a TAIT.

The proper fix here is to instantiate the goal before trying to extract the signature from the self type. See final two commits.

r? lcnr
@bors
Copy link
Contributor

bors commented Mar 18, 2024

☀️ Try build successful - checks-actions
Build commit: 05804f7 (05804f728f276cf7ecedbf71c814df440d37f293)

@lcnr lcnr 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 Mar 19, 2024
@lcnr
Copy link
Contributor

lcnr commented Mar 19, 2024

this prevents an codegen ICE. Please add it as a test

trait Trait {
    type Assoc<'a>: FnOnce(&'a ());
}

impl Trait for () {
    type Assoc<'a> = fn(&'a ());
}

trait Indir {
    fn break_me() {}
}

impl<F: Trait> Indir for F
where
    for<'a> F::Assoc<'a>: FnOnce(&'a ()),
{
    fn break_me() {}
}

fn foo<F: Trait>() {
    F::break_me()
}

fn main() {
    foo::<()>();
}

@rust-lang/types we previously failed to handle builtin Fn-trait (and related traits) impls if the self type referenced a bound variable. We simply ended up considering such goals to not hold, which is incomplete. This mostly affects manually written where-bounds such as for<'a> fn(&'a ()): FnOnce(&'a ()). It also resulted in codegen ICE in somewhat artificial cases. This constraint is unnecessary, as we can simply avoid it by instantiating higher ranked goals more eagerly.

This PR allows more code to pass and can in likely very artificial cases be breaking (by fixing coherence/changing candidate selection). I am confident enough that this won't break anything that I am going to start the FCP right away, even before we've finished the crater run. We can always reconsider if any breakage is found

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 19, 2024

Team member @lcnr 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 Mar 19, 2024
@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-122267 created and queued.
🤖 Automatically detected try build 05804f7
🔍 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 Mar 19, 2024
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 22, 2024
@rfcbot
Copy link

rfcbot commented Mar 22, 2024

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

@craterbot
Copy link
Collaborator

🚧 Experiment pr-122267 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-122267 is completed!
📊 5 regressed and 2 fixed (428726 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 Mar 26, 2024
@compiler-errors
Copy link
Member Author

Crater run is clean.

One failure is lu_packets, which is known bad (it has impl-sorting dependent overflow errors). Others are disk out of space, except for wpilog-rs which has no apparent failure reason and builds fine locally.

@compiler-errors compiler-errors force-pushed the closure-like-goals branch 2 times, most recently from 2496bf9 to b8526f0 Compare March 26, 2024 14:45
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 1, 2024
@rfcbot
Copy link

rfcbot commented Apr 1, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

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 adding comment

//@[next] compile-flags: -Znext-solver
//@ build-pass

trait Trait {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
trait Trait {
// Regression test for incomplete handling of Fn-trait goals,
// fixed in #122267.
trait Trait {

@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Apr 2, 2024

📌 Commit 09ea3f9 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 Apr 2, 2024
@bors
Copy link
Contributor

bors commented Apr 2, 2024

⌛ Testing commit 09ea3f9 with merge e2cf2cb...

@bors
Copy link
Contributor

bors commented Apr 2, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 2, 2024
@bors bors merged commit e2cf2cb into rust-lang:master Apr 2, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 2, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e2cf2cb): 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)

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

Cycles

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

Binary size

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

Bootstrap: 668.516s -> 667.053s (-0.22%)
Artifact size: 315.68 MiB -> 315.75 MiB (0.02%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants