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

Abstract base type for IMaterializationInterceptor, possibly other interceptor interfaces #34764

Closed
kjkrum opened this issue Sep 26, 2024 · 3 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported type-enhancement

Comments

@kjkrum
Copy link

kjkrum commented Sep 26, 2024

The docs for interceptor interfaces suggest extending their abstract base types instead of directly implementing the interface. IMaterializationInterceptor is missing a corresponding abstract base type. Some interfaces, like ISingletonInterceptor, have no abstract base type because they have no methods. Others, like IIdentityResolutionInterceptor, have multiple abstract base types. I think MaterializationInterceptor is the only one missing, but please check that.

Why this matters

#27625 added default method implementations for all interceptor interface methods. Interfaces with default method implementations do not play nicely with Visual Studio. There is no suggestion to implement the interface because you don't have to. There is no "generate override" refactoring because your implementation is not an override. You have to manually write the correct method signatures or copy-paste them from the docs or decompiled definition. When extending an abstract base class, you can use the "generate override" refactoring.

@roji
Copy link
Member

roji commented Sep 27, 2024

Interfaces with default method implementations do not play nicely with Visual Studio.

Yes, when EF was starting out, default method implementations (DIM) didn't exist yet, so we had abstract types; when DIM was introduced, we switched to it for simplification.

I don't think we'd add an abstract base class just because Visual Studio supports DIM a bit less nicely; DIM are a fully supported C# language feature, and the degree of support of an IDE isn't cause to not use it. If you think VS's support is lacking, I'd suggest opening a ticket with VS themselves, describing the problems you see.

@kjkrum
Copy link
Author

kjkrum commented Sep 27, 2024

@roji The purpose of DIM is to allow adding methods to published interfaces. It should only be used for that purpose. VS aside, DIM still behaves differently than normal interface implementations in that it requires calling the default methods via the interface rather than the class type. IMO, #27625 was a mistake that could and should be corrected in a future major version.

@roji
Copy link
Member

roji commented Oct 7, 2024

The purpose of DIM is to allow adding methods to published interfaces. It should only be used for that purpose.

Where does this rule come from? What's the advantage of adding an abstract base class in addition to an interface, if the same can be done without doing that?

DIM still behaves differently than normal interface implementations in that it requires calling the default methods via the interface rather than the class type.

Yep, but does that matter in this case?

@cincuranet cincuranet added the closed-no-further-action The issue is closed and no further action is planned. label Oct 22, 2024
@cincuranet cincuranet closed this as not planned Won't fix, can't repro, duplicate, stale Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants