@bors try @rust-timer queue

@bors try cancel

Try build cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/23089332098

@bors try

☀️ Try build successful (CI)
Build commit: 1edb863 (1edb863e6d15a3bd38cd1bfc3f124a3de6878c25, parent: fd2649988fdc91e056ddd316865d33e812f48a0e)

Finished benchmarking commit (1edb863): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

Binary size

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

Bootstrap: 481.843s -> 481.051s (-0.16%)
Artifact size: 396.85 MiB -> 394.81 MiB (-0.51%)

Yeah, this doesn't feel to me the right way to do the things, either. I think I should look into the problem more deeply and make some correct fix in a long term, but since the problemtaic issues are stable to beta regression, I ended up in somewhat makeshift 😅

Could you instead add a fn resolve_vars_for_fudging which has this special behavior by just ignoring the output of shallow_resolve if its an infer var?

Yeah, this feels better

I don't get why we'd actually limit the closure to only impl FnOnce instead of properly inferring the closure kind as we do normally. That feels like a separate issue here, would be happy for you to look into what happens if we never eagerly infer the closure kind in deduce_closure_signature

I'm already a bit familiar with this closure kind deduction. Gonna be a bit verbose, so I'll continue in a new comment

In the beta,

struct Inv<T, U>(*mut (T, U));
fn pass_through<F>(_: F) -> Inv<F, F> { todo!() }
fn map(_: Inv<impl FnOnce(), impl Fn()>) {}
pub fn traverse() {
    map(pass_through(|| ()))
}

When we fudge the input expectations in map(pass_through(|| ())), we make Inv<?impl FnOnce(), ?impl FnOnce> :> Inv<?F, ?F>, which results into eq relations ?impl FnOnce() = ?F and ?impl Fn() = ?F.

Since the ty vars aren't fully resolved yet, their root variable becomes something feels quite random, the one with the minimal vid index, ?impl FnOnce(). This is somewhat implementation dependent thing here:

With my resolve to root var PR, the pass_through's input type expectation ?F ends up to its root var ?impl FnOnce() from the fudge and therefore, the only predicate we have for the pass_through's input ty is ?impl FnOnce(): impl FnOnce()

This makes us to deduce the closure kind as FnOnce, becase we deduce closure kind from predicates, which leads to {closure}: Fn() -> NoSolution.

But in stable, we do not shallow resolve to root var, so the expectation becomes ?F, which has no Fn trait related predicate.
Thus we infer the closure's kind without any predicates, but modulo upvars, which leads to Fn as || () doesn't have any move or borrow.

For the second code which has a outer concrete wraper type(unary tuple) for the expected input type, the input ty var is resolved into it ((?impl Fn(), )) instead of unresolved ty var both in the beta and the stable, so the closure has {closure}: FnOnce() predicate again, which leads to compilation error for both cases.

I've been thinking this deduce closure kind from predicates behavior somewhat weird since my first PR to rust-lang org - rust-lang/rust-analyzer#16472 😅 (it has been closed, but superseded by my another PR)

rust-analyzer was deducing the kind of a closure purely depending on its upvars, and I thought that makes more sense, but anyway, the source of truth for rust-analyzer is rustc 😄

what happens if we never eagerly infer the closure kind in deduce_closure_signature

So, I can say that the problematic regression(or breaking change) that I meant to fix with that PR will happen if we do so, at least.

struct Foo<F: std::ops::FnOnce()>(F);

fn main() {
    let mut x = String::new();
    let y = Foo(|| {
        x = String::from("foo");
    });
    (y.0)();
}

The above is compiled fine with the current rustc, but if we deduce the kind for the closure with its upvars, it would result in FnMut and the last line inside the main function would emit a mutability error.

☀️ Try build successful (CI)
Build commit: bd1c579 (bd1c579a9f06e65c30a9927c80221375e9804aab, parent: 79d2026ae87386ccbe8fc729d130e5e298959a48)

@bors r=lcnr

📌 Commit b77739a has been approved by lcnr

It is now in the queue for this repository.

CI has been stuck at the same line of output for at least half an hour.

@bors yield

Auto build was cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/23272823066

The next pull request likely to be tested is #154062.

Scheduling: Run a mixture of rollup and non-rollup PRs.

@bors p=5

☀️ Test successful - CI
Approved by: lcnr
Duration: 3h 18m 57s
Pushing 1f7f8ea to main...

Comparing 8b86f48 (parent) -> 1f7f8ea (this PR)

Test differences

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 1f7f8ea0721a3b1eb73e6c6d25cccb371434b320 --output-dir test-dashboard

Job duration changes

1. dist-x86_64-apple: 1h 39m -> 2h 2m (+23.5%)
2. x86_64-gnu-stable: 2h 37m -> 2h 13m (-15.4%)
3. aarch64-apple: 2h 53m -> 2h 27m (-14.9%)
4. x86_64-msvc-ext3: 1h 40m -> 1h 54m (+14.4%)
5. test-various: 2h 8m -> 1h 52m (-11.9%)
6. dist-x86_64-illumos: 1h 38m -> 1h 50m (+11.8%)
7. dist-x86_64-llvm-mingw: 1h 56m -> 1h 43m (-11.7%)
8. i686-gnu-nopt-1: 2h 23m -> 2h 7m (-11.3%)
9. dist-aarch64-apple: 1h 48m -> 2h (+10.6%)
10. i686-gnu-2: 1h 44m -> 1h 33m (-10.4%)

Finished benchmarking commit (1f7f8ea): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

Binary size

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

Bootstrap: 480.842s -> 481.515s (0.14%)
Artifact size: 395.02 MiB -> 394.79 MiB (-0.06%)

@ShoyuVanilla @lcnr just for my understanding: the preferred fix here in the end was merging this patch instead of reverting #151380, correct?

(a revert was discussed on Zulip)

thanks

perf triage:

Improvements mostly outweigh regressions. Secondary regressions in projection-caching and regression-31157 seem to just undo part of the wins from #151380, so this still seems like a net improvement overall.

@rustbot label: +perf-regression-triaged

beta backport declined as per compiler team on Zulip.