[SR-13088] Fix false positive downcast unrelated of types that cannot be statically known #32592

LucianoPAlmeida · 2020-06-28T21:31:17Z

This adjusts rules around the diagnostics on unrelated types of edge cases for types that are erased or involve existential which may have conditional conformances.

It almost resolves SR-13088.

Resolves aggregates:
SR-13081
SR-13035
SR-11434
SR-12321
SR-13025

TODO:
SR-7187, SR-6112

…t be determined statically were producing misleading warnings

LucianoPAlmeida · 2020-06-28T21:34:41Z

@swift-ci please smoke test

varungandhi-apple · 2020-06-28T23:35:33Z

We need to manually re-run tests after force-pushing. 🙂

@swift-ci please smoke test

lib/Sema/TypeCheckConstraints.cpp

…e for SR13035

LucianoPAlmeida · 2020-06-29T02:12:57Z

@varungandhi-apple I've adjusted the comment and add an extra test case, let me know what you think :)

test/Constraints/casts.swift

lib/Sema/TypeCheckConstraints.cpp

hamishknight

Thanks for taking this on!

lib/Sema/TypeCheckConstraints.cpp

…AST TypeBase

… to stdlib collection type before emit an downcast warning

…nElement to be able to verify special cases within typeCheckCheckedCast for collection elements

… check both subtype and supertype

include/swift/AST/Types.h

lib/AST/Type.cpp

lib/Sema/ConstraintSystem.cpp

lib/Sema/TypeCheckConstraints.cpp

…ntraint system method

…de couldDynamicConform

lib/Sema/TypeCheckConstraints.cpp

…Dynamically conform.

lib/Sema/TypeCheckProtocol.cpp

lib/Sema/TypeCheckConstraints.cpp

hamishknight

Thanks for wading through all my feedback, this looks good to me!

LucianoPAlmeida · 2020-07-02T18:50:34Z

@hamishknight Thanks for the great help here!

LucianoPAlmeida · 2020-07-02T18:50:48Z

@swift-ci Please smoke test

xedin

This is great! Thank you, @LucianoPAlmeida!

LucianoPAlmeida · 2020-07-02T23:33:13Z

Thanks @xedin :)

LucianoPAlmeida · 2020-07-02T23:33:27Z

@swift-ci Please smoke test Windows platform

LucianoPAlmeida · 2020-07-02T23:34:08Z

Seems like window build is not being triggered automatically ...

hamishknight · 2020-07-02T23:40:14Z

@LucianoPAlmeida It was (https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-windows/4769/), but I guess Github didn't update to show it. You can always check by going to https://ci-external.swift.org, and clicking "Pull Request" > "Swift Pull Request Windows".

LucianoPAlmeida · 2020-07-02T23:50:29Z

but I guess Github didn't update to show it

Ahh true, we've seen this in some other PRs

LucianoPAlmeida · 2020-07-03T00:11:05Z

@swift-ci Please smoke test Windows platform

LucianoPAlmeida · 2020-07-03T01:08:00Z

Thank you guys! I'll try to put up a follow up for the remaining two soon :)

xedin · 2020-07-03T04:17:52Z

@LucianoPAlmeida Please don't forget to resolve all of the JIRAs :)

LucianoPAlmeida · 2020-07-03T21:51:41Z

Sure! Just resolved :)

AnthonyLatsis

This looks great overall. The Encodable false-positive in the stdlib has been an eyesore during builds for a quite some time now!

AnthonyLatsis · 2020-07-04T09:58:13Z

lib/Sema/TypeCheckConstraints.cpp

@@ -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,


- if both element + if both elements

AnthonyLatsis · 2020-07-04T11:22:25Z

lib/Sema/TypeCheckProtocol.cpp

+
+  ModuleDecl *M = DC->getParentModule();
+  // For standard library collection types such as Array, Set or Dictionary
+  // which have custom casting machinery implemented in situations like:


"implemented in" sounds a bit off. How about "implemented for", just "for" or "used in"?

AnthonyLatsis · 2020-07-04T11:33:40Z

lib/Sema/TypeCheckConstraints.cpp

+  // 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))


- 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?

Yes, if the (up/down)cast is possible

AnthonyLatsis · 2020-07-04T11:37:59Z

lib/Sema/TypeCheckConstraints.cpp

