Delegation: partial generics support #123958

Bryanskiy · 2024-04-15T08:39:50Z

PR explainer: https://github.com/Bryanskiy/posts/blob/master/delegation%20in%20generic%20contexts.md
Part of #118212

rustbot · 2024-04-15T08:39:58Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-04-15T08:40:00Z

HIR ty lowering was modified

cc @fmease

Bryanskiy · 2024-04-15T08:40:17Z

@rustbot author

Bryanskiy · 2024-05-16T12:51:49Z

@rustbot ready

Bryanskiy · 2024-05-16T12:52:02Z

cc @petrochenkov

petrochenkov · 2024-05-23T20:25:33Z

I think it would be better to split this work into parts.

First we support generic inheritance for delegating from all contexts to all contexts (from free/trait/impl to free/trait ~~/impl~~).
- Maybe split this into more than one parts too, e.g. support only free to free first.
Only when that is ready we support generic arguments in delegation paths (reuse foo::<u8>, reuse <u8 as Trait>::foo) on top of that.
- Before this is implemented generic arguments can be rejected by a semantic check (including all qpaths, and explicit _s too).

tests/ui/delegation/generics/fn-to-fn.rs

tests/ui/delegation/generics/fn-to-methods.rs

tests/ui/delegation/not-supported.rs

compiler/rustc_hir_typeck/src/errors.rs

compiler/rustc_hir_typeck/src/delegation.rs

petrochenkov · 2024-05-23T20:57:26Z

compiler/rustc_hir_typeck/src/delegation.rs

+    let generics = delegation_resolver.generics_of();
+    let predicates = delegation_resolver.predicates_of();
+
+    ty::DelegationResults { inputs, output, generics, predicates }


Are all components here interdependent and need to be collected together?

They all depend on the pass, which collects parameter definitions and generic arguments. (GenericDefsCollector)

Maybe that pass should be a query instead of lower_delegation_ty, and the later parts can be just called from predicates_of/etc because they don't need memoization.

I also suspect that the data from GenericDefsMap is already collected and available from elsewhere.

I also suspect that the data from GenericDefsMap is already collected and available from elsewhere.

Inference variables are visible only in type check. Entire body type checking is not possible due to a query cycle on the variance pass. Therefore, if there are any data structures, they are not available to us.

Maybe that pass should be a query instead of lower_delegation_ty, and the later parts can be just called from predicates_of/etc because they don't need memoization.

Maybe. I chose this option because all the logic is localized in one module and it is easy to maintain.

petrochenkov · 2024-05-23T21:02:13Z

I did a high level review, but didn't yet read compiler/rustc_hir_typeck/src/delegation.rs, which is the most important part.

compiler/rustc_hir_typeck/src/delegation.rs

petrochenkov · 2024-05-24T15:08:39Z

The explainer should say something about what happens with defaults for generic parameters.
@rustbot author

Bryanskiy · 2024-05-26T18:30:44Z

I think it would be better to split this work into parts.

* First we support generic inheritance for delegating from all contexts to all contexts (from free/trait/impl to free/trait ~/impl~).
  
  * Maybe split this into more than one parts too, e.g. support only free to free first.

* Only when that is ready we support generic arguments in delegation paths (`reuse foo::<u8>`, `reuse <u8 as Trait>::foo`) on top of that.
  
  * Before this is implemented generic arguments can be rejected by a semantic check (including all qpaths, and explicit `_`s too).

From an implementation perspective, there is no difference between free fn - free fn and free fn - trait method delegation. Technically, I can split this PR into 2 parts:

Delegation from free functions to everything else.
Generic arguments.

But I don't understand why the support for generic arguments(explicit inference variables) should be postponed.

compiler/rustc_hir_typeck/src/delegation.rs

petrochenkov · 2024-05-26T22:14:36Z

But I don't understand why the support for generic arguments(explicit inference variables) should be postponed.

This is a separate feature with separate logic (type walking) that has lower priority because it's less likely to be used with glob delegation. Without it generic arguments (including qpaths) could even be removed from the delegation syntax.
Easier to separately review parts that can be reasonably separated.

petrochenkov · 2024-05-26T22:18:28Z

compiler/rustc_hir_typeck/src/delegation.rs

