rust icon indicating copy to clipboard operation
rust copied to clipboard

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

Open compiler-errors opened this issue 1 year ago • 1 comments

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

compiler-errors avatar Mar 10 '24 05:03 compiler-errors

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

bors avatar Mar 11 '24 19:03 bors

@bors try

compiler-errors avatar Mar 18 '24 16:03 compiler-errors

:hourglass: Trying commit 0d9269fe4e6abb8ba24de906c52710202d0b1367 with merge 05804f728f276cf7ecedbf71c814df440d37f293...

bors avatar Mar 18 '24 16:03 bors

:sunny: Try build successful - checks-actions Build commit: 05804f728f276cf7ecedbf71c814df440d37f293 (05804f728f276cf7ecedbf71c814df440d37f293)

bors avatar Mar 18 '24 18:03 bors

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

lcnr avatar Mar 19 '24 12:03 lcnr

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

  • [x] @BoxyUwU
  • [ ] @aliemjay
  • [x] @compiler-errors
  • [x] @jackh726
  • [x] @lcnr
  • [ ] @nikomatsakis
  • [x] @oli-obk
  • [x] @spastorino

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 avatar Mar 19 '24 12:03 rfcbot

@craterbot check

compiler-errors avatar Mar 19 '24 15:03 compiler-errors

:ok_hand: Experiment pr-122267 created and queued. :robot: Automatically detected try build 05804f728f276cf7ecedbf71c814df440d37f293 :mag: You can check out the queue and this experiment's details.

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

craterbot avatar Mar 19 '24 15:03 craterbot

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

rfcbot avatar Mar 22 '24 15:03 rfcbot

:construction: Experiment pr-122267 is now running

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

craterbot avatar Mar 24 '24 23:03 craterbot

:tada: Experiment pr-122267 is completed! :bar_chart: 5 regressed and 2 fixed (428726 total) :newspaper: Open the full report.

:warning: If you notice any spurious failure please add them to the blacklist! :information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot avatar Mar 26 '24 13:03 craterbot

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 avatar Mar 26 '24 14:03 compiler-errors

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.

rfcbot avatar Apr 01 '24 15:04 rfcbot

@bors r=lcnr

compiler-errors avatar Apr 02 '24 03:04 compiler-errors

:pushpin: Commit 09ea3f93ee6497247172a80ba17493748d08b64c has been approved by lcnr

It is now in the queue for this repository.

bors avatar Apr 02 '24 03:04 bors

:hourglass: Testing commit 09ea3f93ee6497247172a80ba17493748d08b64c with merge e2cf2cb30388385f0fe6b406a31a3f9841a72a62...

bors avatar Apr 02 '24 05:04 bors

:sunny: Test successful - checks-actions Approved by: lcnr Pushing e2cf2cb30388385f0fe6b406a31a3f9841a72a62 to master...

bors avatar Apr 02 '24 07:04 bors

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

rust-timer avatar Apr 02 '24 10:04 rust-timer