Skip to content

[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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
addc6b8
[TypeCheckConstraints] Adjusting cases where checked casts that canno…
LucianoPAlmeida Jun 28, 2020
6bcb9a9
[tests] Adding regression tests for SR-13088
LucianoPAlmeida Jun 28, 2020
43f1401
[TypeCheckConstraints] Adjusting comment and adding an extra test cas…
LucianoPAlmeida Jun 29, 2020
a563883
[TypeCheckConstraints] Fixing typos in comments
LucianoPAlmeida Jun 29, 2020
2a06563
[AST] Moving implementation of isCollection from ConstraintSystem to …
LucianoPAlmeida Jun 30, 2020
b09e34a
[TypeCheckConstraints] Adjusting logic to verify specific conformance…
LucianoPAlmeida Jun 30, 2020
8ea2a69
[TypeCheckConstraints] Creating new CheckedCastContextKind::Collectio…
LucianoPAlmeida Jun 30, 2020
ec3acbf
[TypeCheckConstraints] Adjusting logic around generic substitution to…
LucianoPAlmeida Jun 30, 2020
53e1278
[Sema] Adding isKnownStdlibCollectionType and replacing all usages co…
LucianoPAlmeida Jun 30, 2020
c2e060e
[TypeChecker] Reverting fixes around array element types
LucianoPAlmeida Jun 30, 2020
d01e808
[TypeChecker] Abstract logic of check for conditional requirements on…
LucianoPAlmeida Jul 1, 2020
b14dc2d
[TypeChecker] Ajdustinc can conformDynamically conform and adjust rev…
LucianoPAlmeida Jul 1, 2020
969ede9
[TypeChecker] Ajusting comments and fixing typos
LucianoPAlmeida Jul 2, 2020
101a5c1
[TypeChecker] Adjusting existential and archetype logic to check insi…
LucianoPAlmeida Jul 2, 2020
026f697
[TypeChecker] Adjusting minor and adding existential check into could…
LucianoPAlmeida Jul 2, 2020
31808e8
[TypeChecker] Adjusting comments
LucianoPAlmeida Jul 2, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/swift/AST/Types.h
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,10 @@ class alignas(1 << TypeAlignInBits) TypeBase {

/// Check if this is a nominal type defined at the top level of the Swift module
bool isStdlibType();

/// Check if this is either an Array, Set or Dictionary collection type defined
/// at the top level of the Swift module
bool isKnownStdlibCollectionType();

/// If this is a class type or a bound generic class type, returns the
/// (possibly generic) class.
Expand Down
10 changes: 10 additions & 0 deletions lib/AST/Type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,16 @@ bool TypeBase::isStdlibType() {
return false;
}

bool TypeBase::isKnownStdlibCollectionType() {
if (auto *structType = getAs<BoundGenericStructType>()) {
auto &ctx = getASTContext();
auto *decl = structType->getDecl();
return decl == ctx.getArrayDecl() || decl == ctx.getDictionaryDecl() ||
decl == ctx.getSetDecl();
}
return false;
}

/// Remove argument labels from the function type.
Type TypeBase::removeArgumentLabels(unsigned numArgumentLabels) {
// If there is nothing to remove, don't.
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/CSBindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ bool TypeVarBindingProducer::computeNext() {
auto srcLocator = binding.getLocator();
if (srcLocator &&
srcLocator->isLastElement<LocatorPathElt::ApplyArgToParam>() &&
!type->hasTypeVariable() && CS.isCollectionType(type)) {
!type->hasTypeVariable() && type->isKnownStdlibCollectionType()) {
// If the type binding comes from the argument conversion, let's
// instead of binding collection types directly, try to bind
// using temporary type variables substituted for element
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3373,7 +3373,7 @@ bool MissingMemberFailure::diagnoseInLiteralCollectionContext() const {

auto parentType = getType(parentExpr);

if (!isCollectionType(parentType) && !parentType->is<TupleType>())
if (!parentType->isKnownStdlibCollectionType() && !parentType->is<TupleType>())
return false;

if (isa<TupleExpr>(parentExpr)) {
Expand Down
5 changes: 0 additions & 5 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,6 @@ class FailureDiagnostic {
llvm::function_ref<void(GenericTypeParamType *, Type)> substitution =
[](GenericTypeParamType *, Type) {});

bool isCollectionType(Type type) const {
auto &cs = getConstraintSystem();
return cs.isCollectionType(type);
}

bool isArrayType(Type type) const {
auto &cs = getConstraintSystem();
return bool(cs.isArrayType(type));
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3708,7 +3708,7 @@ bool ConstraintSystem::repairFailures(
// func foo<T>(_: [T]) {}
// foo(1) // expected '[Int]', got 'Int'
// ```
if (isCollectionType(rhs)) {
if (rhs->isKnownStdlibCollectionType()) {
std::function<Type(Type)> getArrayOrSetType = [&](Type type) -> Type {
if (auto eltTy = isArrayType(type))
return getArrayOrSetType(*eltTy);
Expand Down
12 changes: 0 additions & 12 deletions lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -878,18 +878,6 @@ Optional<Type> ConstraintSystem::isSetType(Type type) {
return None;
}

bool ConstraintSystem::isCollectionType(Type type) {
if (auto *structType = type->getAs<BoundGenericStructType>()) {
auto &ctx = type->getASTContext();
auto *decl = structType->getDecl();
if (decl == ctx.getArrayDecl() || decl == ctx.getDictionaryDecl() ||
decl == ctx.getSetDecl())
return true;
}

return false;
}

bool ConstraintSystem::isAnyHashableType(Type type) {
if (auto st = type->getAs<StructType>()) {
auto &ctx = type->getASTContext();
Expand Down
3 changes: 0 additions & 3 deletions lib/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -3348,9 +3348,6 @@ class ConstraintSystem {
/// element type of the set.
static Optional<Type> isSetType(Type t);

/// Determine if the type in question is one of the known collection types.
static bool isCollectionType(Type t);

/// Determine if the type in question is AnyHashable.
static bool isAnyHashableType(Type t);

Expand Down
45 changes: 23 additions & 22 deletions lib/Sema/TypeCheckConstraints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- if both element
+ if both elements

// 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(),
Expand Down Expand Up @@ -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 }
Expand All @@ -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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
}
}

Expand Down Expand Up @@ -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))
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Jul 4, 2020

Choose a reason for hiding this comment

The 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 isBindableTo – we are trying to bind the archetypes of one to the generic arguments of the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand All @@ -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();
Expand Down
34 changes: 34 additions & 0 deletions lib/Sema/TypeCheckProtocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: // The dynamic type of an existential might conform to the protocol.

// we cannot know statically.
if (type->isExistentialType())
return true;

// A generic archetype may have protocol conformances we cannot know
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: // The dynamic type of an archetype might conform to the protocol.

// 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
Expand Down
11 changes: 11 additions & 0 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,17 @@ ProtocolConformanceRef conformsToProtocol(Type T, ProtocolDecl *Proto,
DeclContext *DC,
SourceLoc ComplainLoc = SourceLoc());

/// This is similar to \c conformsToProtocol, but returns \c true for cases where
/// the type \p T could be dynamically cast to \p Proto protocol, such as a non-final
/// class where a subclass conforms to \p Proto.
///
/// \param DC The context in which to check conformance. This affects, for
/// example, extension visibility.
///
///
/// \returns True if \p T conforms to the protocol \p Proto, false otherwise.
bool couldDynamicallyConformToProtocol(Type T, ProtocolDecl *Proto,
DeclContext *DC);
/// Completely check the given conformance.
void checkConformance(NormalProtocolConformance *conformance);

Expand Down
72 changes: 70 additions & 2 deletions test/Constraints/casts.swift
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,8 @@ func test_tuple_casts_no_warn() {
_ = arr as! [(Foo, Foo, Foo)] // expected-warning {{cast from '[(Any, Any)]' to unrelated type '[(Foo, Foo, Foo)]' always fails}}
_ = tup as! (Foo, Foo, Foo) // expected-warning {{cast from '(Any, Any)' to unrelated type '(Foo, Foo, Foo)' always fails}}

_ = arr as! [(a: Foo, Foo)] // expected-warning {{cast from '[(Any, Any)]' to unrelated type '[(a: Foo, Foo)]' always fails}}
_ = tup as! (a: Foo, Foo) // expected-warning {{cast from '(Any, Any)' to unrelated type '(a: Foo, Foo)' always fails}}
_ = arr as! [(a: Foo, Foo)] // Ok
_ = tup as! (a: Foo, Foo) // Ok
}

infix operator ^^^
Expand Down Expand Up @@ -335,3 +335,71 @@ func test_compatibility_coercions(_ arr: [Int], _ optArr: [Int]?, _ dict: [Strin
// The array can also be inferred to be [Any].
_ = ([] ?? []) as Array // expected-warning {{left side of nil coalescing operator '??' has non-optional type '[Any]', so the right side is never used}}
}

// SR-13088
protocol JSON { }
protocol JSONLeaf: JSON {}
extension Int: JSONLeaf { }
extension Array: JSON where Element: JSON { }

protocol SR13035Error: Error {}
class ChildError: SR13035Error {}

protocol AnyC {
func foo()
}

protocol AnyEvent {}

protocol A {
associatedtype C: AnyC
}

protocol EventA: A {
associatedtype Event
}

typealias Container<Namespace>
= (event: Namespace.Event, c: Namespace.C) where Namespace: EventA

enum ConcreteA: EventA {
struct C: AnyC {
func foo() {}
}

enum Event: AnyEvent {
case test
}
}

func tests_SR13088_false_positive_always_fail_casts() {
// SR-13081
let x: JSON = [4] // [4]
_ = x as? [Any] // Ok

// SR-13035
func SR13035<SomeError: SR13035Error>(_ child: Result<String, ChildError>, _: Result<String, SomeError>) {
let _ = child as? Result<String, SomeError> // Ok
}

func SR13035_1<SomeError: SR13035Error, Child: ChildError>(_ child: Result<String, Child>, parent: Result<String, SomeError>) {
_ = child as? Result<String, SomeError> // Ok
_ = parent as? Result<String, Child> // OK
}

// SR-11434 and SR-12321
func encodable(_ value: Encodable) {
_ = value as! [String : Encodable] // Ok
_ = value as? [String: Encodable] // Ok
}

// SR-13025
func coordinate(_ event: AnyEvent, from c: AnyC) {
switch (event, c) {
case let container as Container<ConcreteA>: // OK
container.c.foo()
default:
break
}
}
}