Skip to content

More correct implementation of SE-0110 #6133

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 14, 2016
Merged

More correct implementation of SE-0110 #6133

merged 3 commits into from
Dec 14, 2016

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented Dec 8, 2016 •
edited
Loading

Related radars:

  • rdar://27383557
  • rdar://27857015
  • rdar://28621719
  • rdar://29409725

jrose-apple
jrose-apple reviewed Dec 8, 2016
View reviewed changes
test/ClangImporter/foreign_errors.swift
@@ -14,7 +14,7 @@ func test0() {
func testAndReturnError() throws {
try ErrorProne.fail()
try ErrorProne.go()
try ErrorProne.tryAndReturnError() // collides with 'try' keyword
try ErrorProne.tryAndReturnError(()) // collides with 'try' keyword
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, this is actually a change. We shouldn't be adding a dummy argument here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean it's a clang importer bug? I thought so...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We add a dummy argument when there's a naming conflict with another method, but we shouldn't when there's a conflict with a keyword.

@jrose-apple
Copy link
Contributor

It's worth checking what things we publicly promised would still work after SE-0110 and SE-0111.

@slavapestov
Copy link
Contributor Author

@jrose-apple Yeah I'll definitely take a look at those. This is still a work in progress, it needs the staging flag before going in and in fact some of our libraries don't even build yet due to some other Sema bug I haven't figured out. So the language changes definitely are not final, I just wanted to see an approximation thereof.

@slavapestov
Copy link
Contributor Author

Here is a new version where the corrected logic is conditionalized on the Swift 3 flag.

Note that unfortunately even without the fix, the behavior in master already diverged from Swift 3 in incompatible ways. I'm going to try to reconcile this.

@slavapestov
Copy link
Contributor Author

FWIW the plan is for Swift 3 mode to support a superset of what the real Swift 3 supported. Also obviously I'm not going to try to simulate the old nonsense diagnostics and crashes...

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

(Expecting a couple of failures in Compatibility/tuple_arguments)

@slavapestov
Copy link
Contributor Author

slavapestov commented Dec 13, 2016 •
edited
Loading

Here are the source breaks I'm aware of so far with master vs Swift 3.

These typecheck in Swift 3 but not master (with Swift 3 mode both on or off):

let fn: (Int, Int) -> () = { t in _ = (t.0, t.1) }

let pong: (_ data : Any) -> () = {(data) in}
pong(data: "hello world")

These diagnose in master but not in Swift 3:

/Users/slava/new/swift/test/Compatibility/tuple_arguments.swift:803:29: error: unexpected error produced: extra argument in call
  sTwo.genericFunction(3.0, 4.0) // FIXME
                            ^
/Users/slava/new/swift/test/Compatibility/tuple_arguments.swift:855:27: error: unexpected error produced: extra argument in call
  sTwo.genericFunction(a, b) // FIXME
                          ^
/Users/slava/new/swift/test/Compatibility/tuple_arguments.swift:1063:38: error: unexpected error produced: extra argument in call
  _ = GenericEnum<(Int, Int)>.one(3, 4) // FIXME
                                     ^

@slavapestov slavapestov changed the title Argument tuple crap Fixes for argument tuple quirks, and making -swift-version 3 more faithful Dec 13, 2016
@slavapestov
Revert "[Sema] Implement SE-0110"
321edbf 
This reverts commit e172383.

There were two problems with this commit:
- This was a source-breaking change and should have been feature-gated.
- It only addressed one narrow case of SE-0110.

Fixes <rdar://problem/28621719>.
@slavapestov
Sema: New implementation of SE-0110
3ede1ab 
Fix matchTypes() to be more careful about stripping off ParenType
sugar, in order to match the behavior outlined in SE-0110.

Note that the new logic only executes in Swift 4 mode; there's
no functional change here in Swift 3 mode.

This makes a second copy of the tuple_arguments test:

- Compatibility/tuple_arguments is a test for Swift 3, updated to
  note differences with Swift 3.

- Constraints/tuple_arguments has been updated for the new Swift 4
  mode behavior.

Fixes <rdar://problem/27383557>.
@slavapestov
Sema: Fix a couple of crashes in tuple arguments tests
dbb9d31 
There's a general problem where a SubscriptExpr has an argument
that's a LoadExpr loading a tuple from an lvalue. For some reason
we don't construct the ParenExpr in this case, which confused
CSDiag.

Also, in Swift 3 mode, add a total hack to fudge things in
matchCallArguments() in the case where we erroneously lost
ParenType sugar.
@slavapestov slavapestov changed the title Fixes for argument tuple quirks, and making -swift-version 3 more faithful More correct implementation of SE-0110 Dec 14, 2016
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

2 similar comments
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov slavapestov mentioned this pull request Dec 14, 2016
Closed
@slavapestov slavapestov merged commit a1b113f into swiftlang:master Dec 14, 2016
benrimmington added a commit to swiftlang/swift-evolution that referenced this pull request Dec 30, 2016
@benrimmington
SE-0110 was implemented in swiftlang/swift#6133
fae9c24 
Uses the `-swift-version 4` command-line option.
benrimmington added a commit to swiftlang/swift-evolution that referenced this pull request Dec 30, 2016
@benrimmington
SE-0110 was implemented in swiftlang/swift#6133
9e95709 
Uses the `-swift-version 4` command-line option.
@slavapestov slavapestov mentioned this pull request Dec 30, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrose-apple jrose-apple jrose-apple left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@slavapestov @jrose-apple