delegation: supporting inherent impls - #160505
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
delegation: supporting inherent methods
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (38e5ff7): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.9%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.1%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 489.838s -> 487.916s (-0.39%) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
delegation: supporting inherent impls
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (40975b9): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.1%, secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.2%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 458.664s -> 461.027s (0.52%) |
This comment has been minimized.
This comment has been minimized.
1b2180a to
3a919c6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
| } | ||
|
|
||
| pub fn is_method(self, id: DefId) -> bool { |
There was a problem hiding this comment.
Nit: I'd rather inline and remove this.
There was a problem hiding this comment.
It is used in two different crates, so I prefer not removing it.
|
(I'll continue reviewing tomorrow.) |
| .and_then(get_segment) | ||
| .filter(|(_, def_id)| matches!(tcx.def_kind(*def_id), DefKind::Trait)) | ||
| .filter(|(_, def_id)| { | ||
| matches!(tcx.def_kind(*def_id), DefKind::Trait | DefKind::Struct | DefKind::Enum) |
There was a problem hiding this comment.
Could you panic on any unexpected DefKinds here?
And other similar matches on the resolution kind of the parent segment too.
I suspect that most of type namespace DefKinds are actually possible to encounter here.
There was a problem hiding this comment.
Yes, it is possible to encounter other def kinds here, and that is why we added those checks, they prevent ICEs further. Maybe delayed bug is better than panic?
There was a problem hiding this comment.
Or we need to generate error delegation for unexpected def kinds during AST -> HIR lowering and then we can add asserts here.
There was a problem hiding this comment.
Maybe delayed bug is better than panic?
Panic (preferably) or delayed bug, whatever keeps the tests passing.
There was a problem hiding this comment.
Or we need to generate error delegation for unexpected def kinds during AST -> HIR lowering and then we can add asserts here.
In general, it is usually better to make erroneous state (def kinds) impossible as early as possible, although I don't know how convenient it will be in this specific case.
There was a problem hiding this comment.
In general, it is usually better to make erroneous state (def kinds) impossible as early as possible, although I don't know how convenient it will be in this specific case.
Implemented it.
…ochenkov delegation: add tests for delegations to inherent impls Second part of rust-lang/rust#160505, for more convenient reviewing. Part of rust-lang/rust#118212. r? @petrochenkov
…mpl` to `generics.rs`
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready (comments feedback) |
| } | ||
|
|
||
| #[derive(Debug, StableHash)] | ||
| pub enum DelegationInhFuncKind { |
There was a problem hiding this comment.
| pub enum DelegationInhFuncKind { | |
| pub enum DelegationInherentFnKind { |
| // Information about delegations which is used when handling recursive delegations | ||
| // and ensures easy access to delegation-only `LocalDefId`s. | ||
| pub delegation_infos: FxIndexMap<LocalDefId, DelegationInfo>, | ||
| pub delegation_inh_functions_map: |
There was a problem hiding this comment.
| pub delegation_inh_functions_map: | |
| pub delegation_inherent_fn_map: |
| } | ||
| } | ||
|
|
||
| fn opt_inherent_impl_adt(&self, sig_id: DefId) -> Option<(DefId, ty::GenericArgsRef<'hir>)> { |
There was a problem hiding this comment.
This can also be inlined and removed now.
| /// Simple (hack or heuristic) resolution of some delegations to inherent impls | ||
| /// while correct resolution through `ProbeContext` is not available | ||
| /// during AST -> HIR lowering due to query cycles. | ||
| /// FIXME(fn_delegation): correct resolution through `ProbeContext` engine |
There was a problem hiding this comment.
Could you add a comment telling that successful resolutions from this heuristics are not a subset of successful resolutions from the correct approach.
And that to make it somewhat stabilizable we need a second pass in type checking (i.e. when there's no cycles) that makes sure that resolutions from the heuristic match the correct resolutions, or reports errors otherwise.
#160505 (comment)
| 'inh_loop: for inh_impl_id in tcx.inherent_impls(res) { | ||
| let assoc_items = tcx.associated_items(*inh_impl_id); | ||
| let mut candidates = assoc_items | ||
| .filter_by_name_unhygienic(ident.name) |
There was a problem hiding this comment.
// FIXME: use correct identifier hygiene|
|
||
| let map = self.r.delegation_inh_functions_map.entry(self_type_def_id).or_default(); | ||
|
|
||
| match map.get(ident) { |
There was a problem hiding this comment.
// FIXME: use correct identifier hygiene
View all comments
This PR adds support for delegation to inherent impl functions on the delegation side.
Support for inherent impls in delegation consists of two problems: we need to resolve inherent function through
ProbeContextroutine and then we need to generate delegation function knowing theDefIdof the signature function. The first problem is a fundamental problem given current compiler architecture, and it is not solved in this PR. To imitate working resolution for tests we adopt simple resolution by name only in inherent impls (not trait impls, which would work if we implement fair resolution throughProbeContext). Aresolve_type_relative_delegationsquery was created which tries to resolve unresolved delegations after resolve stage. In future, when we will be able to fairly resolve delegations throughProbeContextcontents of this query can be changed and all other logic implemented in this pull request will work.Free to inherent impl
Unlike free to trait delegation where we generated explicit
Selfparam, here we just use default parameter.Trait to inherent impl
In trait to inherent impl delegation we replace the type of self parameter from impl's type to
Selfgeneric param (if the signature function is a method).Note that we didn't specified target expression, so we would get errors like:
Trait impl to inherent impl
Here the resolution should look signature in trait as in other cases where we delegate from trait impl. We generate function whose signature matches the resolved function in trait. We propagate only child generics if they are not specified.
Inherent impl to inherent impl
In inherent impl to inherent impl delegation we replace signature self type with delegation parent self type in case of methods.
We did not specify target expression so we would get errors like:
Generics
After some experiments I think that we should force user to always specify generics for parent segment of delegation to inherent impls. Consider the following example and imagine that we can use fair resolution through
ProbeContext:How to resolve
X::foo? If we generate parent generics (fn foo<T, U>() { X::<T, U>::foo() }) which clauses should we inherit? It is impossible to determine which function to reuse, and despite the fact that there may be some cases where it is possible, I don't think that we should write heuristics for that. So always specifying parent generics seems to be a good option. Also I think we should ban infers in parent segment too.One implementation aspect of how we map generic args for signature and predicates inheritance, as we inherit predicates not from the ADT declaration but from the impl block we need to take generic args from this impl, not from the declaration. So indices of generic args are taken from the impl block and then they are used in mapping and future instantiation:
Other concerns
Glob and list delegations
List delegations are supported, glob delegations are not supported:
Self type adjustments and target expression deletion
Adjustments for receiver are applied, adjustments for other parameters whose types contain
Selfare not applied asSelfacts as a type alias to the struct, not a generic param which will can get replaced. The deletion of target expression should work as before.Recursive delegations
Works as before, we just check the resolution chain and we do not care whether it came from resolution at resolve stage or from resolution of type relative delegations.
r? @petrochenkov