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 code generation for interfaces with overloaded methods #38

Conversation

HugoX104447
Copy link
Contributor

Context

This pull request addresses an issue in the code generation process for interfaces with overloaded methods. The current implementation does not correctly handle the creation of the storing bags, due to the method GetUniqueMethodName() returning a to generic name.

void Accept(ISymbolVisitor visitor);      // GetUniqueMethodName() returns Accept_ISymbolVisitor
T Accept<T>(ISymbolVisitor<T> visitor);   // GetUniqueMethodName() returns Accept_ISymbolVisitor 

leading to:

9> error CS0102: The type 'ICalculatorMockSetup' already contains a definition for 'Accept_ISymbolVisitorBagStore'
9> error CS0102: The type 'ICalculatorMockSetup.ICalculatorMockCallTracker' already contains a definition for 'Accept_ISymbolVisitorCallStore'

Changes Made

  • Refactored GetUniqueMethodName Method:

    • Keep default behavior for ArgCollectionName creation.
  • Added GetUniqueStoreName Method:

    • This method handles creation of a more specified name for the bags.
  • Updated Unit Tests:

    • Added two new test interfaces ISymbolVisitor.
    • Added two overloaded methods to validate the behavior of the refactored code.

This PR implements the second feature, which depends on the changes made in PR #36 (fix-interface-inheritance). Please review and merge PR #36 before moving on to this one.

- Update the code generator to properly recognize and process inherited
  interfaces.
- Add unit test to validate the changes.
@connorivy connorivy changed the base branch from main to dev January 23, 2025 21:41
@connorivy connorivy changed the base branch from dev to fix-unique-method-names January 23, 2025 22:28
@connorivy connorivy merged commit 87d1a25 into connorivy:fix-unique-method-names Jan 23, 2025
2 of 3 checks passed
connorivy added a commit that referenced this pull request Jan 23, 2025
* Fix code generation for methods with default arguments

* Fix code generation for interfaces inheriting from other interfaces (#36)

- Update the code generator to properly recognize and process inherited
  interfaces.
- Add unit test to validate the changes.

* use unified interface and run formatter

* Fix code generation for methods with default arguments (#35)

* move default argument tests to shared project

* Fix code generation for interfaces with overloaded methods (#38)

* Fix code generation for interfaces inheriting from other interfaces

- Update the code generator to properly recognize and process inherited
  interfaces.
- Add unit test to validate the changes.

* Fix code generation for interfaces with overloaded methods

---------

Co-authored-by: Karsten Heimrich <[email protected]>
Co-authored-by: Karsten Heimrich <[email protected]>
@connorivy
Copy link
Owner

@HugoX104447,

Good catch!

I merged this one into 1.1.0, but I changed it a bit. I changed the uniqueMethodName to just include the type parameters. Your PR changed the names of the call store to be ICalculator_Public_Void_Accept__ISymbolVisitorCallStore and ICalculator_Public_T_Accept_T_ISymbolVisitorCallStore, but I though just including the type parameters and having them named Accept_ISymbolVisitorCallStore and Accept_T_ISymbolVisitorCallStore was sufficient.

Let me know if you think there is some issue with that approach

@HugoX104447
Copy link
Contributor Author

I think your changes are totally fine. Many thanks for taking the time to review and apply my changes.

@HugoX104447 HugoX104447 deleted the fix-interface-method-overloads branch January 24, 2025 16:23
@HugoX104447
Copy link
Contributor Author

Now I remember why I introduced the GetUniqueStoreName; see the related issue #40 for more details.

By the way, I think the GetUniqueMethodName could benefit from a comment clarifying that the generated method name needs to follow a specific format. This seems important because GetArgCollectionName

protected string GetArgCollectionName(bool includeGenericType = true)
appears to have a hard dependency on the naming convention. Additionally, the code in ArgumentModifierTests
var outParamType = typeof(AllOverloadsMockSetup.OutArgument_OutInt32Collection);
seems to expect code generated in a specific format. While I couldn’t fully trace why this dependency exists, it might be worth adding a comment or documentation to clarify.

Thanks again for providing this framework and for investing your valuable time. I really 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

Successfully merging this pull request may close these issues.

2 participants