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

Merged
merged 2 commits into from Oct 6, 2016

Conversation

Projects
None yet
10 participants
@CodaFi
Collaborator

CodaFi commented Oct 4, 2016

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.

Initial implementation of optionals-in-string-interpolation warnings
Basic extension of the optional-to-any AST walker to incorporate
warnings for the as-of-now up and coming Swift evolution proposal.
@harlanhaskins

This comment has been minimized.

Show comment
Hide comment
@harlanhaskins

harlanhaskins Oct 3, 2016

I think we should pull out the original diagnostic into a function. No need to copy and paste.

I think we should pull out the original diagnostic into a function. No need to copy and paste.

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 3, 2016

Owner

Mhm, sounds good.

Owner

CodaFi replied Oct 3, 2016

Mhm, sounds good.

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 3, 2016

Owner

Maybe call '.debugDescription' to silence this warning instead.

Maybe call '.debugDescription' to silence this warning instead.

This comment has been minimized.

Show comment
Hide comment
@harlanhaskins

harlanhaskins Oct 3, 2016

Yeah, it's simpler and conveys just as much information

harlanhaskins replied Oct 3, 2016

Yeah, it's simpler and conveys just as much information

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Oct 3, 2016

It doesn't look like a call, so your current wording is probably preferable.

rudkx replied Oct 3, 2016

It doesn't look like a call, so your current wording is probably preferable.

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 3, 2016

Owner

This is vague and needs better wording.

This is vague and needs better wording.

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Oct 3, 2016

Yeah this is too vague. Perhaps:
"optional-typed value in string interpolation results in printing "Optional()" around the value, or "nil" for empty optionals"
?

rudkx replied Oct 3, 2016

Yeah this is too vague. Perhaps:
"optional-typed value in string interpolation results in printing "Optional()" around the value, or "nil" for empty optionals"
?

@harlanhaskins

This comment has been minimized.

Show comment
Hide comment
@harlanhaskins

harlanhaskins Oct 3, 2016

Maybe we should just suggest as Any for this?

Maybe we should just suggest as Any for this?

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Oct 3, 2016

We could allow as Any to silence this, or could also allow as T? where T? is the optional type of the given expression.

rudkx replied Oct 3, 2016

We could allow as Any to silence this, or could also allow as T? where T? is the optional type of the given expression.

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Oct 7, 2016

as Optional also works.

as Optional also works.

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 7, 2016

Owner

That's the diagnostic we settled on.

Owner

CodaFi replied Oct 7, 2016

That's the diagnostic we settled on.

@rudkx

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Oct 3, 2016

I believe the cast will be within a ParenExpr, but perhaps the segments don't contain that piece?

I believe the cast will be within a ParenExpr, but perhaps the segments don't contain that piece?

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Oct 3, 2016

Let's rename diagnoseOptionalToAnyCoercion to something that makes more sense with this included. Perhaps diagnoseOptionalInDangerousContexts? Or...I'm not good at naming...

rudkx replied Oct 3, 2016

Let's rename diagnoseOptionalToAnyCoercion to something that makes more sense with this included. Perhaps diagnoseOptionalInDangerousContexts? Or...I'm not good at naming...

@rudkx

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Oct 3, 2016

I'm not sure a default value is as sensible in this case as in the case of 'Any', but then I don't have a particularly strong argument for that.

I'm not sure a default value is as sensible in this case as in the case of 'Any', but then I don't have a particularly strong argument for that.

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 4, 2016

Collaborator

@swift-ci please smoke test.

Collaborator

CodaFi commented Oct 4, 2016

@swift-ci please smoke test.

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

This comment has been minimized.

@rudkx

rudkx Oct 4, 2016

Member

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

rudkx Oct 4, 2016

Member

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

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",
+ ())

This comment has been minimized.

@rudkx

rudkx Oct 4, 2016

Member

"unintended behavior" still seems to vague to me.

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

@rudkx

rudkx Oct 4, 2016

Member

"unintended behavior" still seems to vague to me.

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

This comment has been minimized.

@CodaFi

CodaFi Oct 4, 2016

Collaborator

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?

@CodaFi

CodaFi Oct 4, 2016

Collaborator

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?

