Remap impl-trait lifetimes on HIR instead of AST lowering by cjgillot · Pull Request #129383 · rust-lang/rust

cjgillot · 2024-08-22T00:45:23Z

Current AST->HIR lowering goes out of its way to remap lifetimes for opaque types. This is complicated and leaks into upstream and downstream code.

This PR stops trying to be clever during lowering, and prefers to do this remapping during the HIR->ty lowering. The remapping computation easily fits into the bound var resolution code. Its result can be used in by generics_of and hir_ty_lowering::new_opaque to add the proper parameters and arguments.

See an example on the doc for query opaque_captured_lifetimes.

Based on #129244

Fixes#125249
Fixes#126850

cc @compiler-errors @spastorino
r? @petrochenkov

petrochenkov · 2024-08-22T15:31:20Z

Blocked on #129244.
@rustbot blocked

rustbot · 2024-10-05T11:53:33Z

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

bors · 2024-10-15T08:12:31Z

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

petrochenkov · 2024-10-24T15:04:49Z

Reviewed.
It would be better is someone more familiar with impl trait desugaring details looked at the compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs part.
@rustbot author

cjgillot · 2024-10-26T17:37:23Z

@compiler-errors do you mind taking a look? I'm not sure I correctly implemented the logic around use bounds.

compiler-errors · 2024-10-26T17:48:24Z

- //~^ ERROR ['_: o]
- //~| ERROR ['_: o]
- //~| ERROR `impl Trait` captures lifetime parameter
+ //~^ ERROR []


Ah, this doesn't seem right. Or at least, it changes the semantics of what we mean by "capture all in-scope parameters" for nested opaques.
Without having yet looked at the code, I assume what's happening here is that the nested opaque doesn't see any nested lifetimes because of the use<> from the outer opaque, which shadows the lifetimes that the inner opaque considers to be in-scope. Is that right?

I'm not actually certain if https://rust-lang.github.io/rfcs/3617-precise-capturing.html#summary prescribes what should happen here 🤔 -- it doesn't seem to do so, after having done a brief scan.

I changed the code to recurse and capture all in-scope lifetimes when we have nested opaques.

@compiler-errors I have re-implemented the resolution to walk the scopes and not stop at the closest opaque. Is this closer to what you had in mind?

Yeah, I believe so. If in the future we want opaque type captures to stop at the closest opaque, we can do so; that actually seems like the right behavior, but this should mean there are no observable changes.

compiler-errors · 2024-10-30T16:18:43Z

r=me,petrochenkov after rebase

cjgillot · 2024-10-30T17:47:51Z

@bors r=compiler-errors,petrochenkov rollup=iffy

bors · 2024-10-30T17:47:55Z

📌 Commit 2d74d8f has been approved by compiler-errors,petrochenkov

It is now in the queue for this repository.

…kingjubilee Rollup of 5 pull requests Successful merges: - rust-lang#129383 (Remap impl-trait lifetimes on HIR instead of AST lowering) - rust-lang#132210 (rustdoc: make doctest span tweak a 2024 edition change) - rust-lang#132246 (Rename `rustc_abi::Abi` to `BackendRepr`) - rust-lang#132267 (force-recompile library changes on download-rustc="if-unchanged") - rust-lang#132344 (Merge `HostPolarity` and `BoundConstness`) Failed merges: - rust-lang#132347 (Remove `ValueAnalysis` and `ValueAnalysisWrapper`.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129383 - cjgillot:opaque-noremap, r=compiler-errors,petrochenkov Remap impl-trait lifetimes on HIR instead of AST lowering Current AST->HIR lowering goes out of its way to remap lifetimes for opaque types. This is complicated and leaks into upstream and downstream code. This PR stops trying to be clever during lowering, and prefers to do this remapping during the HIR->ty lowering. The remapping computation easily fits into the bound var resolution code. Its result can be used in by `generics_of` and `hir_ty_lowering::new_opaque` to add the proper parameters and arguments. See an example on the doc for query `opaque_captured_lifetimes`. Based on rust-lang#129244Fixesrust-lang#125249Fixesrust-lang#126850 cc `@compiler-errors` `@spastorino` r? `@petrochenkov`

rustbot assigned petrochenkov Aug 22, 2024

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 22, 2024

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 Aug 22, 2024

