Extend Optional-As-Any Warning to String Interpolation Segments #5110

CodaFi · 2016-10-04T00:14:51Z

An implementation of warnings for optional values in string interpolation segments. We now suggest that the user either

Insert .debugDescription
Insert a cast to T?

To explicitly request a debug string representation of an optional. Further discussion of this patch can be found on the list.

Resolves SR-1882.

Basic extension of the optional-to-any AST walker to incorporate warnings for the as-of-now up and coming Swift evolution proposal.

CodaFi · 2016-10-04T00:16:06Z

@swift-ci please smoke test.

rudkx · 2016-10-04T00:54:04Z

test/Sema/diag_optional_to_any.swift

+  print("Always some, Always some, Always some: \(o as Int?)") // No warning
+  print("Always some, Always some, Always some: \(o.debugDescription)") // No warning.
+}
+


Can you rename the test case to match the new functionality? e.g.:
diag_unintended_optional_behavior.swift
or
diag_unintended_optional_uses.swift

rudkx · 2016-10-04T02:26:27Z

include/swift/AST/DiagnosticsSema.def

@@ -2411,6 +2411,14 @@ NOTE(force_optional_to_any,none,
     "force-unwrap the value to avoid this warning", ())
 NOTE(silence_optional_to_any,none,
     "explicitly cast to Any with 'as Any' to silence this warning", ())
+WARNING(optional_in_string_interpolation_segment,none,
+        "optional value in string interpolation can have unintended behavior",
+        ())


"unintended behavior" still seems to vague to me.

Can you try to find something more specific to use here?

Perhaps a middle ground between the two diagnostics:

string interpolation produces a debug description for an optional value; did you mean to make this explicit?

rudkx · 2016-10-04T02:33:34Z

lib/Sema/MiscDiagnostics.cpp


