-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[SR-6796] Swift 4.1 regression type checking closures with Void argument #49345
Comments
I labeled this "4.0 regression" because there is no "4.1 regression" label yet. |
Meta question: how come this wasn't found by the source compatibility suite? |
@swift-ci create |
That code isn't actually correct. |
(Source compatibility might mean continuing to support it, though. :-( ) |
What's the correct syntax for this then? I couldn't find any way of being able to pass the result of log() to f. |
{ _ in log() } would work. |
Is this with |
4. |
@jckarter doesn't seem to work either:
|
re: the meta question about the compat suite - this works with I'll pull this up in a debugger and try to figure out what's going on. |
@DougGregor made a change to fix function type canonicalization; the canonical type of (()) -> () is now distinct from () -> (). Could this be related? |
That's one of my theories, yes. |
While I'm looking into this I can offer a work-around that appears to work with every version of Xcode going back to at least Xcode 7: func f(a: ((()) -> Void)? = nil) {} // add () inside the argument list
func log<T>() -> ((T) -> Void)? { return nil }
f(a: log()) // remove the coercion |
Comment by Reid Ellis (JIRA) @rudkx I'm not clear on where |
It looks like it is standing in for each of the function parameters of public func on(
starting: (() -> Void)? = nil,
started: (() -> Void)? = nil,
event: ((ProducedSignal.Event) -> Void)? = nil,
failed: ((Error) -> Void)? = nil,
completed: (() -> Void)? = nil,
interrupted: (() -> Void)? = nil,
terminated: (() -> Void)? = nil,
disposed: (() -> Void)? = nil,
value: ((Value) -> Void)? = nil
) -> SignalProducer<Value, Error> { |
Comment by Reid Ellis (JIRA) So it seems we should convert this to something like the below? public func on(
starting: ((()) -> Void)? = nil,
started: (((()) -> Void)? = nil,
// ... I did try that but was unsuccessful. I raised a ReactiveSwift issue with a link to this bug report and your workaround. Update: the issue referenced above has been folded into the general Swift 4.1 pull reqest. |
This is still unsolved in Xcode 9.3 beta 2. Any idea if this will be fixed soon? |
I'm looking at this today and will update when I know more. |
rae (JIRA User) The second line you have in your example of trying the work-around seems to have an extra paren. What issues did you hit when you tried the work-around? |
I should be more clear here. What I called a work-around above is really the way the code will need to be written in the Swift 5+. We have no way to infer a generic parameter to no type, but various implementation defects have allowed the cast above to work around that fact. This was broken by a change to improve our function type representation to be more correct and more consistent, and that change cannot be backed out, at least not without substantial risk to stability. |
> and that change cannot be backed out, at least not without substantial risk to stability. So what do we do? As it stands, AFAIK there is no way in Swift 4.1 to represent what that code needs to do, which is a regression. |
> So what do we do? It's not clear to me if anyone tried making these functions take |
Comment by Reid Ellis (JIRA) I think he means e.g. changing
to:
which I think I did try without success. Not sure what now, as just switching to the swift-4.1 branch of ReactiveSwift seemed to fix things for me. |
What I meant was changing the definition of In looking more closely, that might not be a good long term solution because I believe it's just been an accident that we allow the two lines at the bottom of this example to compile: func foo(_ fn: ((())->Void)?) {}
func bar()->() {}
foo{ print("hi") } // we allow this today, despite this closure not having an explicit parameter
foo(bar) // we allow this today despite bar not taking a parameter I'm still looking at this to see if there is something we can do in the compiler. |
> which I think I did try without success. Not sure what now, as just switching to the swift-4.1 branch of ReactiveSwift seemed to fix things for me. That branch contains a workaround that just disables the operator (see ReactiveCocoa/ReactiveSwift@f41b89c3d2565e3938a88185a55d79b2218b838e) |
I just opened a PR that attempts to restore this behavior, but only under -swift-version 4 (for -swift-version 3 it works for other reasons): #14477 |
For future context here, we're having a discussion in the PR about how this should work moving forward, because right now there is no way to compile this code. |
I merged changes make it possible to do this particular conversion for master: a455db6 I posted a pitch on the Swift Evolution forum related to the primary issue here, and that's where the discussion should continue: https://forums.swift.org/t/more-consistent-function-types/9765 If we're going to support this in the future, it needs to be as a result of an intentional design decision with tests backing a specific implementation as opposed to an accident of implementation that happened to allow this to work. The underlying implementation issue was also responsible for the accidental tuple splatting behavior, and I would have expected when the work was done to plug that hole this would have stopped working, but that clearly didn't happen. The pitch I made the in SE forum is extremely modest compared to implicit tuple splatting, and makes writing generic code more uniform and reduces boilerplate. There may be other approaches to restoring this behavior, but as I mention in the pitch I believe they will all have unintended knock-on effects. |
I was reminded of Joe's suggestion and although it doesn't work as-is, I think he meant something like this, which does work. f{ (log() ?? { _ in })(()) } It's a lot less boilerplate than what I had previously suggested, and infers |
Attachment: Download
Environment
Swift version 4.1-dev (LLVM ef53654946, Clang f7df1e5a04, Swift 831b78c)
Additional Detail from JIRA
md5: c1cb679edb9f620bc1867d7a0f3e7436
Issue Description:
I'm working on migrating ReactiveSwift early to Swift 4.1 using the latest development snapshot (org.swift.4120180118a, LLVM ef53654946, Clang f7df1e5a04, Swift 831b78c)
This code used to type-check in Swift 4.0:
But in 4.1 it fails with this error:
I've tried several things. For example, removing the as ... results in:
As well as removing one of the sets of parenthesis:
It seems to me that the expected behavior would be that this type-checks without any type hints using as.
For context, this comes from https://github.com/ReactiveCocoa/ReactiveSwift/blob/master/Sources/EventLogger.swift#L108-L117
The text was updated successfully, but these errors were encountered: