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.
97ee898 to
06d20c7
Compare
…s, r=petrochenkov delegation: add tests for delegations to inherent impls Second part of rust-lang#160505, for more convenient reviewing. Part of rust-lang#118212. r? @petrochenkov
Rollup merge of #161665 - aerooneqq:delegation-inh-impl-tests, r=petrochenkov delegation: add tests for delegations to inherent impls Second part of #160505, for more convenient reviewing. Part of #118212. r? @petrochenkov
This comment has been minimized.
This comment has been minimized.
06d20c7 to
62f65b6
Compare
| } | ||
| } | ||
|
|
||
| pub fn is_infer(&self) -> bool { |
There was a problem hiding this comment.
| pub fn is_infer(&self) -> bool { | |
| pub fn is_maybe_parenthesised_infer(&self) -> bool { |
| } | ||
| } | ||
|
|
||
| pub fn is_method(self, id: DefId) -> bool { |
There was a problem hiding this comment.
Nit: I'd rather inline and remove this.
| TypeWalker::new(self) | ||
| } | ||
|
|
||
| pub fn opt_param_info(self) -> (Option<u32> /* index */, bool /* is ty or const */) { |
There was a problem hiding this comment.
If this logic isn't used anywhere else, then it's probably not generally useful and can be moved to its point of use in delegation.rs.
|
(I'll continue reviewing tomorrow.) |
| cf.is_break() | ||
| } | ||
|
|
||
| pub fn replace( |
There was a problem hiding this comment.
| let mut args = vec![]; | ||
| while let Some(arg) = self.next(ctx, |ctx| ctx.next_id()) { | ||
|
|
||
| // If there were already generic args in HIR that should be replaced by our args, |
There was a problem hiding this comment.
When can this happen?
| self.check_delegation_to_inherent_impl(&res.parent_args, sig_id, span)?; | ||
|
|
||
| let tcx = self.tcx(); | ||
| let skip_self = !generate_self && tcx.def_kind(tcx.parent(sig_id)) == DefKind::Trait; |
There was a problem hiding this comment.
So generate_self in GenericsResolution now gives an incorrect value that needs to be corrected, perhaps GenericsResolution needs to contain the correct value from the start.
| !arg.is_infer() | ||
| }) | ||
| .ok_or_else(|| { | ||
| self.tcx().dcx().emit_err(DelegationToInherentImplParentContainsInfer { span }) |
There was a problem hiding this comment.
What happens if a nested infer happens here?
Why is it required to produce a special error for a non-nested infer?
| Ok((res, self.resolve_and_generate_generics(delegation, sig_id, span)?)) | ||
| } | ||
|
|
||
| pub(super) fn opt_inherent_impl_adt( |
There was a problem hiding this comment.
This function is only used in generics.rs, could you move it there?
(Same for is_delegation_to_inherent_impl and maybe some other functions.)
| map.insert(*ident, DelegationInhFuncKind::Single(self.r.current_owner.def_id)); | ||
| } | ||
| Some(DelegationInhFuncKind::Single(..)) => { | ||
| map.insert(*ident, DelegationInhFuncKind::Ambig); |
There was a problem hiding this comment.
This is also a heuristic, it won't catch ambiguities like
impl S { fn foo() {} }
type Alias = S;
impl Alias { fn foo() {} }So paths resolved by the heuristic are not even a subset of paths resolved through the correct way.
The "subset" semantics can be enforced by resolving all the type-relative paths in delegation for the second time later, when the probe context is available, and reporting errors if the result is different. (Not in this PR though.)
| tcx: TyCtxt<'_>, | ||
| _: (), | ||
| ) -> FxIndexMap<LocalDefId, TypeRelativeDelegationRes> { | ||
| // FIXME(fn_delegation): fair resolution through `ProbeContext` engine. |
There was a problem hiding this comment.
"Fair" is probably not the right word here, did you mean "честный"? "Honest" or just "correct" would be better.
| DelegationBlockSpecifiedWhenNoParams, UnresolvedDelegationCallee, | ||
| }; | ||
|
|
||
| pub(crate) fn resolve_type_relative_delegations( |
There was a problem hiding this comment.
This also needs to be visibly marked as a hack and a heuristic, see the previous comment in rustc_resolve.
Besides not detecting ambiguities correctly it also doesn't use hygiene correctly, and doesn't correctly process paths with multiple type-relative segments (?).
Probably fine for a prototype, it just needs to be documented.
| .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.
| tcx.arena.alloc_from_iter(sig) | ||
| } | ||
|
|
||
| fn adjust_sig_in_inherent_impl_cases<'tcx>( |
There was a problem hiding this comment.
This needs a comment explaining what's going on.
The explainer in the PR description is good, but parts of it need to be added to the code as comments too.
…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
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