+          if (auto segmentTy = segment->getType()) {


I'm not sure the 'if' here is absolutely necessary.

rudkx · 2016-10-04T02:34:33Z

lib/Sema/MiscDiagnostics.cpp


+          if (auto segmentTy = segment->getType()) {
+            if (auto baseTy = segmentTy->getOptionalObjectType()) {


In the spirit of http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code, can you assign baseTy before the if and then invert the test and make it continue, unindenting the following code?

Actually baseTy is only used for the message here, so perhaps you could just skip having that and instead use segmentTy and not append the '?' below?

CodaFi · 2016-10-04T03:36:01Z

@rudkx That should do it.

CodaFi · 2016-10-04T03:37:08Z

@swift-ci please smoke test.

CodaFi · 2016-10-04T03:39:49Z

Sigh, macOS and Linux.

shahmishal · 2016-10-04T04:03:48Z

@swift-ci Please smoke test Linux

CodaFi · 2016-10-04T04:10:01Z

Thanks @shahmishal

rudkx · 2016-10-04T06:50:13Z

lib/Sema/MiscDiagnostics.cpp

+          auto segmentTy = segment->getType();
+          if (auto parenType = dyn_cast<ParenType>(segmentTy.getPointer())) {
+            segmentTy = parenType->getUnderlyingType();
+          }



This isn't necessary. The call to getOptionalObjectType() strips the parens, and if you rework the diagnostics to print Types rather than StringRef I believe it strips parens as well.

Here is a quick tweak I just tried, to see what I mean: https://github.com/rudkx/swift/commit/6a43b76aeb5548d9f80bba98fdcbe4d5d58c79e7

Ah, that's what I was looking for. Thanks!

CodaFi · 2016-10-04T14:19:25Z

@swift-ci please smoke test.

rudkx

LGTM!

rudkx · 2016-10-04T15:06:52Z

@swift-ci Please smoke test

CodaFi · 2016-10-04T23:20:52Z

Gonna give the Swift evolution discussion a chance to settle down a bit before I commit this.

CodaFi · 2016-10-06T19:27:07Z

⛵️

jrose-apple · 2016-10-06T23:38:32Z

Since both fix-its result in the same behavior, I'm inclined to say we should just pick one and drop the other note.

jrose-apple · 2016-10-06T23:39:08Z

Arguably we should still offer the ?? <#default#> fix-it as well.

CodaFi · 2016-10-07T00:43:41Z

Then i'd prefer to keep the debugDescription call over everything else.

jckarter · 2016-10-07T14:41:19Z

debugDescription is intended as a customization point, not something you ever call directly. I don't think we should encourage people to use it explicitly.

jrose-apple · 2016-10-07T16:05:13Z

That's a good point. String(reflecting:) is supposed to be the normal entry point.

CodaFi · 2016-10-07T22:29:26Z

Hm, that's a good idea. I'll amend the diagnostic and shoot ya'll another pull request.

jtbandes · 2016-10-08T23:39:11Z

Thanks, this looks great! 😄

Minor recommendations:

did you mean to make this explicit? isn't very clear by itself. I would recommend simply removing the semicolon and this clause.
what would you think of suggesting as Optional instead of as [the argument type]?

CodaFi · 2016-10-09T03:29:12Z

Thanks!

I disagree. The diagnostic needs to be as specific as possible about what the actual behavior of this expression is and why we intend to fix it.
Definitely cleaner than the existing cast. I'll include it in the patch.

Hiding tremendous warnings. Sad. swiftlang/swift#5110

clearbrian · 2017-03-28T12:37:57Z

This just fills my log code full of (String(describing:someOptional) and Xcode Fix-it is broken doesnt appear all the time especially if theres two " ...(optional1) (optional2).
I though the point of swift was LESS code not I got (String(describing:.. everywhere

CodaFi · 2017-03-28T15:13:05Z

@clearbrian Please file an SR about specific bugs in Swift related to Optional-as-Any diagnostics, and radars about Xcode. It may just be that you're trying to apply stale fixits, it may be something deeper, but I can't tell from this comment alone.

In addition, the diagnostic should offer more ways to fix or silence this warning than the "long way" of going through the initializer. If you rely on logging a lot of optionals, and that optionality is relevant to the log, then consider an as cast to Optional

superarts · 2017-04-03T15:41:46Z

This is an example of fixing a problem by introducing another.

paulrolfe · 2017-04-05T18:58:06Z

@CodaFi Is there any way to silence these warnings without adding so many useless lines of code to my project? Like a compiler flag or something?

CodaFi · 2017-04-05T19:45:31Z

The fixit offers you multiple single-line avenues for doing just that. A cast with as Optional will also silence it. This warning is an extension of Optional-as-Any analysis. Any workarounds for that apply here as well

CodaFi · 2017-04-05T19:48:36Z

This issue is not the most appropriate avenue of discussion about this feature. Refer to the Swift evolution thread linked at the root for details about its inclusion and the problem it solves.

I'm locking this discussion.

Initial implementation of optionals-in-string-interpolation warnings

43a582e

Basic extension of the optional-to-any AST walker to incorporate warnings for the as-of-now up and coming Swift evolution proposal.

CodaFi force-pushed the failure-is-not-an-optional branch from 3c86749 to 31a2141 Compare October 4, 2016 02:03

rudkx requested changes Oct 4, 2016

View reviewed changes

CodaFi force-pushed the failure-is-not-an-optional branch 3 times, most recently from f81bcae to d7a3ffa Compare October 4, 2016 03:35

CodaFi force-pushed the failure-is-not-an-optional branch from d7a3ffa to cb8b8b1 Compare October 4, 2016 03:36

rudkx reviewed Oct 4, 2016

View reviewed changes

General bike shedding

0243497

CodaFi force-pushed the failure-is-not-an-optional branch from cb8b8b1 to 0243497 Compare October 4, 2016 14:10

rudkx approved these changes Oct 4, 2016

View reviewed changes

CodaFi merged commit 97a99ae into swiftlang:master Oct 6, 2016

CodaFi deleted the failure-is-not-an-optional branch October 6, 2016 19:27

CodaFi mentioned this pull request Oct 6, 2016

[Gardening] Normalize the way paren types are stripped in Sema. #5163

Merged

CodaFi mentioned this pull request Oct 9, 2016

Improve diagnostics for optionals in string interpolation segments #5195

Merged

lichtblau added a commit to lichtblau/Noze.io that referenced this pull request Mar 22, 2017

Swift 3.1 warnings: Suppress optional-as-any warnings

c0283ee

Hiding tremendous warnings. Sad. swiftlang/swift#5110

swiftlang locked and limited conversation to collaborators Apr 5, 2017


		if (auto segmentTy = segment->getType()) {
		if (auto baseTy = segmentTy->getOptionalObjectType()) {

Extend Optional-As-Any Warning to String Interpolation Segments #5110

Extend Optional-As-Any Warning to String Interpolation Segments #5110

Uh oh!

Conversation

CodaFi commented Oct 4, 2016

Uh oh!

CodaFi commented Oct 4, 2016

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!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Oct 4, 2016

Uh oh!

CodaFi commented Oct 4, 2016

Uh oh!

CodaFi commented Oct 4, 2016

Uh oh!

shahmishal commented Oct 4, 2016

Uh oh!

CodaFi commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Oct 4, 2016

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

rudkx commented Oct 4, 2016

Uh oh!

CodaFi commented Oct 4, 2016

Uh oh!

CodaFi commented Oct 6, 2016

Uh oh!

jrose-apple commented Oct 6, 2016

Uh oh!

jrose-apple commented Oct 6, 2016

Uh oh!

CodaFi commented Oct 7, 2016

Uh oh!

jckarter commented Oct 7, 2016

Uh oh!

jrose-apple commented Oct 7, 2016

Uh oh!

CodaFi commented Oct 7, 2016

Uh oh!

jtbandes commented Oct 8, 2016

Uh oh!

CodaFi commented Oct 9, 2016

Uh oh!

clearbrian commented Mar 28, 2017

Uh oh!

CodaFi commented Mar 28, 2017

Uh oh!

superarts commented Apr 3, 2017

Uh oh!

paulrolfe commented Apr 5, 2017

Uh oh!

CodaFi commented Apr 5, 2017

Uh oh!

CodaFi commented Apr 5, 2017

Uh oh!

Uh oh!