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

Implement IFromDiscriminator interface (.NET 6+) #232

Open
MihaMarkic opened this issue May 3, 2024 · 12 comments
Open

Implement IFromDiscriminator interface (.NET 6+) #232

MihaMarkic opened this issue May 3, 2024 · 12 comments

Comments

@MihaMarkic
Copy link
Contributor

Models could implement an interface like this for .NET 6+:

public interface IFromDiscriminator<T>
    where T: IParsable, IAdditionalDataHolder, new()
{
    static abstract T CreateFromDiscriminatorValue(IParseNode parseNode);
}

Then such methods are possible:

public Task<T?> DeserializeAsync<T>(MemoryStream  stream, CancellationToken ct)
    where T : IFromDiscriminator<T>, IParsable, IAdditionalDataHolder, new()
{
    var pnf = ParseNodeFactoryRegistry.DefaultInstance;
    var root = await pnf.GetRootParseNodeAsync("application/json", stream);
    return root!.GetObjectValue<T>(T.CreateFromDiscriminatorValue);
}
@baywet
Copy link
Member

baywet commented May 3, 2024

We've considered that, a couple of challenges with that approach though:

  • only available for dotnet from a design perspective
  • would require us to drop netfx and netcore < 6 compatibility

Besides standardized/cleaner code, what other benefits do you think that would yield?

@MihaMarkic
Copy link
Contributor Author

"only available for dotnet from a design perspective"
Absolutely- hence I'm posting in -dotnet repo.

"would require us to drop netfx and netcore < 6 compatibility"
Why would this be the case? I mean you can add a #if NET6+/#endif interface implementation declaration to each model and also implement the interface only for .NET6+. It shouldn't affect other targets.
i.e

public class SomeModel: IParsable
#if NET6_0_OR_GREATER 
, IFromDiscriminator<SomeModel>
#endif

About benefits - well, isn't standardized/cleaner code enough of a benefit?

@baywet
Copy link
Member

baywet commented May 3, 2024

Sure, but it has a cost in terms of binary size at the end. And if it doesn't enable new scenarios, we're unlikely to accept such a change.
Doing some more research, I couldn't find a reference to that interface, Can you point to some documentation about it please?

@MihaMarkic
Copy link
Contributor Author

Ah, that is just a sample interface I've crafted for demonstration (though it can be useful as it is). Sorry for not making it clearer.
And I doubt that code will increase beyond a bunch of bytes.

@baywet
Copy link
Member

baywet commented May 3, 2024

I'm still trying to understand the design here (outside of the partial discussion on the other thread).
If this is a new interface we'd define, why would the support be limited to net6+ ?
Why would the method be an instance method? (besides the fact that you want it to be part of the interface) This would require one to:

  1. instantiate a "dummy" object.
  2. call the deserialize async method.
  3. get the object they actually care about.

I'm not sure this would be a better experience than what we have today? (besides the fact that you don't need to get the parse node yourself)

@MihaMarkic
Copy link
Contributor Author

The trick is with static abstract interface method introduced in C# 11/.NET 7 which allows static methods to be a part of the interface, in this case it's static CreateFromDiscriminatorValue(...) method.
(Actually, it was available in .NET6 through (.NET7) preview, hence a minimum is .NET7, not .NET6 as I wrote above.)

So, it's static, not instance, method provided through the interface. Does it explain it?

@baywet
Copy link
Member

baywet commented May 3, 2024

ah, I missed that detail, thanks for pointing this out.

What if we had instead

public interface IFromDiscriminator<T>
    where T: IParsable
{
#if NET7_OR_GREATER
    static abstract T CreateFromDiscriminatorValue(IParseNode parseNode);
#endif
}

(note the removal of IAdditionalDataHolder as well)

This way we could add the interface declaration to all models, and it'd work in any context?

Technically we could even have a default implementation for all classes that don't have discriminated types like this, and omit generating generate the method with conditional directive, leading to smaller binary size for more modern applications.

public interface IFromDiscriminator<T>
    where T: IParsable, new()
{
#if NET7_OR_GREATER
    static virtual T CreateFromDiscriminatorValue(IParseNode parseNode)
    {
            _ = parseNode ?? throw new ArgumentNullException(nameof(parseNode));
            return new T();
    }
#endif
}

Which would lead to this in the models

+       #if !NET7_OR_GREATER
-        public static new User CreateFromDiscriminatorValue(IParseNode parseNode)
+       public static override User CreateFromDiscriminatorValue(IParseNode parseNode)
        {
            _ = parseNode ?? throw new ArgumentNullException(nameof(parseNode));
            return new User();
        }
+       #endif

Thoughts?

@MihaMarkic
Copy link
Contributor Author

First solution - fine with me.
Second (static virtual) - yes, that would work as well (note - there is no override keyword in method implementation). Albeit there will be no code reduction, since you are still implementing CreateFromDiscriminatorValue methods on type (which is required if you want access them through type directly - SomeType.CreateFromDiscriminatorValue, AFAIK). But types without explicit implementation will certainly gain these.

@baywet
Copy link
Member

baywet commented May 6, 2024

Albeit there will be no code reduction, since you are still implementing CreateFromDiscriminatorValue methods on type (which is required if you want access them through type directly - SomeType.CreateFromDiscriminatorValue, AFAIK). But types without explicit implementation will certainly gain these.

I'm a bit confused by this statement, where the second sentence seems to contradict the first one?

@MihaMarkic
Copy link
Contributor Author

I meant that if you implement it like that, there would be no code reduction (assembly size would still be the same or a few bytes larger) though other types will gain the functionality. OTOH all other supported types gain CreateFromDiscriminatorValue though interface, so there is code reduction in that sense. Makes sense?

@baywet
Copy link
Member

baywet commented May 7, 2024

Thanks for clarifying! I'd understand that the method prototype still gets declared in every implementer (IL-wise), but don't you think we'll save at least the method body? (declared once instead of on every type)

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Proposed 💡
Development

No branches or pull requests

2 participants