Wf check closure's args while borrow checking its parent

[ShoyuVanilla]

View all comments

Fixes #151637

The actuall cause wasn't that the closure's return type is not being wf checked.
For example, the following code is errored out as intended:

struct Wrap<T: Default>(T);

fn error<T>(x: T) {
    || Wrap(x);
    //~^ ERROR: the trait bound `T: Default` is not satisfied
}

The problem was that we are adding implied bound T: 'static even when borrowck-ing the closures.

struct Wrap<T: 'static>(T);

fn no_error<T>(x: T) {
    || Wrap(x);
}

[ShoyuVanilla]

Filtering external bounds here regresses this

rust/tests/ui/impl-trait/issues/issue-104815.rs

Line 63 in eeb94be

.map(|&n| Container::new(n, &resolver))

[ShoyuVanilla]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 484ac15 (484ac156cc381bb2cfc74c800423322f261d074d, parent: eeb94be79adc9df7a09ad0b2421f16e60e6d932c)

[rust-timer]

Finished benchmarking commit (484ac15): comparison URL.

Overall result: no relevant changes - no action needed

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.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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 (secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.4%	[2.9%, 7.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-4.5%, -4.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 481.498s -> 479.757s (-0.36%)
Artifact size: 395.88 MiB -> 397.89 MiB (0.51%)

[ShoyuVanilla]

@craterbot check

[craterbot]

👌 Experiment pr-153027 created and queued.
🤖 Automatically detected try build 484ac15
🔍 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

[ShoyuVanilla]

This doesn't feel correct. I think I should rather add implied bounds as before and directly propagate them again as requirements.
@craterbot cancel

[craterbot]

🗑️ Experiment pr-153027 deleted!

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

[lcnr]

This doesn't feel correct. I think I should rather add implied bounds as before and directly propagate them again as requirements.
@craterbot cancel

hmm, I do feel like only having implied bounds involving the late-bound things from the closure signature seems correct to me 🤔

[ShoyuVanilla]

hmm, I do feel like only having implied bounds involving the late-bound things from the closure signature seems correct to me 🤔

Yeah, only keeping bounds that involve late-bound params feels right to me, too.

But more precisely, I’m currently leaning toward

I'm a bit more inclined to: adding all the implied bounds as before, but gathering the implied bounds don't involve the late bounds and throwing them as propagated closure requirements to the parent body.

I don't have a solid logical basis for this yet 😅, but:

1. I tried this approach and it avoids the ugly FIXME with the RegionSubAlias bounds I’m not happy with here: Wf check closure's args while borrow checking its parent #153027 (comment)
2. I'm not even sure such cases actually exist, but I have a vague (and admittedly ungrounded) worry that dropping “legitimate” implied bounds, i.e., ones that can be proven from the parent body and don't mention late-bound params, could cause unexpected borrowck failures.
   Even if those bounds are provable by the parent, removing them outright means the closure gets borrowck-ed without assumptions that the parent could have supplied.
   Conceptually, the mental model I’m gravitating toward is:
   "Closure: Hey, parent, if you want to define me with this signature, these implied bounds are required. I'll borrowck myself assuming they hold, and you're responsible for proving them."

[ShoyuVanilla]

Added a bit messy experimental PoC commit: 96312b1 😅

[lcnr]

#153027 (comment) I think this should allow more code as propagating requirements from closures doesn't propagate them perfectly, so having more knowledge about the external regions inside of the closure is useful to make it easier to only propagate the correct things.

Looking at your impl, is there a reason we can't lazily go over the UniversalRegionRelations and propagate all the ones which only mention external regions to the parent at this point?

The actual cause wasn't that the closure's return type is not being wf checked.

Though separately, another fix would be to require the closure signature to be well-formed in the parent, would it? It's fine for the closure to assume that its signature is entirely wf if the parent body just explicitly checks that the signature is wf

[ShoyuVanilla]

Looking at your impl, is there a reason we can't lazily go over the UniversalRegionRelations and propagate all the ones which only mention external regions to the parent at this point?

Not really. I was just doing quick PoC with a messy implementation and it ICEd 😅

Though separately, another fix would be to require the closure signature to be well-formed in the parent, would it? It's fine for the closure to assume that its signature is entirely wf if the parent body just explicitly checks that the signature is wf

Yeah, this sounds like a better fix 👍

[ShoyuVanilla]

@bors try @rust-timer queue @craterbot check

[rust-timer]

Error occurred while parsing comment: Invalid command argument @craterbot (there may be no spaces around the = character)

[ShoyuVanilla]

@rust-timer queue
@craterbot check

[craterbot]

👌 Experiment pr-153027 created and queued.
🤖 Automatically detected try build 7612e2d
⚠️ Try build based on commit c23ddc5, but latest commit is b909428. Did you forget to make a new try build?
🔍 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

[ShoyuVanilla]

Oh, it tries to use the previous artifact instead of waiting for the current one 😅
@craterbot cancel

[craterbot]

🗑️ Experiment pr-153027 deleted!

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

[rust-bors]

💥 Test timed out after 21600s

[ShoyuVanilla]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: c86ec27 (c86ec274072509d26d0ee52682a0c94ed012ed89, parent: 55e86c996809902e8bbad512cfb4d2c18be446d9)

[rust-timer]

Finished benchmarking commit (c86ec27): comparison URL.

Overall result: ❌ regressions - 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.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	1.0%	[0.2%, 1.4%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	1

Max RSS (memory usage)

Results (primary -1.0%, secondary -1.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-1.6%	[-2.0%, -1.1%]	2
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Cycles

Results (secondary 4.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.6%	[4.6%, 4.6%]	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: 486.952s -> 486.921s (-0.01%)
Artifact size: 395.04 MiB -> 395.12 MiB (0.02%)

[ShoyuVanilla]

@craterbot check

[craterbot]

👌 Experiment pr-153027 created and queued.
🤖 Automatically detected try build c86ec27
🔍 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