lib/Sema/MiscDiagnostics.cpp
+ if (auto segmentTy = segment->getType()) {

This comment has been minimized.

@rudkx

rudkx Oct 4, 2016

Member

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

@rudkx

rudkx Oct 4, 2016

Member

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

lib/Sema/MiscDiagnostics.cpp
+ if (auto segmentTy = segment->getType()) {
+ if (auto baseTy = segmentTy->getOptionalObjectType()) {

This comment has been minimized.

@rudkx

rudkx Oct 4, 2016

Member

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?

@rudkx

rudkx Oct 4, 2016

Member

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?

This comment has been minimized.

@rudkx

rudkx Oct 4, 2016

Member

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?

@rudkx

rudkx Oct 4, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 4, 2016

Collaborator

@rudkx That should do it.

Collaborator

CodaFi commented Oct 4, 2016

@rudkx That should do it.

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 4, 2016

Collaborator

@swift-ci please smoke test.

Collaborator

CodaFi commented Oct 4, 2016

@swift-ci please smoke test.

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 4, 2016

Collaborator

Sigh, macOS and Linux.

Collaborator

CodaFi commented Oct 4, 2016

Sigh, macOS and Linux.

@shahmishal

This comment has been minimized.

Show comment
Hide comment
@shahmishal

shahmishal Oct 4, 2016

Member

@swift-ci Please smoke test Linux

Member

shahmishal commented Oct 4, 2016

@swift-ci Please smoke test Linux

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 4, 2016

Collaborator

Thanks @shahmishal

Collaborator

CodaFi commented Oct 4, 2016

Thanks @shahmishal

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

This comment has been minimized.

@rudkx

rudkx Oct 4, 2016

Member

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: rudkx@6a43b76

@rudkx

rudkx Oct 4, 2016

Member

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: rudkx@6a43b76

This comment has been minimized.

@CodaFi

CodaFi Oct 4, 2016

Collaborator

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

@CodaFi

CodaFi Oct 4, 2016

Collaborator

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

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 4, 2016

Collaborator

@swift-ci please smoke test.

Collaborator

CodaFi commented Oct 4, 2016

@swift-ci please smoke test.

@rudkx

rudkx approved these changes Oct 4, 2016

LGTM!

@rudkx

This comment has been minimized.

Show comment
Hide comment
@rudkx

rudkx Oct 4, 2016

Member

@swift-ci Please smoke test

Member

rudkx commented Oct 4, 2016

@swift-ci Please smoke test

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 4, 2016

Collaborator

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

Collaborator

CodaFi commented Oct 4, 2016

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

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 6, 2016

Collaborator

⛵️

Collaborator

CodaFi commented Oct 6, 2016

⛵️

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

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@CodaFi CodaFi deleted the CodaFi:failure-is-not-an-optional branch Oct 6, 2016

@jrose-apple

This comment has been minimized.

Show comment
Hide comment
@jrose-apple

jrose-apple Oct 6, 2016

Member

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

Member

jrose-apple commented Oct 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jrose-apple

jrose-apple Oct 6, 2016

Member

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

Member

jrose-apple commented Oct 6, 2016

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

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 7, 2016

Collaborator

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

Collaborator

CodaFi commented Oct 7, 2016

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

@jckarter

This comment has been minimized.

Show comment
Hide comment
@jckarter

jckarter Oct 7, 2016

Member

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.

Member

jckarter commented Oct 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jrose-apple

jrose-apple Oct 7, 2016

Member

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

Member

jrose-apple commented Oct 7, 2016

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

@CodaFi

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 7, 2016

Collaborator

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

Collaborator

CodaFi commented Oct 7, 2016

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

@jtbandes

This comment has been minimized.

Show comment
Hide comment
@jtbandes

jtbandes Oct 8, 2016

Collaborator

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

jtbandes commented Oct 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Oct 9, 2016

Collaborator

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.
Collaborator

CodaFi commented Oct 9, 2016

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.

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

@clearbrian

This comment has been minimized.

Show comment
Hide comment
@clearbrian

clearbrian Mar 28, 2017

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

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

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Mar 28, 2017

Collaborator

@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

Collaborator

CodaFi commented Mar 28, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@superarts

superarts Apr 3, 2017

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

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

@paulrolfe

This comment has been minimized.

Show comment
Hide comment
@paulrolfe

paulrolfe Apr 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Apr 5, 2017

Collaborator

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

Collaborator

CodaFi commented Apr 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@CodaFi

CodaFi Apr 5, 2017

Collaborator

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.

Collaborator

CodaFi commented Apr 5, 2017

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.

@apple apple locked and limited conversation to collaborators Apr 5, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.