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

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

Closed
grigorye opened this issue Aug 14, 2024 · 3 comments · Fixed by #114

Comments

@grigorye
Copy link
Contributor

grigorye commented Aug 14, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants