pointfreeco / swift-issue-reporting

Report issues in your application and library code as Xcode runtime warnings, breakpoints, assertions, and do so in a testable manner.
https://www.pointfree.co
MIT License
391 stars 64 forks source link

`unimplemented()` without placeholder for @MainActor closures crashes _on generation_ (regression vs 1.1.2) #113

Closed grigorye closed 2 months ago

grigorye commented 2 months ago

We discovered the following issue while upgrading from 1.1.2 to the latest version of XCTDynamicOverlay (1.2.2):

The code that employs unimplimented for closures that are @MainActor (we don't employ placeholders), crashes right at the moment of initialization of the closure vs. at the moment of actually invoking it. Further investigation revealed that it's due to wrong (non closure specific) overload of unimplemented employed by the compiler in cases like this. It does not happen if @MainActor is omitted from the closure. It also clearly looks like a regression vs. 1.1.2 where the correct overload is employed in both cases.

Screenshot 2024-08-14 at 14 55 04 Screenshot 2024-08-14 at 14 55 38 Screenshot 2024-08-14 at 14 52 10

I'm adding a PR with the tests that exercise the above scenario, for illustration purposes.

stephencelis commented 2 months ago

@grigorye Just a note, but the version that doesn't take a placeholder is technically deprecated and will be removed in a future version of the library. Not only do we want to avoid potential crashes, but some of the tricks employed to provide defaults has subtly changed in Swift 6 that can produce more crashes than before.

We'll see if it's possible to fix this regression, but please consider migrating to the newer, supported APIs.

grigorye commented 2 months ago

@grigorye Just a note, but the version that doesn't take a placeholder is technically deprecated and will be removed in a future version of the library. Not only do we want to avoid potential crashes, but some of the tricks employed to provide defaults has subtly changed in Swift 6 that can produce more crashes than before.

Yeah, we indeed already employed placeholders-enabled versions as a workaround. Btw, unrelated, but do I get it correctly, placeholders are also necessary to guide the compiler into choosing the correct overload? Otherwise, I wonder if it could be possible to generate them automatically? (I ended up using : .init() for every placeholder that I used as part of the workaround).

stephencelis commented 2 months ago

The placeholders are necessary because the function can get called and must return data to not crash. Crashing by default is not only disruptive to tests, but could potentially cause crashes to your application if an unimplemented endpoint is left unimplemented.

It's not possible to automatically generate placeholders because Swift doesn't have the concept of an Initializable protocol with an init() requirement. The previous version of the library attempted to provide this internally, but we encountered language bugs that made it unreliable, thus the new crashes in Swift 6.