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 associatedtype generics #1345

Merged
merged 14 commits into from
Jun 19, 2024
Merged

Fix associatedtype generics #1345

merged 14 commits into from
Jun 19, 2024

Conversation

art-divin
Copy link
Collaborator

@art-divin art-divin commented Jun 16, 2024

Resolves: #1333

Description

This MR introduces a correct way of generating mocks for associatedtype in a protocol marked as AutoMockable.

Context

Previously, Sourcery was not generating a compilable mock if a mockable protocol had associatedtypes used as arguments or return values or types for variables in that protocol.

The reason was both that during processing, protocol's generic requirements for associated types were not stored, and that a protocol was not checked if it had associated type or not.

Now, AutoMockable.stencil contains logic to check if there are both associated types and generic requirements for those types in a protocol or not.
Now, if an associated type has a primitive type, and not a protocol, a typealias is injected into the mockable type. When an associated type has a protocol type, then that type is:

  • migrated to Generic constraint of the generated type
  • preserving and specifying generic requirements, if any

Example

Input

//sourcery:AutoMockable
protocol TestProtocol<SomeType> {
    associatedtype SomeType: Sequence where Value.Element: Collection, Value.Element: Hashable, Value.Element: Comparable

    func getValue() -> SomeType

    associatedtype Value2 = Int

    func getValue2() -> Value2
 }

Output

class TestProtocolMock<
    SomeType: Sequence>: TestProtocol
    where SomeType.Element : Collection, SomeType.Element : Hashable, SomeType.Element : Comparable {
    typealias Value2 = Int

    ...
}

@art-divin art-divin added this to the 2.2.5 milestone Jun 16, 2024
@art-divin art-divin self-assigned this Jun 16, 2024
@art-divin art-divin merged commit 7153768 into master Jun 19, 2024
2 checks passed
@art-divin art-divin deleted the fix-associatedtype-generics branch June 19, 2024 18:50
art-divin added a commit that referenced this pull request Jun 22, 2024
commit 33f948a
Author: MontakOleg <[email protected]>
Date:   Sat Jun 22 15:38:14 2024 +0300

    AutoMockable: fix generating static reset func (#1336)

    - don't generate invalid code (see
    #1335)
    - use type's access level for generated reset() function

commit 2f8fc64
Author: art-divin <[email protected]>
Date:   Wed Jun 19 18:58:34 2024 +0000

    Update Docs

commit 7153768
Author: Ruslan A <[email protected]>
Date:   Wed Jun 19 22:50:18 2024 +0400

    Fix associatedtype generics (#1345)

    * Preliminary associatedtype support

    * Implemented associatedtype support with generic requirements

    * Fixed failing test

    * Squashed commit of the following:

    commit 9d01e6f
    Author: Ruslan A <[email protected]>
    Date:   Fri Jun 14 20:06:41 2024 +0400

        Improved concurrency support in SwiftTemplate caching (#1344)

    * Removed test code

    * Removed comment

    * Updated Linux classes

    * update internal boilerplate code.

    * Updated generated code

    * Removed warnings

    * Updated expected file

    * Updated expected file

    * Adjusted protocol type for Linux

    * Removed protocol composition due to Swift compiler crash under Linux
Comment on lines +56 to +59
/// Map associated types
associatedTypes.forEach {
typeMap[$0.key] = $0.value.type
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@art-divin i'm digging into a regression that I have run into and I want to confirm if this change is the culprit or not...

I have the following example:

protocol FooValue {}

protocol Foo {
    associatedtype Bar: FooValue
    var value: Bar { get }
}

class Baz {
    struct Bar {
    }

    let bar: Bar

    init(bar: Bar) {
        self.bar = bar
    }
}

For the Bar associated type inside the Foo protocol, this change causes typeMap["Bar"] to be set to FooValue.

When I dump the parsed AST for Baz, I get expected values from the type of the stored variable, but I get the wrong type from the initialiser parameter:

(lldb) po result.first!.storedVariables.first!.typeName!.name
"Baz.Bar"
(lldb) po result.first!.initializers.first!.parameters.last!.type!.name
"FooValue"

I have a hunch that the issue comes in two parts:

  1. The initializer parameter type isn't getting the global name assigned to its typeName the same way that the store variable does:
    (lldb) po result.first!.storedVariables.first!.typeName
    Baz.Bar
    (lldb) po result.first!.initializers.first!.parameters.last!.typeName
    Bar
    
  2. Associated types are being treated as globals which is a potential cause for conflict

I can try to dig further, or put this into an issue, but I wanted to check to see if this is something that you're actively aware of atm?

Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello @liamnichols ,

amazing work! Very nice of you sharing this 🤝

I have a hunch that the issue comes in two parts:

Sounds like it, there were and still are a number of features related to associated types which are not supported at all.
Previously they were completely ignored when mocks were being generated during to missing metadata.

I can try to dig further, or put this into an issue, but I wanted to check to see if this is something that you're actively aware of atm?

If you can, sure, that would be awesome -> creating an issue, digging further... Sounds like a plan!
To answer your question, I am not aware of this regression and I really appreciate the code sample of things not working as expected in your comment here.

If you would be able to at least provide "Expected" and "Actual" sections in your issue, say, if you cannot fix this at your convenience,
it would play a crucial role in speeding up the investigation and the fix in future. 🙏

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.

Associatedtype not parsing to mock
2 participants