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

Spy on protocol that confirms another protocol #47

Open
alpha-moisol opened this issue Oct 26, 2023 · 10 comments
Open

Spy on protocol that confirms another protocol #47

alpha-moisol opened this issue Oct 26, 2023 · 10 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@alpha-moisol
Copy link

alpha-moisol commented Oct 26, 2023

Is your feature request related to a problem? Please describe.
When using @Spyable macro on protocol that confirms another protocol, there is no additional methods of confirmed protocol, which leads to compilation error. Eg:

protocol A {
    func a()
}

@Spyable
protocol B: A {
    func b() 
}

class BSpy: B {
    var bCallsCount = 0
    var bCalled: Bool {
        return bCallsCount > 0
    }
    var bClosure: (() -> Void)?
        func b() {
        bCallsCount += 1
        bClosure?()
    }
}

Describe the solution you'd like
Would be great to have confirmed protocol methods in Spy class

@Matejkob
Copy link
Owner

Hi @alpha-moisol,

Thank you for bringing up this idea!

Ideally, the @Spyable macro would be able to traverse the declaration of each inherited type and automatically generate spies for every member. However, this is not feasible. Macros can only access the AST node to which they're directly attached. Since the declaration of an inherited type could be in a different file or module, it's inaccessible from the macro context. We're limited to knowing the names of the inherited protocols.

I have a few strategies in mind to tackle this challenge, and I'm open to further discussion or fresh perspectives.

  1. Restrict @Spyable Usage: One option is to disallow attaching the @Spyable macro to types with an inheritance clause and throw a compilation error with an explanatory message. Personally, I'm not keen on this approach because it fails to address the need. Plus, there could be valid use-cases where one might want to attach the macro to a protocol that implements "basic" protocols like Sendable or Equatable.

  2. Automatic Spy Inheritance: A more promising route might be to generate spies that inherit from the spied-upon parent protocols, like so:

    @Spyable
    protocol A {
      func a()
    }
    
    @Spyable
    protocol B: A {
      func b()
    }
    // This would generate BSpy inheriting from ASpy

    While this approach actually addresses the issue, it opens up another can of worms. For example, what if a user attaches a "basic" protocol like Sendable or Equatable? Also, can we safely assume that a @Spyable macro has been added to every protocol in the inheritance list?

  3. Extend Macro Interface: A potentially better solution is to extend the macro interface to accept an argument profile, listing the names of Spy classes that should be part of the inheritance clause:

    @Spyable(inheritedTypes: "ASpy")
    protocol B {
      func b()
    }

    This would give developers more control while keeping the macro flexible.

I'm looking forward to your thoughts on these options!

@Matejkob
Copy link
Owner

@dafurman If you would find a while I would love to hear your opinion on this issue ☺️

@alpha-moisol
Copy link
Author

@Matejkob I found 2nd and 3rd approaches valid, but do not quite understand how it can be implemented, cause as you said we have only access to the AST node, so we cannot access ASpy AST from BSpy

@Matejkob
Copy link
Owner

@Matejkob I found 2nd and 3rd approaches valid, but do not quite understand how it can be implemented, cause as you said we have only access to the AST node, so we cannot access ASpy AST from BSpy

@alpha-moisol, I might not have articulated my idea clearly—my apologies. The advantage here is that spies are classes, which allows for overriding. Consider the following example:

@Spyable
protocol A {
  func a()
}

@Spyable
protocol B: A {
  func b()
}

Given that protocol B extends protocol A, we can infer from its name that the corresponding spy for this protocol would be ASpy. With this understanding, we can generate a spy for the B protocol as follows:

class BSpy: ASpy, B {
// Here we include only the methods from the B protocol
// because the methods from A are covered in `ASpy`
}

@alpha-moisol
Copy link
Author

@Matejkob thanks, I understand now, but yes, we cannot do multi inheritance this way

@Matejkob
Copy link
Owner

@Matejkob thanks, I understand now, but yes, we cannot do multi inheritance this way

That is way in my view the best options with current restrictions would be the 3th option.

@alpha-moisol
Copy link
Author

@Matejkob thanks, I understand now, but yes, we cannot do multi inheritance this way

That is way in my view the best options with current restrictions would be the 3th option.

Ok, but what if there will be 2 parent protocols? We still cannot do multi inheritance here

@alpha-moisol
Copy link
Author

As for me, that is the problem, that cannot be solved without global code analyser, unfortunately.

@Matejkob
Copy link
Owner

@Matejkob thanks, I understand now, but yes, we cannot do multi inheritance this way

That is way in my view the best options with current restrictions would be the 3th option.

Ok, but what if there will be 2 parent protocols? We still cannot do multi inheritance here

Oh, that you meant! I've got you!

Yeah... I've forgotten about this limitation.

In that case, probably there is no solution to that.

@dafurman
Copy link
Contributor

This is a pretty interesting issue. I think any way we go with this, there's going to have to be a documentation component of the macro's limitations and our advised workarounds in these situations.

Approach 2

It's syntactically simple and more automated, but as mentioned, it doesn't handle custom protocols well.

If we go with an approach where we grab the conformed protocol and suffix "Spy", that'd lead to trying to inherit from a nonexistent EquatableSpy in this example:

@Spyable
protocol A: Equatable { func a() }
@Spyable
protocol B: A { func b() }

// Generated:
class ASpy: EquatableSpy, A {
    func a() {}
}
class BSpy: ASpy, B {
    func b() {}
}

Our documented workaround could be to suggest creating EquatableSpy. Definitely not ideal, but I think that's better than not supporting this functionality at all. At least we'd be able to provide a solution for some more basic protocol inheritances.

Also, can we safely assume that a @Spyable macro has been added to every protocol in the inheritance list?

This is definitely a concern, but while it could be easy to forget to annotate a parent protocol with @Spyable, if this step is forgotten, the error is at least pretty clearly in the user's face. Ex: "Cannot find type 'ASpy' in scope". It'd be similar to the error that'd show up in the example I gave, Cannot find type 'EquatableSpy' in scope. In both cases, the workaround would be to create these types, either by using @Spyable to generate them, or in the case of EquatableSpy, by creating them by hand.

Approach 3

This requires more work from the user via the parameters, but it has the same issues where a Spy inheritance could be forgotten, by forgetting to use the inheritedTypes parameter, or by using it incorrectly. It also doesn't automatically support basic protocols. Unfortunately, it has the same issue as approach 2, with the same workaround: A custom class conforming to the basic protocol would need to be created:

@Spyable(inheritedTypes: "EquatableSpy")
protocol A: Equatable { func a() }
@Spyable(inheritedTypes: "ASpy")
protocol B: A { func b() }

// Generated:
class ASpy: EquatableSpy, A {
    func a() {}
}
class BSpy: ASpy, B {
    func b() {}
}

With approach 3, applying @Spyable to protocol A would still require the manual creation of some class that conforms to Equatable.

Summary

Given that both approaches have the same issues, and approach 2 keeps the @Spyable API simpler, I'm leaning towards that approach.

To provide some guidance for users, we could generate a comment that links to documentation on the issue of conforming to things like Equatable, or just simply say something like, // Create EquatableSpy manually if it doesn't exist. whenever we see a protocol is conforming to another protocol.

@Matejkob Matejkob added enhancement New feature or request question Further information is requested labels Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants