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

Incorrect Generation of Closures with Multiple Optional Parameters Using any Keyword in Automockable.stencil Template #1366

Open
lucaArchidiacono opened this issue Sep 6, 2024 · 3 comments

Comments

@lucaArchidiacono
Copy link

lucaArchidiacono commented Sep 6, 2024

What version of Swift are you using (swift --version)?

swift-driver version: 1.113 Apple Swift version 6.0 (swiftlang-6.0.0.7.6 clang-1600.0.24.1)
Target: arm64-apple-macosx14.0

What did you do?

I'm using Sourcery 2.2.5 and the default Automockable.stencil template. The default template for Automockable, parses and generates not compiler safe code.
Given the following code:

// sourcery: AutoMockable
protocol AnyClosureProtocol {
    func evaluateJavaScript(_ javaScriptString: String, completionHandler: (@MainActor @Sendable (Any?, (any Error)?) -> Void)?)
}

What did you expect to see?

I expect the default Automockable.stencil to generate compiler safe code. I expected that it generates the following code:

    //MARK: - evaluateJavaScript

    var evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCallsCount = 0
    var evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCalled: Bool {
        return evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCallsCount > 0
    }
    var evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidClosure: ((String, (@MainActor @Sendable (Any?, (any Error)?) -> Void)?) -> Void)?

    func evaluateJavaScript(_ javaScriptString: String, completionHandler: (@MainActor @Sendable (Any?, (any Error)?) -> Void)?) {
        evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCallsCount += 1
        evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidClosure?(javaScriptString, completionHandler)
    }

What did you see instead?

When building and trying to generate mocked code, the compiler will fail and tell that the newly generated code has wrong syntax.
This is the generated code it came up with:

    //MARK: - evaluateJavaScript

    var evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCallsCount = 0
    var evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCalled: Bool {
        return evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCallsCount > 0
    }
    var evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidClosure: ((String, ((@MainActor @Sendable (Any)?, (any Error)?) -> Void))?) -> Void)?

    func evaluateJavaScript(_ javaScriptString: String, completionHandler: (((@MainActor @Sendable (Any)?, (any Error)?) -> Void))?) {
        evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidCallsCount += 1
        evaluateJavaScriptJavaScriptStringStringCompletionHandlerMainActorSendableAnyAnyErrorVoidVoidClosure?(javaScriptString, completionHandler)
    }

This does not work because of the followintg compiler errors:
image
image

What is the actual problem?

The problem is, that it replaces every ? with )? which should not be the case for the Any? parameter.
This happens only, and only if:

  1. The parameter is a closure
  2. Is optional
  3. contains:"any "

The replace function is too naive and adds too many parentheses.
Here are the lines, which are in my opinion, the places where the replace is to generous:

@lucaArchidiacono
Copy link
Author

Here is a naive solution I have in mind. Please check it out and let me know if thats the correct way to go forward or not.
I'd also like to contribute and help solving this issue if possible.

@iDevid
Copy link
Contributor

iDevid commented Sep 13, 2024

I had several problems with closures and I fixed them all with the snippet from @lucaArchidiacono , thanks!

@krzysztofzablocki
Copy link
Owner

@lucaArchidiacono if you submit PR will be able to see if there are any tests failing with that approach, we can deal with that and get it merged into upstream then, would appreciate it!

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

No branches or pull requests

3 participants