Skip to content
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

Flatten nested optionals resulting from try? and optional sub-expressions #16942

Merged
merged 13 commits into from Nov 16, 2018

Conversation

Projects
None yet
8 participants
@bjhomer
Copy link
Contributor

commented Jun 1, 2018

This PR implements the flattening of a double-optional type produced from code like let x = try? optionalThing(). The pitch thread can be found here: https://forums.swift.org/t/make-try-optional-chain-flattening-work-together/7415/55

@bjhomer bjhomer force-pushed the bjhomer:bjhomer/optional-try-flattening branch 2 times, most recently to 2fcf429 Oct 9, 2018

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

@brentdax Rebased on master, tests are passing locally. Could you run the compatibility suite tests?

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2018

@swift-ci please test source compatibility

@rudkx
Copy link
Member

left a comment

This change would benefit from additional test cases.

For example, what happens with something like:

func foo() throws -> Int? { return 1 }
let x = (try? foo())!!

With the new semantics I would expect this to fail type checking, hopefully with a good diagnostic.

It's possible we already have some interesting test cases along these lines, but they are in files not currently compiled with -swift-version 5. Can you take a look and add some more comprehensive tests to a file that is compiled with -swift-version 5?

@@ -2848,7 +2848,29 @@ namespace {
}

Expr *visitOptionalTryExpr(OptionalTryExpr *expr) {
return simplifyExprType(expr);
if (cs.getTypeChecker().getLangOpts().isSwiftVersionAtLeast(5) == false) {

This comment has been minimized.

Copy link
@rudkx

rudkx Oct 26, 2018

Member

Nitpick: Can you change this to if (! ...) rather than if ( ... == false) as the former is used extensively throughout the codebase and is what people expect?

Not nitpick: Can you add a comment above the if explaining what the behavioral difference is from Swift 4 to Swift 5+?

@rudkx rudkx requested a review from xedin Oct 26, 2018

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

Yep, I'll get on this.

@xedin
Copy link
Member

left a comment

I agree with @rudkx, it would be great to add more tests for this, otherwise changes to Sema LGTM!

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

I have added a bunch more tests. A couple of them are currently failing; it appears I need to make some changes for cases where we used to suggest changing try? to try!.

bjhomer added some commits May 19, 2018

Make 'try?' flatten optional chaining like optional-chaining does.
If the sub-expression of the 'try?' is optional, the result will be the same level of optional-ness.
If the sub-expression is non-optional, the result is optional.

Thus, the following lines all end up with the same type of 'Int?'
 - let x = try? 3 as Int
 - let x = try? 3 as? Int
 - let x = try? 3 as Int?
Add a migration for the Swift 5 'try?' changes.
This migrates anything using the following pattern:

```swift
if let optX = try? someOptional(),
    let x = optX,
    ...
```

to this:

```swift
if let x = try? someOptional(),
    ...
```
Add a bunch more 'try' tests.
Some of these are currently failing. For example:

```swift
// expected-error {{cannot convert value of type 'Int??' to specified type 'Int'}}
let _: Int = try? producer.produceDoubleOptionalInt()
```

This actually produces this warning:

```
value of optional type 'Int?' not unwrapped; did you mean to use 'try!' or chain with '?'?
```

Note that the value in question is not an `Int?`, so a suggestion of `try!` here is inappropriate.
(And, due to the change in semantics of 'try?', wouldn't change anything anyway.)
Don't consider fixups that won't do anything.
Since 'try?' no longer unconditionally adds a layer of optional, converting it
to 'try!' will no longer unconditionally remove a layer of optional. So let's not
suggest it. This leads to better diagnostics anyway.

@bjhomer bjhomer force-pushed the bjhomer:bjhomer/optional-try-flattening branch to 1789d44 Nov 7, 2018

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@xedin, @rudkx Tests have been added. I discovered that a fix-it would suggest changing try? into try! in cases where that would no longer fix anything, so that has also been addressed. I think we're ready for further review.

@rudkx

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@swift-ci Please smoke test

@@ -1,4 +1,4 @@
// RUN: %target-typecheck-verify-swift
// RUN: %target-typecheck-verify-swift -swift-version 5

This comment has been minimized.

Copy link
@rudkx

rudkx Nov 7, 2018

Member

Can you split this into two tests, one for ambient-version mode (with no changes to the existing test) and one for -swift-version 5?

We still want to test the older behavior as long as it is supported.

This comment has been minimized.

Copy link
@bjhomer

bjhomer Nov 8, 2018

Author Contributor

Yeah, I can do that.

@rudkx

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@nkcsgexi I cannot seem to currently add you as a reviewer. Can you take a look at the migrator changes?

@rudkx rudkx requested a review from jckarter Nov 7, 2018

@nkcsgexi nkcsgexi self-requested a review Nov 7, 2018

@nkcsgexi

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

hmm, interesting. I've added myself as a reviewer and will take a look.

@rudkx
Copy link
Member

left a comment

I looked at the AST & Sema changes as well as the tests. LGTM. Once you split out the new tests into a separate file for -swift-version 5 I think that portion is good to go.

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

Regarding the migration, I have a test file that I've been using locally. I would be happy to add it as an automated test, if I could figure out how. Is there some example of how to run a test like that in the repository right now?

@rudkx

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

@nkcsgexi Can you answer the question re: migration testing?

@rudkx

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

@swift-ci Please smoke test

@nkcsgexi

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

@bjhomer you can follow some examples in test/migrator. Taking remove_override.swift as an example, it's both test driver and the code example before migration. After running the migrator, the result should be identical to remove_override.swift.expected.

@rudkx

rudkx approved these changes Nov 9, 2018

@bjhomer bjhomer force-pushed the bjhomer:bjhomer/optional-try-flattening branch to c371c06 Nov 12, 2018

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

@nkcsgexi: Thanks for that example. I'm bulking up these tests, and it's pointing out a few things I need to improve.

Make the try? migration simpler and more reliable.
We were trying to get fancy rewriting cases where multiple 'if let' clauses were
used to unwrap a double optional, but in testing that turned out to be too
unreliable. Now we simply detect any 'try?' expressions whose behavior will change,
and add an explicit 'as OldType' to the expression, in order to preserve the original
behavior.

@bjhomer bjhomer force-pushed the bjhomer:bjhomer/optional-try-flattening branch to f166b0d Nov 14, 2018

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

The migration tests are ready to go. I've simplified the approach a bit after discovering some problems with the earlier one.

Rename try_swift4.swift back to try.swift.
It makes it clearer what has changed.
@tkremenek

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

This would be good to land before Swift 5 branches one last time from master on November 16.

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

Agreed. I think everything is ready to go, but if there's any further feedback, I'll be responsive.

@natecook1000

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

@swift-ci Please smoke test

@natecook1000

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

@nkcsgexi @rudkx Okay to merge?

@nkcsgexi

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

The migrator part LGTM to merge!

@rudkx

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

Yes, merging.

@rudkx rudkx merged commit 018498f into apple:master Nov 16, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

@bjhomer It looks like this broke the foundation build: https://ci.swift.org/job/swift-PR-Linux-smoke-test/11232/consoleFull#-1189064102373bf607-d277-47c6-812e-27acc852624c

/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/swift-corelibs-foundation/TestFoundation/TestURL.swift:38:38: error: cannot force unwrap value of non-optional type '[String : Any]'
13:46:22     guard let parsingTests = testRoot![kURLTestParsingTestsKey] as? [Any] else {
@rudkx

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

Okay, I don't see how this passed PR testing yesterday but would now fail today. I might be missing something that changed in foundation. Reverting for now, though.

@bjhomer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.