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

Fix #52 - Support generic functions #71

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dafurman
Copy link
Contributor

@dafurman dafurman commented Nov 28, 2023

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.

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.62%. Comparing base (424a9f3) to head (7e794c8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
+ Coverage   98.41%   98.62%   +0.20%     
==========================================
  Files          18       20       +2     
  Lines         631      727      +96     
==========================================
+ Hits          621      717      +96     
  Misses         10       10              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

return cast.nestedTypeSyntaxes
}
// Return an empty array for unsupported types
return []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, we could throw an error, but this would result in error propagation that could ends at forceCastType in FunctionDeclSyntax+Extension.swift by having that return nil. If we wanted to take it even further, we could continue propagating all the way to the top of the Spy factory and emit a diagnostic as well.

I'm just not sure if this is worth the trouble and adding throws everywhere, as I think the only situation that'd reach this code path is if this computed var is triggered on a MissingTypeSyntax, and in that case, I think returning an empty array would probably be fine.

The same applies to erasingGenericTypes() below.

@dafurman dafurman marked this pull request as ready for review November 30, 2023 21:22
@dafurman
Copy link
Contributor Author

@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.

@Matejkob
Copy link
Owner

@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.

@dafurman
Copy link
Contributor Author

dafurman commented Jan 7, 2024

@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 dafurman changed the title Fix #36 - Support generic functions Fix #52 - Support generic functions Jan 7, 2024
# Conflicts:
#	Examples/Sources/ViewModel.swift
#	Sources/SpyableMacro/Factories/ClosureFactory.swift
#	Tests/SpyableMacroTests/Factories/UT_ClosureFactory.swift
#	Tests/SpyableMacroTests/Factories/UT_ReceivedArgumentsFactory.swift
#	Tests/SpyableMacroTests/Factories/UT_SpyFactory.swift
@Matejkob Matejkob self-assigned this Jun 29, 2024
@Matejkob Matejkob self-requested a review June 29, 2024 15:53
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

Successfully merging this pull request may close these issues.

2 participants