-        }
+    if (auto *protocolDecl =
+          dyn_cast_or_null<ProtocolDecl>(fromType->getAnyNominal())) {
+      if (!couldDynamicallyConformToProtocol(toType, protocolDecl, dc)) {


Are you planning to cover existential types in general in a follow-up?

let foo: P & Q = ... let mightSucceed = foo as? ToType

We didn't think in that protocol composition case I think, I'm working on a followup ... will add this as well :)

AnthonyLatsis · 2020-07-04T11:38:38Z

lib/Sema/TypeCheckProtocol.cpp

+bool
+TypeChecker::couldDynamicallyConformToProtocol(Type type, ProtocolDecl *Proto,
+                                               DeclContext *DC) {
+  // An existential may have a concrete underlying type with protocol conformances


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

AnthonyLatsis · 2020-07-04T11:49:12Z

lib/Sema/TypeCheckProtocol.cpp

+  if (type->isExistentialType())
+    return true;
+
+  // A generic archetype may have protocol conformances we cannot know


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

AnthonyLatsis · 2020-07-04T11:49:41Z

lib/Sema/TypeCheckProtocol.cpp

+  //  }
+  // we are skipping checking conditional requirements using lookupConformance,
+  // as an intermediate collection cast can dynamically change if the conditions
+  // are met or not.


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.

LucianoPAlmeida requested review from xedin, hamishknight and varungandhi-apple June 28, 2020 21:31

[TypeCheckConstraints] Adjusting cases where checked casts that canno…

addc6b8

…t be determined statically were producing misleading warnings

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 6b9b934 to 47c1baa Compare June 28, 2020 21:33

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 47c1baa to 24d968a Compare June 28, 2020 22:56

[tests] Adding regression tests for SR-13088

6bcb9a9

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 24d968a to 6bcb9a9 Compare June 28, 2020 22:58

varungandhi-apple reviewed Jun 28, 2020

View reviewed changes

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

[TypeCheckConstraints] Adjusting comment and adding an extra test cas…

43f1401

…e for SR13035

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 59e81d1 to 43f1401 Compare June 29, 2020 02:08

varungandhi-apple reviewed Jun 29, 2020

View reviewed changes

test/Constraints/casts.swift Outdated Show resolved Hide resolved

theblixguy reviewed Jun 29, 2020

View reviewed changes

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

theblixguy reviewed Jun 29, 2020

View reviewed changes

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

[TypeCheckConstraints] Fixing typos in comments

a563883

hamishknight reviewed Jun 29, 2020

View reviewed changes

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida added 3 commits June 29, 2020 21:04

[AST] Moving implementation of isCollection from ConstraintSystem to …

2a06563

…AST TypeBase

[TypeCheckConstraints] Adjusting logic to verify specific conformance…

b09e34a

… to stdlib collection type before emit an downcast warning

[TypeCheckConstraints] Creating new CheckedCastContextKind::Collectio…

8ea2a69

…nElement to be able to verify special cases within typeCheckCheckedCast for collection elements

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 3dc5437 to 8ea2a69 Compare June 30, 2020 00:41

[TypeCheckConstraints] Adjusting logic around generic substitution to…

ec3acbf

… check both subtype and supertype

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 3ad1bb1 to ec3acbf Compare June 30, 2020 02:18

LucianoPAlmeida commented Jun 30, 2020

View reviewed changes

include/swift/AST/Types.h Outdated Show resolved Hide resolved

LucianoPAlmeida requested a review from hamishknight June 30, 2020 02:33

hamishknight reviewed Jun 30, 2020

View reviewed changes

include/swift/AST/Types.h Outdated Show resolved Hide resolved

lib/AST/Type.cpp Outdated Show resolved Hide resolved

lib/Sema/ConstraintSystem.cpp Outdated Show resolved Hide resolved

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida added 2 commits June 30, 2020 18:15

[Sema] Adding isKnownStdlibCollectionType and replacing all usages co…

53e1278

…ntraint system method

[TypeChecker] Reverting fixes around array element types

c2e060e

[TypeChecker] Adjusting existential and archetype logic to check insi…

101a5c1

…de couldDynamicConform

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 82842b3 to 101a5c1 Compare July 2, 2020 12:54

hamishknight reviewed Jul 2, 2020

View reviewed changes

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

[TypeChecker] Adjusting minor and adding existential check into could…

026f697

…Dynamically conform.

hamishknight reviewed Jul 2, 2020

View reviewed changes

lib/Sema/TypeCheckProtocol.cpp Outdated Show resolved Hide resolved

lib/Sema/TypeCheckConstraints.cpp Outdated Show resolved Hide resolved

[TypeChecker] Adjusting comments

31808e8

LucianoPAlmeida force-pushed the SR-13081-cast-unrelated branch from 59dec49 to 31808e8 Compare July 2, 2020 18:10

LucianoPAlmeida requested a review from hamishknight July 2, 2020 18:10

hamishknight approved these changes Jul 2, 2020

View reviewed changes

xedin approved these changes Jul 2, 2020

View reviewed changes

LucianoPAlmeida merged commit 28fb66c into swiftlang:master Jul 3, 2020

LucianoPAlmeida deleted the SR-13081-cast-unrelated branch July 3, 2020 01:08

AnthonyLatsis reviewed Jul 4, 2020

View reviewed changes

theblixguy mentioned this pull request Jul 27, 2020

[SR-13303] Erroneous warning shown when casting protocol type to dictionary type #55743

Closed

typesanitizer mentioned this pull request Jun 2, 2021

[SR-14682] Misleading warning on conditional cast from Any to Any? #57032

Closed

[SR-13088] Fix false positive downcast unrelated of types that cannot be statically known #32592

[SR-13088] Fix false positive downcast unrelated of types that cannot be statically known #32592

Uh oh!

Conversation

LucianoPAlmeida commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LucianoPAlmeida commented Jun 28, 2020

Uh oh!

varungandhi-apple commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida commented Jun 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida commented Jul 2, 2020

Uh oh!

LucianoPAlmeida commented Jul 2, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida commented Jul 2, 2020

Uh oh!

LucianoPAlmeida commented Jul 2, 2020

Uh oh!

LucianoPAlmeida commented Jul 2, 2020

Uh oh!

hamishknight commented Jul 2, 2020

Uh oh!

LucianoPAlmeida commented Jul 2, 2020

Uh oh!

LucianoPAlmeida commented Jul 3, 2020

Uh oh!

LucianoPAlmeida commented Jul 3, 2020

Uh oh!

xedin commented Jul 3, 2020

Uh oh!

LucianoPAlmeida commented Jul 3, 2020

Uh oh!

AnthonyLatsis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis Jul 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida commented Jun 28, 2020 •

edited

Loading

varungandhi-apple commented Jun 28, 2020 •

edited

Loading

AnthonyLatsis Jul 4, 2020 •

edited

Loading