[SR-11421][Diagnostics] Tailored diagnostic for checked downcast with literals #29493

LucianoPAlmeida · 2020-01-28T01:55:10Z

Tailored diagnostic when ConditionalCheckedCastExpr involves a literal subexpr that can be statically coerced.
cc @CodaFi

Resolves SR-11421.

CodaFi · 2020-01-28T04:53:58Z

lib/Sema/CSApply.cpp

+
+      // Special handle for literals conditional checked cast when they can be
+      // statically coerced to the cast type.
+      if (isa<LiteralExpr>(sub)) {


Could you please leave behind a FIXME about revising this behavior? This is a warning for a deeply unintuitive behavior that we need to revisit.

Filed SR-12093 about this.

Could you please leave behind a FIXME about revising this behavior? This is a warning for a deeply unintuitive behavior that we need to revisit.

Sure :)

CodaFi · 2020-01-28T04:54:52Z

lib/Sema/CSApply.cpp

+      // statically coerced to the cast type.
+      if (isa<LiteralExpr>(sub)) {
+        if (auto protocol = TypeChecker::getLiteralProtocol(ctx, sub)) {
+          if (!fromType->isEqual(toType) &&


What happens if the to type is more complex like NSString or NSNumber?

This is still emitted, and the same is also valid for any type that conforms to the actual literal protocol can be coerced e.g.

struct S: ExpressibleByStringLiteral, CustomStringConvertible { typealias StringLiteralType = String var _value: String = "" init(stringLiteral value: Self.StringLiteralType) { self._value = value } var description: String { _value } } let a = "A" as S print(a) // "A"

I'm just adding the test cases :)

Note: When the literal inferred type can be bridged to the cast type, the conditional downcast always succeeds, also added the test cases :)

LucianoPAlmeida · 2020-02-04T04:14:21Z

Just a few minor adjustments, this may be good to review.
cc @CodaFi :)

LucianoPAlmeida · 2020-02-05T17:21:55Z

Offering a fix-it to these situations is not the best approach because of normal cases conditional downcasts are used like

if let a = "a" as? Character {
}

And a fix-it to replace with coercion would not be ideal. So this is now only a better message with a hint to use coercion.
@CodaFi I'm not sure if we can do much more than that considering this case, so let me know what you think 👍

@xedin since you are involved with the latest diagnostics improvements, can you give some thoughts on this one, especially if the message is ideal :)

xedin · 2020-02-07T23:15:05Z

@LucianoPAlmeida I have this open and going to try and give it a look soon. Sorry, I have been swamped lately...

LucianoPAlmeida · 2020-02-08T15:04:38Z

@LucianoPAlmeida I have this open and going to try and give it a look soon. Sorry, I have been swamped lately...

@xedin No worries, And thank you for taking the time to look at this :)

xedin

LGTM! Looks like to make SR-12093 work we need to do a revamp of how checked casts are handled in the solver. Having it warn in CSApply for now is fine but ultimately we should be able to detect this in the solver and avoid producing a valid solution with invalid casts...

LucianoPAlmeida · 2020-02-11T02:19:42Z

LGTM! Looks like to make SR-12093 work we need to do a revamp of how checked casts are handled in the solver. Having it warn in CSApply for now is fine but ultimately we should be able to detect this in the solver and avoid producing a valid solution with invalid casts...

Thanks for the review @xedin :)
That makes total sense... and got me thinking that we can see a couple of diagnostics like this in CSApply, it should be possible for some of them to be moved to the new framework?

xedin · 2020-02-11T02:23:09Z

Yes, that's something which should happen, CSApply shouldn't diagnose anything.

…volving literals that can be statically coerced

LucianoPAlmeida · 2020-02-17T21:45:22Z

I had forgot about this one, it should be good to go? cc @xedin

xedin · 2020-02-19T17:52:22Z

If @CodaFi doesn't have any other suggestions I think that this is good to go!

xedin · 2020-02-19T17:52:59Z

@swift-ci please smoke test

xedin · 2020-02-19T19:12:33Z

@LucianoPAlmeida Looks like is a single test failure on Linux.

LucianoPAlmeida · 2020-02-19T19:33:31Z

@LucianoPAlmeida Looks like is a single test failure on Linux.

Humm... I'll take a look. Thanks for kick-in the CI :)

theblixguy · 2020-02-19T19:36:33Z

The test can run on Linux if we split ones that use NSString/NSNumber into a separate file (so they're only run on macOS and rest runs on both Linux and macOS)

xedin · 2020-02-19T20:07:43Z

@LucianoPAlmeida You can use // REQUIRES: OS=macosx instead of objc_interop

xedin · 2020-02-19T21:13:41Z

@swift-ci please test

swift-ci · 2020-02-19T21:16:15Z

Build failed
Swift Test Linux Platform
Git Sha - 47b7602d1046549928840a8d7f4dae5309061f1c

swift-ci · 2020-02-19T21:17:13Z

Build failed
Swift Test OS X Platform
Git Sha - 47b7602d1046549928840a8d7f4dae5309061f1c

LucianoPAlmeida · 2020-02-19T21:41:31Z

@xedin Just did the adjustments real quick :)
In which cases is ideal to use // REQUIRES: OS=macosx and which is ideal REQUIRES: objc_interop?

