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: genericmethodinvoker now correctly handles generic methods with overloads #6844

Merged

Conversation

oising
Copy link
Contributor

@oising oising commented Nov 21, 2020

  • added code to select correct overload for generic methods; previously was always selecting first overload, regardless of parameters provided.

  • invokers cache now caches with a compound key of generic type parameter(s) + argument type(s). Before it was caching by type parameter(s) only, which would collapse all overloads sharing type parameters into one entry, e.g.

        Task<int> Method<T>(int x);
        Task<int> Method<T>(int x, string y);

If these two methods were invoked with the same T, then the callsite cache would only ever invoke the first match, even if two params were passed.

  • improve error message on failure to find suitable overload (it now shows the actual type parameters, and not just the arity)

  • added tests for invoker

image

Closes #6801

@dnfadmin
Copy link

dnfadmin commented Nov 21, 2020

CLA assistant check
All CLA requirements met.

@oising
Copy link
Contributor Author

oising commented Nov 21, 2020

There's room to reduce allocations by killing off LINQ usage. I may add another commit this weekend.

@oising
Copy link
Contributor Author

oising commented Nov 21, 2020

Oh, interesting -- it seems I triggered two failures in two other generic method tests. I wonder if I broke something, or revealed more bugs...

edit: I'm dumb. My bugs. I found my bugs.

@oising oising marked this pull request as draft November 21, 2020 15:53
clean up old experiments

extend tests; tweaks

update grain reference generator to pack argument types (as well as values) into args for invoker

rewrite generic method invoker's routine to find appropriate generic overloads using types instead of values

update tests for genericmethodinvoker
@oising oising force-pushed the feature/genericmethodinvoker-overloads branch from d0345db to 44f56f1 Compare December 6, 2020 07:55
@oising
Copy link
Contributor Author

oising commented Dec 6, 2020

Looking good so far...

image

@oising oising marked this pull request as ready for review December 6, 2020 08:29
@ReubenBond ReubenBond merged commit 043fa5a into dotnet:master Dec 16, 2020
@ReubenBond
Copy link
Member

Great work, thank you!

@oising oising deleted the feature/genericmethodinvoker-overloads branch January 30, 2021 22:12
ReubenBond added a commit to ReubenBond/orleans that referenced this pull request Apr 7, 2021
…loads (dotnet#6844)

* Fix GenericMethodInvoker to handle overloaded methods. Add tests.

Co-authored-by: Oisin Grehan <oisin.grehan@hiloenergie.com>
ReubenBond added a commit to ReubenBond/orleans that referenced this pull request Jun 18, 2021
…loads (dotnet#6844)

* Fix GenericMethodInvoker to handle overloaded methods. Add tests.

Co-authored-by: Oisin Grehan <oisin.grehan@hiloenergie.com>
oising added a commit to oising/orleans that referenced this pull request Aug 4, 2021
ReubenBond pushed a commit that referenced this pull request Aug 5, 2021
Co-authored-by: Oisin Grehan <oisin.grehan@hiloenergie.com>
ReubenBond pushed a commit that referenced this pull request Aug 11, 2021
…#6844) (#7190)

* backport PR #6844 to 3.5.0: generic overloads selection bugfix

* fixed nuget reference (constant version) for codeanalysis to build var

* add failing test case

* Implement support for ValueTask and ValueTask<T> grain return types for generic grain methods.

* address Reuben's comment

* address Reuben's comment, again :)

Co-authored-by: Oisin Grehan <oisin.grehan@hiloenergie.com>
benjaminpetit pushed a commit to benjaminpetit/orleans that referenced this pull request Sep 22, 2021
…dotnet#6844) (dotnet#7190)

* backport PR dotnet#6844 to 3.5.0: generic overloads selection bugfix

* fixed nuget reference (constant version) for codeanalysis to build var

* add failing test case

* Implement support for ValueTask and ValueTask<T> grain return types for generic grain methods.

* address Reuben's comment

* address Reuben's comment, again :)

Co-authored-by: Oisin Grehan <oisin.grehan@hiloenergie.com>
ReubenBond pushed a commit that referenced this pull request Sep 22, 2021
Co-authored-by: Oisin Grehan <oisin.grehan@hiloenergie.com>
ReubenBond pushed a commit that referenced this pull request Sep 22, 2021
…#6844) (#7190)

* backport PR #6844 to 3.5.0: generic overloads selection bugfix

* fixed nuget reference (constant version) for codeanalysis to build var

* add failing test case

* Implement support for ValueTask and ValueTask<T> grain return types for generic grain methods.

* address Reuben's comment

* address Reuben's comment, again :)

Co-authored-by: Oisin Grehan <oisin.grehan@hiloenergie.com>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overloaded generic grain methods resolve to only 1 implementation
3 participants