Use mapSignatureFunctionType on SubscriptDecls #23457
Conversation
| @@ -2311,7 +2320,12 @@ CanType ValueDecl::getOverloadSignatureType() const { | |||
| if (isa<VarDecl>(this)) { | |||
| defaultSignatureType = TupleType::getEmpty(getASTContext()); | |||
| } else { | |||
| defaultSignatureType = getInterfaceType()->getCanonicalType(); | |||
| defaultSignatureType = mapSignatureFunctionType( | |||
There was a problem hiding this comment.
Does it matter whether we map the type before or after we add the curried self param? It means that we won't strip inout from the self param but it wasn't clear to me whether addCurriedSelfType already deals with that.
There was a problem hiding this comment.
addCurriedSelfType() does not mark the self parameter as inout even if the storage is mutating. So I guess it doesn't matter if you add it before or after.
lib/AST/Decl.cpp
Outdated
| @@ -2221,6 +2221,15 @@ static Type mapSignatureFunctionType(ASTContext &ctx, Type type, | |||
| bool isMethod, | |||
| bool isInitializer, | |||
| unsigned curryLevels) { | |||
| if (auto errorType = type->getAs<ErrorType>()) { | |||
There was a problem hiding this comment.
Can you explain why this part was necessary?
There was a problem hiding this comment.
In test/IDE/complete_where_clause.swift, the subscript on line 106 results in an ErrorType (even though the underlying original type seems to be correct—I'm not entirely sure what that signifies). When we tried to cast to AnyFunctionType on line 2244 below the compiler crashed. Should this be added as a comment and/or tracked as a bug?
There was a problem hiding this comment.
Should this method assume that type->castTo<AnyFunctionType>() will always succeed and leave any error checking to the caller instead?
There was a problem hiding this comment.
I see, I think is coming from checkRedeclaration(). Instead of destructuring the error type, how about just returning in this case.
|
@slavapestov Does this need more work in order to be mergeable? |
|
@Jumhyn Sorry for the late responses. |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@slavapestov No worries! Thanks for the feedback. Will update the PR with your suggestion this evening. |
Jumhyn
force-pushed
the
subscript-escaping-overload
branch
from
a809d26
to
427bdc5
Compare
|
@slavapestov Updated. |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@slavapestov Any chance you could shed some light on the failure here? Looks like there's a |
|
Is this going to be source-breaking? That is, were there any programs that compiled previously that now will not compile? cc @hamishknight |
|
This is technically a source breaking change, though in the discussion of SR-10088 it was felt that it's fairly unlikely that anyone's relying on overloading a subscript function-typed parameter by escaping-ness. |
|
:-/ I agree, but a core team member should weigh in. @jckarter, @DougGregor? |
|
I agree it seems unlikely. The only source compatibility suite failure looks like an UPASS: |
|
"it seems unlikely" = "this is acceptable source breakage without going through the full evolution review process"? |
|
Sorry, yeah, that's my opinion. |
|
I think Harlan just un-XFAILed Lark, so I think it's okay to ignore the UPASS. If Slava's good with this then I guess it's good to go. :-) |
|
@slavapestov Given the above, can this be merged? |
When we call
getOverloadSignatureType()on aSubscriptDecl, usemapSignatureFunctionTypeto perform the same transform that we use for other function types. This makes the allowed overloading behavior consistent betweenAbstractFunctionDeclandSubscriptDecl.Resolves SR-10088.