Skip to content

_isOptional(type(of:)) Does Not Do What You Think It Does #28994

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 1 commit into from
Jan 6, 2020

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented Jan 3, 2020

In particular, if value is Any in a generic context, then type(of: value) is Any.Protocol which is never considered optional. As a result, the first clause here (that provided special handling for printing optionals) was never actually being used for print() or other similar paths. (Curiously, it was used for string interpolation.)

Fortunately? print() was producing the right results for optionals because of a dynamic cast bug that failed to unwrap optionals in these same contexts.

Resolves rdar://58302667

In particular, if value is `Any` in a generic context, then `type(of: value)` is
`Any.Protocol` which is never considered optional.  As a result, the first
clause here was never actually being used for `print()` or other similar paths.
(Curiously, it _was_ used for string interpolation.)

Fortunately? `print()` was producing the right results for
optionals because of a dynamic cast bug that failed to
unwrap optionals in these same contexts. <sigh>
@tbkka tbkka requested a review from beccadax January 3, 2020 19:22
@tbkka tbkka changed the title _isOptional(type(of: value)) Does Not Do What You Think It Does _isOptional(type(of:)) Does Not Do What You Think It Does Jan 3, 2020
@tbkka tbkka requested a review from jckarter January 3, 2020 19:23
@tbkka
Copy link
Contributor Author

tbkka commented Jan 3, 2020

@swift-ci Please test Linux

@tbkka
Copy link
Contributor Author

tbkka commented Jan 3, 2020

@swift-ci Please smoke test

@jckarter
Copy link
Contributor

jckarter commented Jan 6, 2020

Looks good for preserving the existing behavior. (As we discussed previously, this still isn't sufficient if the value is a multiply-wrapped Any, but that bug already existed.)

@tbkka
Copy link
Contributor Author

tbkka commented Jan 6, 2020

The best technical answer is probably to eliminate the special case for Optional entirely. But that will require some additional discussion and socialization.

@tbkka tbkka merged commit 93d07c6 into swiftlang:master Jan 6, 2020
@tbkka tbkka deleted the tbkka-fix-isOptional-in-print-path branch January 6, 2020 18:29
@jckarter
Copy link
Contributor

jckarter commented Jan 6, 2020

Yeah, ultimately it's only there in order to give Optional a fighting chance, because the dynamic casts that look for conformances below will look through Optionals. If we had separate "recursively unwrap and open an existential" and "test if this type conforms to a protocol" operations that didn't magically unwrap, we could do away with the special case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants