Copy generics from functions to Return Position Impl Traits in HIR lowering #101345

spastorino · 2022-09-02T21:17:56Z

Going to write some good description here. Opening to allow reviews as I write a proper description.

This is going to allow proper support for RPITITs and async fns in traits.
#101224 can be rebased on top of this one.

cc @oli-obk @cjgillot @jackh726 @compiler-errors (unsure if somebody else is missing in the list of interested people)

I still need to make some improvements of code but please do not wait for that to review. I've already spotted some things that I want to fix.

Adds a feature flag to use the new code only in certain tests to be able to make progress step by step.

We're going to need where clauses too.

craterbot · 2022-09-03T02:06:16Z

🚧 Experiment pr-101345 is now running

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

cjgillot

This PR is huge, and will probably take a while to review. Do you have a possibility to split it up?

I wonder if all this business around RPIT/TAIT/async should go in a separate module, just to isolate it from the rest of lowering?

Do we have a test with fn foo<T: Trait<fn(&u8) -> &u8>>()? I don't understand how this case works.

compiler/rustc_ast_lowering/src/lib.rs

cjgillot · 2022-09-03T17:36:34Z

compiler/rustc_resolve/src/late/lifetimes.rs

+
+                                match hir_map.get(parent) {
+                                    hir::Node::Item(&hir::Item { kind: hir::ItemKind::OpaqueTy(..), .. }) => {
+                                        // do not warn


Why do we need this?

Because due to this 'static lifetime trick we end with 'static in the where clauses of the RPIT. This silences the warning for this case.

Document this in the source

compiler/rustc_ast_lowering/src/lib.rs

craterbot · 2022-09-04T03:43:11Z

🎉 Experiment pr-101345 is completed!
📊 6816 regressed and 4 fixed (242594 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

spastorino · 2022-09-05T20:55:19Z

This PR is huge, and will probably take a while to review. Do you have a possibility to split it up?

I'd be all to simplify this or split into different things. I don't see how given that this is all about this RPIT change. There are some commits that are unrelated but really minor and I don't think it'd simplify things. I'm all ears to make whatever changes are needed to simplify the review of this.

I wonder if all this business around RPIT/TAIT/async should go in a separate module, just to isolate it from the rest of lowering?

Maybe, I'm willing to keep doing work on lowering, should we even meet and discuss some changes to simplify this code?. I was talking with you, @nikomatsakis and @oli-obk but maybe if we meet and make some simple plan I could work on things.

Do we have a test with fn foo<T: Trait<fn(&u8) -> &u8>>()? I don't understand how this case works.

I'm not sure I understand the exact case you want to test. Can you write the complete test case?. I'd assume that you want to return an RPIT, otherwise this PR would be completely unrelated but just in case if you can write the test I can properly check it.

BTW: I've gone over all your comments, commented and marked as resolved as a way to organize myself over the review better. Feel free to reopen unaddressed things or if you have more thoughts comment and unresolve.

oli-obk · 2022-09-06T08:59:27Z

compiler/rustc_ast_lowering/src/lib.rs

-        hir::TyKind::OpaqueDef(hir::ItemId { def_id: opaque_ty_def_id }, lifetimes)
+        let result = hir::TyKind::OpaqueDef(hir::ItemId { def_id: opaque_ty_def_id }, generic_args);
+        debug!("lower_opaque_impl_trait = {:?}", result);
+        result


you can add ret to the #[instrument] on the function to generate this very code that you wrote by hand here

oli-obk

RIP (review in progress) 😆

oli-obk · 2022-09-06T09:01:59Z

compiler/rustc_middle/src/ty/generics.rs

@@ -337,6 +337,9 @@ impl<'tcx> GenericPredicates<'tcx> {
        if let Some(def_id) = self.parent {
            tcx.predicates_of(def_id).instantiate_into(tcx, instantiated, substs);
        }
+
+        debug!("instantiate_into: predicates={:#?} substs={:#?}", instantiated.predicates, substs);


This happens a lot, use trace!

oli-obk · 2022-09-06T09:03:39Z

compiler/rustc_resolve/src/late/lifetimes.rs

@@ -525,6 +525,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
        }
    }

+    #[tracing::instrument(level = "debug", skip(self))]


after a rebase you can just use instrument without the tracing:: prefix

oli-obk · 2022-09-06T09:08:29Z

compiler/rustc_ast_lowering/src/lib.rs

-                let param = self.resolver.get_remapped_def_id(param);
-
-                hir::LifetimeName::Param(param, p_name)
+                if self.in_scope_generics.contains(&param) {
+                    hir::LifetimeName::Static
+                } else {
+                    let param = self.resolver.get_remapped_def_id(param);
+                    hir::LifetimeName::Param(param, p_name)
+                }


Please document this. From the naming in_scope_generics it is not obvious why these are remapped to 'static

oli-obk · 2022-09-06T09:09:59Z

compiler/rustc_resolve/src/late/lifetimes.rs

+
+                                match hir_map.get(parent) {
+                                    hir::Node::Item(&hir::Item { kind: hir::ItemKind::OpaqueTy(..), .. }) => {
+                                        // do not warn


Document this in the source

oli-obk · 2022-09-06T09:11:21Z

compiler/rustc_trait_selection/src/traits/wf.rs

+                            // for opaque types introduced via `-> impl Trait` or async fn desugaring,
+                            // the where-clauses are currently kinda bogus so we skip them.
+                            //
+                            // These opaque type references only appear in 1 place (the fn return type)
+                            // and we know they are well-formed in that location if the fn's where-clauses are met.
+                            //
+                            // The only other place this type will propagate to is the return type of a fn call,
+                            // and it of course must satisfy the fn where clauses, so we know the type will be well-formed
+                            // in that context as well.\


wait... if we are entirely ignoring where bounds, why are we copying them at all?

oli-obk · 2022-09-06T09:13:33Z

compiler/rustc_typeck/src/astconv/mod.rs

+                assert!(matches!(
+                    ty::impl_trait_origin(tcx, did),
+                    None | Some(hir::OpaqueTyOrigin::FnReturn(_))
+                        | Some(hir::OpaqueTyOrigin::TyAlias)


why did the TyAlias case change?

Is this reachable for opaque types from other crates or can we avoid checking the None case now?

oli-obk · 2022-09-06T09:13:54Z

compiler/rustc_typeck/src/astconv/mod.rs

+                let ty = tcx.mk_opaque(def_id, tcx.intern_substs(&substs));
+                debug!(?ty);
+                return ty;


again, use ret on #[instrument]

oli-obk · 2022-09-06T09:16:53Z

compiler/rustc_typeck/src/collect.rs

+    debug!("gather_explicit_predicates_of(predicates={:?})", predicates);
+    debug!("gather_explicit_predicates_of(ast_generics={:?})", ast_generics);


if you're using instrument, don't mention the function name in the tracing statements again

Suggested change

debug!("gather_explicit_predicates_of(predicates={:?})", predicates);

debug!("gather_explicit_predicates_of(ast_generics={:?})", ast_generics);

debug!(?predicates);

debug!(?ast_generics);

oli-obk · 2022-09-06T09:17:18Z

compiler/rustc_typeck/src/collect/item_bounds.rs

+        let result = tcx.arena.alloc_from_iter(bounds.predicates(tcx, item_ty));
+        debug!("opaque_type_bounds(predicates={:?})", result);
+        result


#[instrument(ret)]

…ontext, r=oli-obk Pass ImplTraitContext as &mut to avoid the need of ImplTraitContext::reborrow `@oli-obk` requested this and other changes as a way of simplifying rust-lang#101345. This is just going to make the diff of rust-lang#101345 smaller. r? `@oli-obk` `@cjgillot`

…iler-errors Add debug calls `@oli-obk` requested this and other changes as a way of simplifying rust-lang#101345. This is just going to make the diff of rust-lang#101345 smaller. r? `@oli-obk` `@cjgillot`

cjgillot · 2022-09-08T08:28:07Z

@spastorino could you summarize the motivation for this PR? It's a spread across several zulip threads over 6 months, and I'm afraid I don't have a comprehensive view any more, and the articulation you expect with #101224.

…pi_changes, r=oli-obk Allow lower_lifetime_binder receive a closure ``@oli-obk`` requested this and other changes as a way of simplifying rust-lang#101345. This is just going to make the diff of rust-lang#101345 smaller. r? ``@oli-obk`` ``@cjgillot``

…li-obk Introduce lowering_arena to avoid creating AST nodes on the fly `@oli-obk` requested this and other changes as a way of simplifying rust-lang#101345. This is just going to make the diff of rust-lang#101345 smaller. r? `@oli-obk` `@cjgillot`

spastorino · 2022-09-15T17:17:26Z

We are going to take a different route.

Revert "Introduce lowering_arena to avoid creating AST nodes on the fly" This reverts commit d9a1faa (rust-lang#101499). This was originally part of rust-lang#101345 which has now been closed as a different approach is taken now. r? `@oli-obk` cc `@spastorino`

spastorino added 30 commits September 2, 2022 10:42

Set up new approach for impl trait in return position

7bbe7b6

Adds a feature flag to use the new code only in certain tests to be able to make progress step by step.

Make RPIT not inherit generics from its function

b3cc702

Call mk_param_from_def for all params

bae1deb

Copy generic parameters from function to impl trait opaque type

a5f8780

Test nested impl trait in return position

c1f8fbd

Add generic type and const args to OpaqueDef

2cccbc4

Handle all kind of generic args when converting to ty

bb28711

param_env def_id is the one of the opaque type for RPIT

f60f609

Properly gather explicit predicates of opaque type

a71699d

is_impl_trait_defn doesn't have any effect now with RPIT

a311717

register_member_constraints is 0 for RPIT

4642ec9

gather_explicit_predicates_of return empty for OpaqueTyOrigin::AsyncFn

35a98a7

Properly type check type parameter predicates for RPIT

fee7dc6

Properly handle Const generics

df9356d

Pass Generics to lower impl trait instead of generic params

79eeffa

We're going to need where clauses too.

Copy generic param bounds and where clauses from function to RPIT

d102712

Remap on lower_res

8d26963

Lower lifetime bounds/predicates as static if not part of RPIT bounds

a08ff68

Add APIT test for RPIT v2

cce5412

Add bounds regression test for RPIT v2

3b4ab9d

Add generic associated type test for RPIT v2

be33fa2

Add where bounds test for RPIT v2

c67ab47

Lower APIT generics to RPIT

064675d

Copy generic params from outer impl/trait blocks to RPIT

50bc776

Add debug call on rustc_middle generics

8f20d42

Supress warn on artificial static lifetimes for RPIT

98d43e5

instrument type param predicates and bounds in generic fns

cfa73f8

Add APIT with lifetimes test

9118087

Add a simple APIT test

3049a8d

Add lifetimes bounds test

8b5c082

cjgillot reviewed Sep 3, 2022

View reviewed changes

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 Sep 4, 2022

spastorino added 4 commits September 5, 2022 16:24

Add debug_asserts back

0fadeed

call lower_span before adding it to GenericArg::Type

835d7de

Move itctx creation to outside loop

971641f

Extend Rustdocs of lower_lifetime_binder

d45f1d3

oli-obk reviewed Sep 6, 2022

View reviewed changes

This was referenced Sep 6, 2022

Pass ImplTraitContext as &mut to avoid the need of ImplTraitContext::reborrow #101493

Merged

Allow lower_lifetime_binder receive a closure #101496

Merged

Introduce lowering_arena to avoid creating AST nodes on the fly #101499

Merged

spastorino mentioned this pull request Sep 6, 2022

Add debug calls #101503

Merged

compiler-errors mentioned this pull request Sep 6, 2022

Normalize before erasing late-bound regions in equal_up_to_regions #101437

Merged

tjallingt mentioned this pull request Sep 8, 2022

ImplTraitContext doesn't need to be mutable #101552

Closed

spastorino mentioned this pull request Sep 8, 2022

fix(generic_const_exprs): Fix predicate inheritance for children of opaque types #99801

Merged

spastorino closed this Sep 15, 2022

Noratrieb mentioned this pull request Dec 18, 2022

Revert "Introduce lowering_arena to avoid creating AST nodes on the fly" #105879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy generics from functions to Return Position Impl Traits in HIR lowering #101345

Copy generics from functions to Return Position Impl Traits in HIR lowering #101345

spastorino commented Sep 2, 2022

craterbot commented Sep 3, 2022

cjgillot left a comment

cjgillot Sep 3, 2022

spastorino Sep 5, 2022

oli-obk Sep 6, 2022

craterbot commented Sep 4, 2022

spastorino commented Sep 5, 2022

oli-obk Sep 6, 2022

oli-obk left a comment

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

oli-obk Sep 6, 2022

cjgillot commented Sep 8, 2022

spastorino commented Sep 15, 2022

		debug!("gather_explicit_predicates_of(predicates={:?})", predicates);
		debug!("gather_explicit_predicates_of(ast_generics={:?})", ast_generics);

Copy generics from functions to Return Position Impl Traits in HIR lowering #101345

Copy generics from functions to Return Position Impl Traits in HIR lowering #101345

Conversation

spastorino commented Sep 2, 2022

craterbot commented Sep 3, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craterbot commented Sep 4, 2022

spastorino commented Sep 5, 2022

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Sep 8, 2022

spastorino commented Sep 15, 2022