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

Xcode 7.3 beta 5 miscompiles SignalProducerType.combineLatestWith on ARM64 #2751

Closed
jckarter opened this issue Mar 4, 2016 · 13 comments
Closed
Labels
Milestone

Comments

@jckarter
Copy link

jckarter commented Mar 4, 2016

I hate to incovenience you all, but we have a bug in the 7.3 betas that causes us to miscompile the currying of Signal.combineLatestWith here:

    public func combineLatestWith<U>(otherProducer: SignalProducer<U, Error>) -> SignalProducer<(Value, U), Error> {
        return liftRight(Signal.combineLatestWith)(otherProducer)
    }

I'm investigating a fix right now. In case we don't fix it in time for 7.3 final, I recommend hand-inlining the liftRight combinator here as a workaround,

    public func combineLatestWith<U>(otherProducer: SignalProducer<U, Error>) -> SignalProducer<(Value, U), Error> {    
        return SignalProducer { observer, outerDisposable in
            self.startWithSignal { signal, disposable in
                outerDisposable.addDisposable(disposable)

                otherProducer.startWithSignal { otherSignal, otherDisposable in
                    outerDisposable.addDisposable(otherDisposable)

                    signal.combineLatestWith(otherSignal).observe(observer)
                }
            }
        }
    }

The bug on our side is rdar://problem/24960559, for reference.

@NachoSoto
Copy link
Member

I can't even begin to express how excited I am to see a Swift contributor open an issue, not only explaining the bug but offering a workaround 😍

If it's not fixed by the GM we can temporarily make this change in #2684, since we'll likely need to change other things for Swift 2.2 anyway.

Thanks a lot @jckarter 😄

@NachoSoto NachoSoto added the bug label Mar 4, 2016
@NachoSoto NachoSoto added this to the 4.1 milestone Mar 4, 2016
@mdiep
Copy link
Contributor

mdiep commented Mar 5, 2016

I can't even begin to express how excited I am to see a Swift contributor open an issue, not only explaining the bug but offering a workaround

Yes! Thank you!!! 😍

@calebd
Copy link
Contributor

calebd commented Mar 8, 2016

@jckarter Is there an issue on bugs.swift.org that we can follow as well?

@jckarter
Copy link
Author

jckarter commented Mar 8, 2016

Not that I know of. The radar unfortunately contains a private project I can't post publicly.

@NachoSoto
Copy link
Member

@calebd reports that this is still an issue on the final 7.3.

I'm finishing the download now. I'll make a branch with Swift 2.2 changes (probably including this hopefully temporary workaround) and try to make a release by EOD today.

NachoSoto added a commit that referenced this issue Mar 21, 2016
Running tests with -O (and ensuring `ENABLE_TESTABILITY` is `YES`) crash.
@NachoSoto
Copy link
Member

Confirmed, this crashes when running tests with -O. I added a workaround in #2760: 6f63eb6.

@mdiep
Copy link
Contributor

mdiep commented Mar 26, 2016

Fixed by #2760.

@NachoSoto
Copy link
Member

Let's keep this open so that we remember to get rid of the workaround once this is fixed in Swift :)

@mdiep mdiep removed this from the 4.1 milestone Mar 26, 2016
@calebd
Copy link
Contributor

calebd commented Mar 28, 2016

@jckarter Any idea if this will be fixed in the 2.2.x milestone?

@jckarter
Copy link
Author

We'll try, at least. We're looking into it.

@jckarter
Copy link
Author

Looks like @eeckstein has a fix on the way: apple/swift#1916

@ikesyo ikesyo mentioned this issue Apr 3, 2016
2 tasks
@ikesyo
Copy link
Member

ikesyo commented Apr 20, 2016

It seems that this is fixed by apple/swift#1937 and Xcode 7.3.1 / Swift 2.2.1 contains the fix.

https://github.com/apple/swift/releases/tag/swift-2.2.1-RELEASE

@mdiep mdiep added this to the 5.0 milestone May 13, 2016
NachoSoto added a commit that referenced this issue Jul 29, 2016
This was originally introduced in #2751, but it's not necessary anymore. Tests pass with `-O`!.
This will have to be changed to `liftLeft` once rebasing #3096.
NachoSoto added a commit that referenced this issue Aug 3, 2016
This was originally introduced in #2751, but it's not necessary anymore. Tests pass with `-O`!.
This will have to be changed to `liftLeft` once rebasing #3096.
NachoSoto added a commit that referenced this issue Aug 3, 2016
This was originally introduced in #2751, but it's not necessary anymore. Tests pass with `-O`!.
@mdiep
Copy link
Contributor

mdiep commented Aug 8, 2016

The workarounds have been removed for Swift 3!

@mdiep mdiep closed this as completed Aug 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants