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

Add support for default interface members in inheritance-based proxy types #661

Merged

Conversation

stakx
Copy link
Member

@stakx stakx commented Aug 31, 2023

This is another step towards completing #447. I'm limiting this PR to inheritance-based proxy types (i. e. those without target) due to time constraints – having to adjust only 2 out of 5 implementations is a lot less work! – and because these are likely the most important proxy types for downstream testing libraries such as Moq, NSubstitute, etc.

  • methods
  • generic methods
  • properties and events (i. e. where the accessor methods are not "standalone" ones) – only testing properties at this time, which should be enough
  • different accessibilities (e. g. protected , protected internal, internal)
  • mixins
  • additional interfaces
  • static members
  • static abstract members (requires targeting .NET 7 or newer & a major architectural expansion to include non-instance members; let's do this in a future PR)

Testing all possible combinations of these aspects would be prohibitive, I've therefore restricted tests to a subset of these combinations that seemed reasonable and justifiable by the nature of the actual code changes / extensions made to DynamicProxy.

@stakx stakx self-assigned this Aug 31, 2023
@stakx stakx force-pushed the enhancements/default-interface-members branch from 001f261 to 41ebbcc Compare September 1, 2023 10:07
@stakx stakx force-pushed the enhancements/default-interface-members branch from 41ebbcc to f06e648 Compare September 1, 2023 11:04
@stakx stakx changed the title Draft: Add support for default interface members in inheritance-based proxy types Add support for default interface members in inheritance-based proxy types Sep 1, 2023
@stakx
Copy link
Member Author

stakx commented Sep 1, 2023

@jonorossi, I think I've finally got this in a reasonably understandable & mergable state. I'd appreciate if you could give this at least a cursory review, if you can spare some time.

As noted in the initial description above, this is only for proxies without targets, and I am not testing all possible combinations of DynamicProxy features. Do let me know if you think I have missed any crucial test cases.

Regarding the structure of this PR: most of the commits consist simply of new tests (usually grouped by some aspect listed in the PR's description). Most of those actually pass right away, without any code changes. In the few cases where tests don't pass, the test-only commit is immediately followed by one that makes them succeed. Those actual-code-change commits are sometimes followed by a little refactoring & clean-up work.

@stakx stakx requested a review from jonorossi September 1, 2023 11:27
@stakx stakx added this to the vNext milestone Sep 1, 2023
@stakx stakx mentioned this pull request Sep 1, 2023
... instead of `MethodBase`s to identify which methods get invoked. This
should make tests easier to read & understand.
Two of these tests are failing:

 * Class proxies cannot intercept interface methods with a default
   implementation if the proxied class type does not override it.

 * Interface proxies cannot proceed to default implementations.
A class implementing an interface with default method implementations
will not necessarily override those methods; in such cases, the methods
are currently "invisible" to DynamicProxy and thus won't get proxied.

This can be solved by an extra pass over all interfaces to collect such
methods, as is done in this commit.

Ideally we wouldn't have to iterate over all interfaces more than once,
so we should next make an effort to limit the runtime impact of this
extra pass.
Assuming that default interface methods are relatively rare, if we keep
track of which interfaces contain any of them at all, we can then limit
our extra pass during class proxy generation to just those interfaces
and exclude the rest (i.e. hopefully the majority).

Additionally, our collector may skip some of the more expensive checks
by bailing out for methods it isn't actually interested in.
... by properly forwarding the "standalone" flag in the newly added
collector.
Those essentially target "mixed" scenarios where default implementations
show up next to "regular" methods. This is to better ensure that the new
code paths will not interfere with pre-existing logic.
The code changes we made do not suggest that pre-existing logic dealing
with generics might somehow no longer work, however we did add a bit of
new code for generics which we should target with at least one test.
This includes a test for `static abstract` methods that remains ignored
for now, since we haven't implemented support for this C# 11 language
feature, nor do we target any runtimes that support it. (The test is
being added as a future reminder, once we start targeting .NET 7+.)
A few of these tests fail due to `Debug.Assert` failures in `Interface-
MembersWithDefaultImplementationCollector`.
`sealed` methods do not show up in interface mappings, so make sure
`InterfaceMembersWithDefaultImplementationCollector` can deal with them
anyway in case of interfaces that contain both `sealed` methods as well
as overridable ones with a default implementation.
@stakx stakx force-pushed the enhancements/default-interface-members branch from f06e648 to 51112db Compare September 10, 2023 22:08
Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

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

Looks good, sorry for taking so long to get to this. From a cursory review I don't see anything missing.

Nice work with the extremely neat and organised commits.

@stakx
Copy link
Member Author

stakx commented Aug 30, 2024

I think I've let this sit for long enough. (My bad!) I looked over the code again with a fresh set of eyes, this should still be good to go.

@stakx stakx merged commit a3e629b into castleproject:master Aug 30, 2024
3 checks passed
@stakx stakx deleted the enhancements/default-interface-members branch August 30, 2024 14:09
@stakx stakx mentioned this pull request Aug 30, 2024
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