Ensure we are adding T : Differentiable conformance from protocol conditional conformance #77446
+56
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
protocol conditional conformance. Fixes #75711
asl
requested review from
AnthonyLatsis,
hborla,
slavapestov and
jckarter
as code owners
|
@swift-ci please test |
|
Tagging @rxwei @JaapWijnen |
slavapestov
reviewed
Nov 7, 2024
| // generic signature. Update witness substitution map generic signature to | ||
| // have them as well. | ||
| if (auto *derivativeId = witness.getDerivativeFunctionIdentifier()) | ||
| witnessSubs = SubstitutionMap::get(derivativeId->getDerivativeGenericSignature(), |
There was a problem hiding this comment.
Should the correct witnessSubs be computed in Sema? See lib/AST/RequirementEnvironment.cpp
There was a problem hiding this comment.
It seems to be calculated correctly there:
<τ_0_0 where τ_0_0 : Differentiable> is a witness generic thunk signature and the corresponding requirement-to-witness substitution map is:
(substitution_map generic_signature=<Self where Self : P>
(substitution Self ->
(bound_generic_struct_type decl="main.(file)[email protected]:3:8"
(generic_type_param_type depth=0 index=0 param_kind=type)))
(conformance type="Self"
(normal_conformance type="Wrapper<T>" protocol="P"
(assoc_type req="T" type="T")
(assoc_conformance type="Self" proto="Copyable"
(builtin_conformance type="Wrapper<T>" protocol="Copyable"))
(assoc_conformance type="Self" proto="Escapable"
(builtin_conformance type="Wrapper<T>" protocol="Escapable"))
(assoc_conformance type="Self.T" proto="Differentiable"
(abstract_conformance protocol="Differentiable"))
(requirement "T" conforms_to "Differentiable"))))
However, witnessSubs here is:
(substitution_map generic_signature=<T where T : Copyable, T : Escapable>
(substitution T ->
(primary_archetype_type address=0x15c9d8c18 conforms_to="_Differentiation.(file).Differentiable" name="\xCF\x84_0_0"
(interface_type=generic_type_param_type depth=0 index=0 param_kind=type)))
(conformance type="T"
(abstract_conformance protocol="Copyable"))
(conformance type="T"
(abstract_conformance protocol="Escapable")))
So T : Differentiable comes from conditional conformance via substitution and is not part of generic signature.
There was a problem hiding this comment.
@slavapestov So, what do you think about this? Should we change the calculation somehow? For now I just made it conditional for autodiff and only adding T : Differentiable requirement. W/o this we are unable to calculate derivative function type in protocol conformance as we need to have corresponding associated types etc.
There was a problem hiding this comment.
@slavapestov Any further suggestions here?
|
@asl It looks like that the following case is related to the issue which is supposed to be fixed by the PR. With this patch applied, we still have an issue described below. Consider the following (with forgotten When compiling, we get the following crash (see under spoiler). If we uncomment While it looks like that we should emit an error if conditional conformance is missing, we definitely do not want to crash. The error should probably look like the following ( |
@kovdan01 I believe your example is different than the code from the PR: there is no |
@asl Thanks for clarification. I thought it might be related because the crash stack dump looks similar to the one being fixed by this PR. The issue does not seem to be reported before, so created an issue #78370 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #75711