+            TypeckRootCtxt::new_with_infcx(self.tcx, self.def_id, infcx);
+        let param_env = ty::ParamEnv::empty();
+        let fcx = FnCtxt::new(&root_ctxt, param_env, self.def_id);
+        fcx.check_expr_with_expectation_and_args(


I don't think we should ever type check bodies to produce signatures, only signatures should be analyzed to produce signatures.

(Well, until we start implementing step 3, but I'm not sure we even need that part anymore.)

fmease

Some suggestions, comments and questions. I haven't looked at all the changes yet. Nor have I evaluated the overall design/architecture/approach you've taken.

I'm surprised that we need to typeck the body as petrochenkov has mentioned. I wouldn't've expected it to be necessary. However, I can't elaborate on that just yet as I need to go over all the design constraints and the implementation again and have a think. This is just a drive-by review.

fmease · 2024-05-27T12:38:02Z

compiler/rustc_hir_typeck/src/delegation.rs

+    fn collect(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> GenericDefsMap<'tcx> {
+        let mut collector = GenericDefsCollector::new(tcx, def_id);
+        let caller_kind = fn_kind(tcx, def_id.into());
+        // generics are not yet supported for associated delegation items


Suggested change

// generics are not yet supported for associated delegation items

// FIXME(fn_delegation): Support generics on associated delegation items.

fmease · 2024-05-27T12:38:39Z

compiler/rustc_hir_typeck/src/delegation.rs

+}
+
+fn fn_kind<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> FnKind {
+    assert!(matches!(tcx.def_kind(def_id), DefKind::Fn | DefKind::AssocFn));


Suggested change

assert!(matches!(tcx.def_kind(def_id), DefKind::Fn | DefKind::AssocFn));

debug_assert!(matches!(tcx.def_kind(def_id), DefKind::Fn | DefKind::AssocFn));

maybe

fmease · 2024-05-27T12:42:14Z

compiler/rustc_hir_typeck/src/delegation.rs

+        if caller_kind == FnKind::Free {
+            let path = collector.check_call();
+            path.visit_with(&mut collector);
+        }


Maybe panic or error if caller_kind != FnKind::Free?

Error was reported earlier in check_constraints. I added a comment.

fmease · 2024-05-27T12:52:26Z

compiler/rustc_hir_typeck/src/delegation.rs

+
+    // Collect generic parameters definitions during callee type traversal according to
+    // encountered inference variables.
+    fn extract_info_from_def(&mut self, def_id: DefId, args: GenericArgsRef<'tcx>) {


This currently doesn't support const params (const generics), is that intentional? If so, it should at least panic or error when encountering them. Keep other rustc devs in mind that won't be super familiar with fn_delegation but might still stumble upon this code for various reasons. It should be self-documenting (via code and comments).

Const parameters are not inherited, but they are allowed in callee path. I added a comment and updated the explainer.

fmease · 2024-05-27T12:57:20Z

compiler/rustc_hir_typeck/src/delegation.rs

+        match ty.kind() {
+            ty::Adt(adt, args) => self.extract_info_from_def(adt.did(), args),
+            ty::FnDef(did, args) => self.extract_info_from_def(*did, args),
+            _ => {}


I've only read so far. What happens if the delegation path contains _ in "unexpected" positions? Imo, we should emit an error.

While I've read the RFC a while ago and skimmed your explainer, I don't remember the expected behavior of _ inside delegation paths off the top of my head. I'm surprised that we need to match on the ty.kind() here at all, this doesn't seem very general.

Having briefly read some of the comments inside this file, it seems like _s are expected to map to fresh type/const parameters? If so, I don't think your current setup supports reuse func::<(_, _)>;, reuse func::<&_>; or reuse func::<dyn Trait<_>>;?

Might be nice to document any such constraints inside this file. Apologies if my observations are a short-sighted or outright unnecessary, I didn't have the time to familiarize myself with all the details of fn delegation yet.

Yes, all the above cases should be supported, I overlooked this. We decided to postpone _ syntax until generics are supported in all contexts. I'm going to return to this issue later.

fmease · 2024-05-27T13:08:26Z

compiler/rustc_hir_typeck/src/delegation.rs

+            self.tcx,
+            ty::EarlyParamRegion { index: rid.as_u32(), name: param.name },
+        )
+    }


(as noted above, lacks support for const generics)

I added a comment and updated the explainer.

fmease · 2024-05-27T13:08:44Z

compiler/rustc_hir_typeck/src/delegation.rs

+                let args = self.tcx.mk_args_from_iter(std::iter::once(generic_self_ty));
+                caller_sig.instantiate(self.tcx, args)
+            }
+            // here generics are not yet supported


panic or error instead?

Error was reported earlier in check_constraints. I added a comment.

petrochenkov · 2024-05-27T16:50:07Z

compiler/rustc_hir_typeck/src/delegation.rs

+) -> ty::LoweredDelegation<'tcx> {
+    let info = GenericDefsCollector::collect(tcx, def_id);
+    let delegation_resolver = DelegationLowerer::new(tcx, def_id, &info);
+    if let Err(err) = check_constraints(tcx, def_id) {


Nit: this call can be moved to the top now.

…etrochenkov Delegation: support generics for delegation from free functions (The PR was split from rust-lang#123958, explainer - https://github.com/Bryanskiy/posts/blob/master/delegation%20in%20generic%20contexts.md) This PR implements generics inheritance from free functions to free functions and trait methods. #### free functions to free functions: ```rust fn to_reuse<T: Clone>(_: T) {} reuse to_reuse as bar; // desugaring: fn bar<T: Clone>(x: T) { to_reuse(x) } ``` Generics, predicates and signature are simply copied. Generic arguments in paths are ignored during generics inheritance: ```rust fn to_reuse<T: Clone>(_: T) {} reuse to_reuse::<u8> as bar; // desugaring: fn bar<T: Clone>(x: T) { to_reuse::<u8>(x) // ERROR: mismatched types } ``` Due to implementation limitations callee path is lowered without modifications. Therefore, it is a compilation error at the moment. #### free functions to trait methods: ```rust trait Trait<'a, A> { fn foo<'b, B>(&self, x: A, y: B) {...} } reuse Trait::foo; // desugaring: fn foo<'a, 'b, This: Trait<'a, A>, A, B>(this: &This, x: A, y: B) { Trait::foo(this, x, y) } ``` The inheritance is similar to the previous case but with some corrections: - `Self` parameter converted into `T: Trait` - generic parameters need to be reordered so that lifetimes go first Arguments are similarly ignored. --- In the future, we plan to support generic inheritance for delegating from all contexts to all contexts (from free/trait/impl to free/trait /impl). These cases were considered first as the simplest from the implementation perspective.

Delegation: support generics for delegation from free functions (The PR was split from rust-lang/rust#123958, explainer - https://github.com/Bryanskiy/posts/blob/master/delegation%20in%20generic%20contexts.md) This PR implements generics inheritance from free functions to free functions and trait methods. #### free functions to free functions: ```rust fn to_reuse<T: Clone>(_: T) {} reuse to_reuse as bar; // desugaring: fn bar<T: Clone>(x: T) { to_reuse(x) } ``` Generics, predicates and signature are simply copied. Generic arguments in paths are ignored during generics inheritance: ```rust fn to_reuse<T: Clone>(_: T) {} reuse to_reuse::<u8> as bar; // desugaring: fn bar<T: Clone>(x: T) { to_reuse::<u8>(x) // ERROR: mismatched types } ``` Due to implementation limitations callee path is lowered without modifications. Therefore, it is a compilation error at the moment. #### free functions to trait methods: ```rust trait Trait<'a, A> { fn foo<'b, B>(&self, x: A, y: B) {...} } reuse Trait::foo; // desugaring: fn foo<'a, 'b, This: Trait<'a, A>, A, B>(this: &This, x: A, y: B) { Trait::foo(this, x, y) } ``` The inheritance is similar to the previous case but with some corrections: - `Self` parameter converted into `T: Trait` - generic parameters need to be reordered so that lifetimes go first Arguments are similarly ignored. --- In the future, we plan to support generic inheritance for delegating from all contexts to all contexts (from free/trait/impl to free/trait /impl). These cases were considered first as the simplest from the implementation perspective.

Dylan-DPC · 2024-08-14T17:35:20Z

@Bryanskiy any updates on this? thanks

Bryanskiy · 2024-08-15T11:58:59Z

This needs to be significantly reworked, but I don't have time for it right now. I'll come back to it later.

rustbot assigned fmease Apr 15, 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. labels Apr 15, 2024

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 Apr 15, 2024

Bryanskiy force-pushed the delegation-generics branch 3 times, most recently from 71aa0a9 to 142272a Compare May 16, 2024 12:48

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

petrochenkov self-assigned this May 16, 2024

petrochenkov reviewed May 23, 2024

View reviewed changes

petrochenkov reviewed May 24, 2024

View reviewed changes

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 May 24, 2024

Bryanskiy force-pushed the delegation-generics branch from 142272a to dab1670 Compare May 26, 2024 16:40

Bryanskiy force-pushed the delegation-generics branch from dab1670 to ba05af3 Compare May 26, 2024 19:40

petrochenkov reviewed May 26, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/delegation.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_typeck/src/delegation.rs Outdated Show resolved Hide resolved

petrochenkov reviewed May 26, 2024

View reviewed changes

Bryanskiy force-pushed the delegation-generics branch from ba05af3 to 602cde9 Compare May 27, 2024 11:45

This comment has been minimized.

Sign in to view

Bryanskiy force-pushed the delegation-generics branch from 602cde9 to f02af50 Compare May 27, 2024 12:19

This comment has been minimized.

Sign in to view

Bryanskiy force-pushed the delegation-generics branch from f02af50 to a4f8ae0 Compare May 27, 2024 12:35

fmease reviewed May 27, 2024

View reviewed changes

petrochenkov reviewed May 27, 2024

View reviewed changes

Bryanskiy force-pushed the delegation-generics branch from a4f8ae0 to 7634ac5 Compare May 29, 2024 15:28

This comment has been minimized.

Sign in to view

Delegation: partial generics support

f2b1278

Bryanskiy force-pushed the delegation-generics branch from 7634ac5 to f2b1278 Compare May 29, 2024 16:30

Bryanskiy mentioned this pull request Jun 3, 2024

Delegation: support generics for delegation from free functions #125929

Merged

petrochenkov added the F-fn_delegation `#![feature(fn_delegation)]` label Jun 24, 2024

petrochenkov mentioned this pull request Jun 24, 2024

Tracking Issue for function delegation (fn_delegation, RFC 3530) #118212

Open

25 tasks

Bryanskiy closed this Aug 15, 2024

	// generics are not yet supported for associated delegation items
	// FIXME(fn_delegation): Support generics on associated delegation items.

	assert!(matches!(tcx.def_kind(def_id), DefKind::Fn \| DefKind::AssocFn));
	debug_assert!(matches!(tcx.def_kind(def_id), DefKind::Fn \| DefKind::AssocFn));

Delegation: partial generics support #123958

Delegation: partial generics support #123958

Conversation

Bryanskiy commented Apr 15, 2024 • edited Loading

rustbot commented Apr 15, 2024

rustbot commented Apr 15, 2024

Bryanskiy commented Apr 15, 2024

Bryanskiy commented May 16, 2024

Bryanskiy commented May 16, 2024

petrochenkov commented May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Bryanskiy May 26, 2024 • edited Loading

Choose a reason for hiding this comment

petrochenkov May 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented May 23, 2024

petrochenkov commented May 24, 2024

Bryanskiy commented May 26, 2024

petrochenkov commented May 26, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

fmease left a comment • edited Loading

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

fmease May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Bryanskiy May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease May 27, 2024 • edited Loading

Choose a reason for hiding this comment

fmease May 27, 2024 • edited Loading

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

fmease May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Bryanskiy May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Dylan-DPC commented Aug 14, 2024

Bryanskiy commented Aug 15, 2024

Bryanskiy commented Apr 15, 2024 •

edited

Loading

petrochenkov commented May 23, 2024 •

edited

Loading

Bryanskiy May 26, 2024 •

edited

Loading

petrochenkov May 26, 2024 •

edited

Loading

fmease left a comment •

edited

Loading

fmease May 27, 2024 •

edited

Loading

Bryanskiy May 29, 2024 •

edited

Loading

fmease May 27, 2024 •

edited

Loading

fmease May 27, 2024 •

edited

Loading

fmease May 27, 2024 •

edited

Loading

Bryanskiy May 29, 2024 •

edited

Loading