swift-spyable icon indicating copy to clipboard operation
swift-spyable copied to clipboard

Fix #52 - Support generic functions

Open dafurman opened this issue 9 months ago • 4 comments

Today, the following code

@Spyable
protocol ServiceProtocol {
  func foo<T>() -> T
}

will produce

Spy that doesn't compile
class ServiceProtocolSpy: ServiceProtocol {
    var fooCallsCount = 0
    var fooCalled: Bool {
        return fooCallsCount > 0
    }
    var fooReturnValue: T! // ❌ Cannot find type 'T' in scope
    var fooClosure: (() -> T)? // ❌ Cannot find type 'T' in scope
      func foo<T>() -> T {
        fooCallsCount += 1
        if fooClosure != nil {
            return fooClosure!()
        } else {
            return fooReturnValue
        }
    }
}

This PR adds support for generic functions by having stored properties retain generic types as Any, generating this instead:

Spy that compiles
class ServiceProtocolSpy: ServiceProtocol {
    var fooCallsCount = 0
    var fooCalled: Bool {
        return fooCallsCount > 0
    }
    var fooReturnValue: Any!
    var fooClosure: (() -> Any)?
      func foo<T>() -> T {
        fooCallsCount += 1
        if fooClosure != nil {
            return fooClosure!()
        } else {
            return fooReturnValue
        }
    }
}

More information and guidance about this has been added to the README in this PR as well.


This PR has already gotten really large, but I'm interested in these followup improvements:

  1. I feel that crashing in a failed force cast could have better messaging. Screenshot 2023-11-27 at 10 03 57 PM It's hard to know for sure that the "signal SIGABRT" is caused by a casting issue unless you look at the console. The root cause is not as in-your-face as it could be. I'd like to Improve erroring to not crash directly during a force cast, but to instead rely on guards and fatalErrors with more precise error messaging.

For example, something like this:

guard let returnValue = useGenericsOneTwoThreeFourReturnValue as? T else {
    fatalError("Expected functionNameReturnValue to be of type T, but found \(type(of: returnValueVariableName!))")
}
  1. Swap uses of Any with generic constraints where possible, to ease use when you only need to refer to the generic by its constraint. This is described well by @arennow here.

dafurman avatar Nov 28 '23 06:11 dafurman

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (8ced7a5) 98.36% compared to head (b5267c0) 98.63%. Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
+ Coverage   98.36%   98.63%   +0.26%     
==========================================
  Files          18       20       +2     
  Lines         736      879     +143     
==========================================
+ Hits          724      867     +143     
  Misses         12       12              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 28 '23 07:11 codecov[bot]

@Matejkob Just a heads up, after my other two PRs get in, I’ll likely take another look at self-reviewing this PR with a bit of a fresh take having given this some time. I’m not sure how possible it’d be, but I’d like to see if I can reduce the size of this PR at all to make this more manageable. For example, I realized that my implementation for supporting parameter packs may need some more work, so it might be possible to just avoid supporting them in this PR (they’re already essentially not usable yet anyways) and leave that for a smaller follow-up.

dafurman avatar Dec 29 '23 23:12 dafurman

@Matejkob Just a heads up, after my other two PRs get in, I’ll likely take another look at self-reviewing this PR with a bit of a fresh take having given this some time. I’m not sure how possible it’d be, but I’d like to see if I can reduce the size of this PR at all to make this more manageable. For example, I realized that my implementation for supporting parameter packs may need some more work, so it might be possible to just avoid supporting them in this PR (they’re already essentially not usable yet anyways) and leave that for a smaller follow-up.

@dafurman, thanks for the heads-up! I appreciate your willingness to revisit the PR with fresh eyes — it often brings new insights. Your idea to streamline the PR by postponing certain features, like the parameter packs implementation, sounds like a smart approach to maintain manageability. It's usually better to focus on getting the more stable parts merged first and then iterate. I look forward to seeing the refined PR and am happy to help review or brainstorm further reductions if needed.

Matejkob avatar Dec 30 '23 11:12 Matejkob

@Matejkob Alrighty I think I've simplified my approach as much as I'm able to right now. It's still quite a bit of code, so no rush on getting to this, I totally understand it's a lot to review!

The main meat of the changes is in TypeSyntax+Extensions.swift, with most of the rest of the code changes being added to support additions in this file.

To shrink the changes down from my original approach, to make this more reviewable, I also culled syntax types being initially supported for generics down to these, which I think ought to be enough for demonstrating how we can support generics in a fairly modular way. I can add in more types as a followup PR if we're happy with this approach.

dafurman avatar Jan 07 '24 03:01 dafurman