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

swift4: Fixes for closure arguments #995

Closed
wants to merge 4 commits into from

Conversation

spevans
Copy link
Collaborator

@spevans spevans commented May 21, 2017

  • Fix destructuring

  • Use correct number of arguments in closures

Compatible with swift 3 & 4

@pushkarnk
Copy link
Collaborator

@swift-ci test

@parkera
Copy link
Member

parkera commented May 22, 2017

I'm not sure I understand what was wrong with the previous syntax. Why did this become a problem?

@spevans
Copy link
Collaborator Author

spevans commented May 22, 2017

There were 2 sets of errors:

  1. The closure argument count needs to match the signature, eg Contextual closure type '(UnsafeMutableRawPointer, Int) -> Void' expects 2 arguments, but 1 was used in closure body and Contextual type for closure argument list expects 1 argument, which cannot be implicitly ignored

  2. Some cases for { (key, value) needed to be replaced with { (arg) ; let (key, value) = arg to solve the following:
    Closure tuple parameter '(Notification, [RunLoopMode])' does not support destructuring

Most of the fixes were from fix-its in Xcode

@damuellen
Copy link
Contributor

Just yesterday, the error message "..does not support destructuring" occurred to me in some places in one of my projects. The Xcode fix-it has helped, but I would also like to know why Swift 4 is different in this case than Swift 3.

@spevans
Copy link
Collaborator Author

spevans commented May 23, 2017

Its due to SE-110 https://github.com/apple/swift-evolution/blob/master/proposals/0110-distingish-single-tuple-arg.md
I think it made an incorrect argument count an error in Swift4

@parkera
Copy link
Member

parkera commented May 23, 2017

Is there an alternative to effectively re-declaring the argument all over the place? This can't be the pattern we really want people to use.

@parkera
Copy link
Member

parkera commented May 24, 2017

I sent a message to the swift evolution mailing list about this. I think we should have a further discussion before we accept this; it seems like a dramatic reduction in usability of the language in general.

@spevans
Copy link
Collaborator Author

spevans commented May 24, 2017

Yeah thats a good idea. It does seem quite a source breaking change given how much closures are used in general

Copy link
Collaborator

@xwu xwu left a comment

Choose a reason for hiding this comment

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

An unfortunate result of SE-0110 indeed. If it needs to be committed, just a few stylistic points.

@@ -18,7 +18,8 @@ extension Dictionary : _ObjectTypeBridgeable {

var idx = 0

self.forEach { (keyItem, valueItem) in
self.forEach { (arg) in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Stylistic point: this and all subsequent parameter lists in closures don't need the parentheses around arg.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change actually came from the migrator, it may be worth fixing that to not add parentheses as I was taking the fix-its as best practice. Although I'll see what the outcome of the discussion on swift-evolution is before making any changes to this PR

@@ -92,8 +92,8 @@ open class NSIndexSet : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {

open func mutableCopy(with zone: NSZone? = nil) -> Any {
let set = NSMutableIndexSet()
enumerateRanges(options: []) {
set.add(in: $0.0)
enumerateRanges(options: []) { (range, stop) in
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be range, _.

@@ -617,8 +617,8 @@ open class XMLNode: NSObject, NSCopying {
if let parentNode = _CFXMLNodeGetParent(parent!) {
let grandparent = XMLNode._objectNodeForNode(parentNode)
let possibleParentNodes = grandparent.filter { $0.name == self.parent?.name }
let count = possibleParentNodes.reduce(0) {
return $0.0 + 1
let count = possibleParentNodes.reduce(0) { (x, y) in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too, this could be x, _.

@igor-makarov
Copy link

It seems like most of those are derived from problems in they way functions in the Swift Standard Library are defined, for example public func forEach(_ body: ((key: Key, value: Value)) throws -> Void) rethrows expects a function that receives a tuple, whereas if it was written as func forEach(_ body: (_ key: Key, _ value: Value) throws -> Void), it would expect a function that receives two params.

@spevans spevans mentioned this pull request Jun 3, 2017
- Fix destructuring

- Use correct number of arguments in closures
- Fixes "Nested tuple parameter '([Character], JSONReader.Index)'
  (aka '(Array<Character>, Int)') of function
 '((([Character], JSONReader.Index)) throws -> _?) throws -> _?'
  (aka '((Array<Character>, Int) throws -> Optional<_>) throws -> Optional<_>')
  does not support destructuring"
@spevans
Copy link
Collaborator Author

spevans commented Jun 14, 2017

Rebased. I added an extra commit with some tidy ups of the closure arguments although of course these are subject to personal taste more than anything.

@parkera Is this ok for merging now? I noticed most of the SE-110 discussion seems to have ended and any possible changes to the parser will be post swift4 now.

@parkera
Copy link
Member

parkera commented Jun 15, 2017

I don't think that's actually the case, or at least I strongly hope it's not, so I still want to hold off on this.

@spevans
Copy link
Collaborator Author

spevans commented Jun 20, 2017

Closing for now, this is not needed as the SE-110 change is going to be backed out. See https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170619/037616.html

If any individual closures still generate warnings after the change a new PR can be opened.

@spevans spevans closed this Jun 20, 2017
@spevans spevans deleted the pr_swift4_closure_fixes branch August 9, 2017 06:43
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.

None yet

6 participants