LucianoPAlmeida · 2020-02-19T21:43:34Z

Those new failures seems unrelated ...

xedin · 2020-02-19T21:47:07Z

@LucianoPAlmeida objc_interop i believe is going to run on all - iOS, tvOS and watchOS so if there are API differences between them tests would fail.

xedin · 2020-02-19T21:47:13Z

@swift-ci please test

swift-ci · 2020-02-19T21:48:06Z

Build failed
Swift Test Linux Platform
Git Sha - 705e468

swift-ci · 2020-02-19T21:50:34Z

Build failed
Swift Test OS X Platform
Git Sha - 705e468

LucianoPAlmeida · 2020-02-19T22:03:41Z

ninja: error: 'lib/sourcekitd.framework/sourcekitd', needed by 'stdlib/tools/swift-lang/OSX/x86_64/SwiftLang.o', missing and no known rule to make it seems to be unrelated

LucianoPAlmeida · 2020-02-19T22:04:40Z

@LucianoPAlmeida objc_interop i believe is going to run on all - iOS, tvOS and watchOS so if there are API differences between them tests would fail.

Got it 👍 Thank you :)

owenv · 2020-02-19T22:06:50Z

I think the revert/fix in #29934 should fix that SourceKit issue

xedin · 2020-02-19T22:12:53Z

Alright, we'll wait then :)

theblixguy · 2020-02-19T22:50:15Z

@swift-ci please clean test macOS

LucianoPAlmeida requested review from xedin and CodaFi January 28, 2020 01:58

CodaFi reviewed Jan 28, 2020

View reviewed changes

LucianoPAlmeida force-pushed the SR-11421-checked-cast-diag branch 2 times, most recently from 8c5faf1 to aa44334 Compare January 30, 2020 03:05

LucianoPAlmeida requested a review from CodaFi January 30, 2020 13:20

LucianoPAlmeida force-pushed the SR-11421-checked-cast-diag branch 3 times, most recently from 477a748 to 348d81e Compare February 4, 2020 04:11

LucianoPAlmeida force-pushed the SR-11421-checked-cast-diag branch 6 times, most recently from 05774cc to 47b7602 Compare February 5, 2020 17:06

xedin approved these changes Feb 10, 2020

View reviewed changes

LucianoPAlmeida added 2 commits February 13, 2020 01:04

[Diagnostics] Tailored diagnostic message for conditional downcast in…

968a37d

…volving literals that can be statically coerced

[tests] Adding literal downcast tests

fd2dbe3

LucianoPAlmeida force-pushed the SR-11421-checked-cast-diag branch from 47b7602 to fd2dbe3 Compare February 13, 2020 04:04

Require objc interop on cast/literals_downcast tests

705e468

[tests] Spliting literals downcast tests into commom and bridged

497d46f

xedin merged commit 8af4f2d into swiftlang:master Feb 20, 2020

LucianoPAlmeida deleted the SR-11421-checked-cast-diag branch March 25, 2021 23:45

LucianoPAlmeida mentioned this pull request Feb 1, 2020

[SR-12093] Checked Casts With Literals Are Deeply Unintuitive #54529

Open

[SR-11421][Diagnostics] Tailored diagnostic for checked downcast with literals #29493

[SR-11421][Diagnostics] Tailored diagnostic for checked downcast with literals #29493

Uh oh!

Conversation

LucianoPAlmeida commented Jan 28, 2020

Uh oh!

CodaFi Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

CodaFi Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

CodaFi Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida commented Feb 4, 2020

Uh oh!

LucianoPAlmeida commented Feb 5, 2020

Uh oh!

xedin commented Feb 7, 2020

Uh oh!

LucianoPAlmeida commented Feb 8, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida commented Feb 11, 2020

Uh oh!

xedin commented Feb 11, 2020

Uh oh!

LucianoPAlmeida commented Feb 17, 2020

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

LucianoPAlmeida commented Feb 19, 2020

Uh oh!

theblixguy commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

swift-ci commented Feb 19, 2020

Uh oh!

swift-ci commented Feb 19, 2020

Uh oh!

LucianoPAlmeida commented Feb 19, 2020

Uh oh!

LucianoPAlmeida commented Feb 19, 2020

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

swift-ci commented Feb 19, 2020

Uh oh!

swift-ci commented Feb 19, 2020

Uh oh!

LucianoPAlmeida commented Feb 19, 2020

Uh oh!

LucianoPAlmeida commented Feb 19, 2020

Uh oh!

owenv commented Feb 19, 2020

Uh oh!

xedin commented Feb 19, 2020

Uh oh!

theblixguy commented Feb 19, 2020 •

edited

Loading