eiriktsarpalis / PolyType

Practical generic programming for C#
https://eiriktsarpalis.github.io/PolyType/
MIT License
145 stars 6 forks source link

`[GenerateShape<string>]` causes public ctor and static `Default` member to be generated #48

Open AArnott opened 2 days ago

AArnott commented 2 days ago

I have a type that starts with this:

[GenerateShape<string>]
internal partial class MemoFormatter : MessagePackConverter<Memo>
{
    public static readonly MemoFormatter Instance = new();

    private MemoFormatter()
    {
    }

The mere presence of the [GenerateShape<string>] attribute causes this to be source generated:

internal partial class MemoFormatter
{
    private const global::System.Reflection.BindingFlags InstanceBindingFlags = 
        global::System.Reflection.BindingFlags.Public | 
        global::System.Reflection.BindingFlags.NonPublic | 
        global::System.Reflection.BindingFlags.Instance;

    /// <summary>Gets the default instance of the <see cref="MemoFormatter"/> class.</summary>
    public static MemoFormatter Default { get; } = new();

    /// <summary>Initializes a new instance of the <see cref="MemoFormatter"/> class.</summary>
    public MemoFormatter() { }
}

I already define a (private) default constructor, and I don't want a public static Default member either. Why is MemoFormatter having its API added to in this way?

eiriktsarpalis commented 1 day ago

This is closely related to https://github.com/eiriktsarpalis/PolyType/issues/47. The current generation strategy augments witness types with a full-blown implementation of the underlying ITypeShapeProvider. We could try changing this so that witnesses only receive explicit interface implementations that are stubs pointing to a compilation-wide provider.

One open question is how we source generate for netstandard2.0, assuming we do add support for that target in the future. With IShapeable<T> and static abstracts not being available, the source generator would need to generate something else in its place.

AArnott commented 1 day ago

We could try changing this so that witnesses only receive explicit interface implementations that are stubs pointing to a compilation-wide provider.

True. I don't mind knocking out two bugs at once. But if the other one is far more expensive to fix, you could always just emit a private nested class and make that one instantiable instead of the one that has the attribute on it.

I'll start thinking about the netstandard2.0 problem...

AArnott commented 1 day ago

And at the very least, can these members be emitted as internal rather than subtly contributing public API to the library? The static shapeable method should still be able to access them, right?

eiriktsarpalis commented 1 day ago

Again, this is just emulating the generation strategy used by JsonSerializerContext. The public properties provide a strongly typed ITypeShape<T> instance in a way that predates the introduction of IShapeable<T>. It may or may not be necessary to keep for netstandard2.0 depending on what pattern we decide to use there.