-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[SR-13088] Fix false positive downcast unrelated of types that cannot be statically known #32592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
addc6b8
6bcb9a9
43f1401
a563883
2a06563
b09e34a
8ea2a69
ec3acbf
53e1278
c2e060e
d01e808
b14dc2d
969ede9
101a5c1
026f697
31808e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3357,7 +3357,14 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType, | |
const auto &fromElt = fromTuple->getElement(i); | ||
const auto &toElt = toTuple->getElement(i); | ||
|
||
if (fromElt.getName() != toElt.getName()) | ||
// We should only perform name validation if both element have a label, | ||
// because unlabeled tuple elements can be converted to labeled ones | ||
// e.g. | ||
// | ||
// let tup: (Any, Any) = (1, 1) | ||
// _ = tup as! (a: Int, Int) | ||
if ((!fromElt.getName().empty() && !toElt.getName().empty()) && | ||
fromElt.getName() != toElt.getName()) | ||
return failed(); | ||
|
||
auto result = checkElementCast(fromElt.getType(), toElt.getType(), | ||
|
@@ -3527,8 +3534,9 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType, | |
(toType->isAnyObject() || fromType->isAnyObject())) | ||
return CheckedCastKind::ValueCast; | ||
|
||
// A cast from an existential type to a concrete type does not succeed. For | ||
// example: | ||
// If we have a cast from an existential type to a concrete type that we | ||
// statically know doesn't conform to the protocol, mark the cast as always | ||
// failing. For example: | ||
// | ||
// struct S {} | ||
// enum FooError: Error { case bar } | ||
|
@@ -3541,19 +3549,10 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType, | |
// } | ||
// } | ||
// | ||
// Note: we relax the restriction if the type we're casting to is a | ||
// non-final class because it's possible that we might have a subclass | ||
// that conforms to the protocol. | ||
if (fromExistential && !toExistential) { | ||
if (auto NTD = toType->getAnyNominal()) { | ||
if (!toType->is<ClassType>() || NTD->isFinal()) { | ||
auto protocolDecl = | ||
dyn_cast_or_null<ProtocolDecl>(fromType->getAnyNominal()); | ||
if (protocolDecl && | ||
!conformsToProtocol(toType, protocolDecl, dc)) { | ||
return failed(); | ||
} | ||
} | ||
if (auto *protocolDecl = | ||
dyn_cast_or_null<ProtocolDecl>(fromType->getAnyNominal())) { | ||
if (!couldDynamicallyConformToProtocol(toType, protocolDecl, dc)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you planning to cover existential types in general in a follow-up? let foo: P & Q = ...
let mightSucceed = foo as? ToType There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We didn't think in that protocol composition case I think, I'm working on a followup ... will add this as well :) |
||
return failed(); | ||
} | ||
} | ||
|
||
|
@@ -3616,10 +3615,12 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType, | |
return CheckedCastKind::ValueCast; | ||
} | ||
|
||
// If the destination type can be a supertype of the source type, we are | ||
// performing what looks like an upcast except it rebinds generic | ||
// parameters. | ||
if (fromType->isBindableTo(toType)) | ||
// We perform an upcast while rebinding generic parameters if it's possible | ||
// to substitute the generic arguments of the source type with the generic | ||
// archetypes of the destination type. Or, if it's possible to substitute | ||
// the generic arguments of the destination type with the generic archetypes | ||
// of the source type, we perform a downcast instead. | ||
if (toType->isBindableTo(fromType) || fromType->isBindableTo(toType)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. - generic archetypes
+ archetypes In regards to the comment, is it not the other way around with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if the (up/down)cast is possible |
||
return CheckedCastKind::ValueCast; | ||
|
||
// Objective-C metaclasses are subclasses of NSObject in the ObjC runtime, | ||
|
@@ -3636,8 +3637,8 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType, | |
} | ||
} | ||
|
||
// We can conditionally cast from NSError to an Error-conforming | ||
// type. This is handled in the runtime, so it doesn't need a special cast | ||
// We can conditionally cast from NSError to an Error-conforming type. | ||
// This is handled in the runtime, so it doesn't need a special cast | ||
// kind. | ||
if (Context.LangOpts.EnableObjCInterop) { | ||
auto nsObject = Context.getNSObjectType(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4470,6 +4470,40 @@ TypeChecker::conformsToProtocol(Type T, ProtocolDecl *Proto, DeclContext *DC, | |
return lookupResult; | ||
} | ||
|
||
bool | ||
TypeChecker::couldDynamicallyConformToProtocol(Type type, ProtocolDecl *Proto, | ||
DeclContext *DC) { | ||
// An existential may have a concrete underlying type with protocol conformances | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: |
||
// we cannot know statically. | ||
if (type->isExistentialType()) | ||
return true; | ||
|
||
// A generic archetype may have protocol conformances we cannot know | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: |
||
// statically. | ||
if (type->is<ArchetypeType>()) | ||
return true; | ||
|
||
// A non-final class might have a subclass that conforms to the protocol. | ||
if (auto *classDecl = type->getClassOrBoundGenericClass()) { | ||
if (!classDecl->isFinal()) | ||
return true; | ||
} | ||
|
||
ModuleDecl *M = DC->getParentModule(); | ||
// For standard library collection types such as Array, Set or Dictionary | ||
// which have custom casting machinery implemented in situations like: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "implemented in" sounds a bit off. How about "implemented for", just "for" or "used in"? |
||
// | ||
// func encodable(_ value: Encodable) { | ||
// _ = value as! [String : Encodable] | ||
// } | ||
// we are skipping checking conditional requirements using lookupConformance, | ||
// as an intermediate collection cast can dynamically change if the conditions | ||
// are met or not. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: - // we are skipping checking conditional requirements using lookupConformance,
- // as an intermediate collection cast can dynamically change if the conditions
- // are met or not.
+ //
+ // We skip checking any conditional requirements using lookupConformance,
+ // because an intermediate collection cast can dynamically change depending on
+ // whether these conditions are met. |
||
if (type->isKnownStdlibCollectionType()) | ||
return !M->lookupConformance(type, Proto).isInvalid(); | ||
return !conformsToProtocol(type, Proto, DC).isInvalid(); | ||
} | ||
|
||
/// Exposes TypeChecker functionality for querying protocol conformance. | ||
/// Returns a valid ProtocolConformanceRef only if all conditional | ||
/// requirements are successfully resolved. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.