Match generic parameters by position, not by name #580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stakx
force-pushed
the
generic-parameter-name-mismatch
branch
from
bba86ea
to
3e7ce38
Compare
stakx
commented
Feb 22, 2021
|
Seems to fix #585. |
teneko
added a commit
to teneko/Teronis.DotNet
that referenced
this pull request
Mar 14, 2021
- a pending pull request (castleproject/Core#580) has fixed this :)
jonorossi
approved these changes
Mar 24, 2021
To understand why this change is permissible, it is helpful to know the types generated by DynamicProxy: * Proxy types. Those are (at present) never generic. * Delegate types and invocation classes. Those may be generic, and when they are, their generic parameters correspond exactly with those of some target method. It is this correspondence which makes any mapping by name unnecessary.
The constructor being changed is only called by event/property emitters, via the `MetaType` model, and by contributors, all of which are used for proxy type generation only (and those are never generic). The only generic types generated by DynamicProxy are delegate types and invocation classes, but their generators never call this `MethodEmitter` ctor.
Because `name2GenericType` is in practice always an empty dictionary, the helper methods `ExtractCorrectType` and `ExtractParametersTypes` in `GenericUtil` don't really do anything. They can be removed since noone else is using them. (Side note: it looks like `ExtractCorrectType` might have malfunctioned, had it ever actually done anything: it maps parameters `T` -> `Tmapped` and arrays `T[]` -> `Tmapped[]`, but forgets about instantiatd generic types involving `T` as an argument, such as `IEnumerable<T>`.)
stakx
force-pushed
the
generic-parameter-name-mismatch
branch
from
3e7ce38
to
c3780f1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #106. See commit messages for further explanations.
This is a redo of #465, which we closed because I needed more time to study how DynamicProxy deals with generics. I think I've now got an understanding of the bigger picture, and am feeling more confident that the proposed change is sound.
One might think that some of the infrastructure being removed here could have come in handy once we decide to support open generic proxy type generation (#448). However, I don't think this is the case:
As one commit message remarks, some of the parameter mapping code appears to be incomplete. Worse than that, it might even be buggy. It is hard to tell which because we don't have any test cases nor real-world scenarios that cover it (which also makes it so much harder to even understand what the code was supposed to achieve).
More importantly, matching arguments by name fundamentally seems like the wrong approach, because that is not how the underlying platform works: the CLI generally identifies generic parameters by their position.
I think we're better off basing future developments on new (position-based) code instead of the
name2GenericTypeinfrastructure being removed here.