cjgillot force-pushed the opaque-noremap branch 2 times, most recently from 8127ba2 to d334955Compare August 24, 2024 00:06

This comment has been minimized.
Sign in to view

cjgillot force-pushed the opaque-noremap branch from d334955 to 1729436Compare August 24, 2024 02:16

This comment was marked as resolved.
Sign in to view

cjgillot force-pushed the opaque-noremap branch from 1729436 to 92cd183Compare September 1, 2024 16:57

This comment was marked as resolved.
Sign in to view

cjgillot force-pushed the opaque-noremap branch 2 times, most recently from c2ad831 to 12b94cdCompare October 5, 2024 11:52

cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 5, 2024

cjgillot marked this pull request as ready for review October 5, 2024 11:53

This comment was marked as resolved.
Sign in to view

cjgillot force-pushed the opaque-noremap branch from 7a0204e to 44ced4fCompare October 11, 2024 22:34

fmease mentioned this pull request Oct 19, 2024
Make sure that outer opaques capture inner opaques's lifetimes even with precise capturing syntax #131789
Merged

petrochenkov reviewed Oct 24, 2024
View reviewed changes

Comment threadcompiler/rustc_resolve/src/late.rs Outdated
Comment threadcompiler/rustc_hir/src/hir.rs Outdated
Comment threadcompiler/rustc_ast_lowering/src/lib.rs
Comment threadcompiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs Outdated

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2024

cjgillot force-pushed the opaque-noremap branch from 44ced4f to e9c3954Compare October 26, 2024 17:36

compiler-errors reviewed Oct 26, 2024
View reviewed changes

This comment has been minimized.
Sign in to view

petrochenkov removed their assignment Oct 26, 2024

cjgillot force-pushed the opaque-noremap branch from b196430 to ad543e4Compare October 27, 2024 15:54

This comment has been minimized.
Sign in to view

cjgillot force-pushed the opaque-noremap branch from ad543e4 to f7beaabCompare October 27, 2024 16:30

compiler-errors approved these changes Oct 30, 2024
View reviewed changes

cjgillot added 6 commits October 30, 2024 16:18

Remap impl-trait lifetimes on HIR instead of AST lowering.
b6e1214

Promote crashes tests to ui.
d693e19

Adapt comments.
d804ef8

Review comments.
27c958f

Promote test.
8f6e0a6

Actually capture all in-scope lifetimes.
2d74d8f

cjgillot force-pushed the opaque-noremap branch from f7beaab to 2d74d8fCompare October 30, 2024 16:55

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 Oct 30, 2024

workingjubilee mentioned this pull request Oct 30, 2024
Rollup of 5 pull requests #132371
Merged

bors merged commit 6b60f03 into rust-lang:masterOct 31, 2024

rustbot added this to the 1.84.0 milestone Oct 31, 2024

cjgillot deleted the opaque-noremap branch October 31, 2024 01:01

This was referenced Oct 31, 2024
ICE: assertion failed: value <= 0xFFFF_FF00#132429
Closed
ICE: impl_trait_in_fn_trait_return index out of bounds #132518
Closed

This was referenced Nov 24, 2024
RPITITs may imply unsound outlives for late-bound args in signature #133427
Closed
Actually use placeholder regions for trait method late bound regions in collect_return_position_impl_trait_in_trait_tys#133428
Merged

lqd mentioned this pull request Dec 26, 2024
ICE: cannot convert "'a/#4" to a region vid#131295
Closed

Uh oh!

Uh oh!

Conversation

cjgillot commented Aug 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Aug 22, 2024

Uh oh!

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

rustbot commented Oct 5, 2024

Uh oh!

This comment was marked as resolved.

bors commented Oct 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Oct 24, 2024

Uh oh!

cjgillot commented Oct 26, 2024

Uh oh!

compiler-errorsOct 26, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errorsOct 26, 2024

Choose a reason for hiding this comment

Uh oh!

cjgillotOct 27, 2024

Choose a reason for hiding this comment

Uh oh!

cjgillotOct 30, 2024

Choose a reason for hiding this comment

Uh oh!

compiler-errorsOct 30, 2024

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Oct 30, 2024

Uh oh!

cjgillot commented Oct 30, 2024

Uh oh!

bors commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cjgillot commented Aug 22, 2024•
edited
Loading

compiler-errorsOct 26, 2024•
edited
Loading