Clarify usage of IUnionMembers interface

[BarionLP]

Summary

clarify that all union members must be part of the interface
so far the paragraph mostly talks about factory methods.

---

Internal previews

File	Preview link
docs/csharp/language-reference/builtin-types/snippets/unions/MemberProvider.cs	docs/csharp/language-reference/builtin-types/snippets/unions/MemberProvider.cs
docs/csharp/language-reference/builtin-types/union.md	docs/csharp/language-reference/builtin-types/union

[BillWagner]

Hi @BarionLP

This is a good start. I'd like to see the example updated to include the additional members. So, instead of:

[System.Runtime.CompilerServices.Union]
public record class Outcome<T> : Outcome<T>.IUnionMembers
{
    private readonly object? _value;

    private Outcome(object? value) => _value = value;

    public interface IUnionMembers
    {
        static Outcome<T> Create(T? value) => new(value);
        static Outcome<T> Create(Exception? value) => new(value);
        object? Value { get; }
    }

    object? IUnionMembers.Value => _value;
}

The sample should include:

[System.Runtime.CompilerServices.Union]
public record class Outcome<T> : Outcome<T>.IUnionMembers
{
    private readonly object? _value;

    private Outcome(object? value) => _value = value;

    public interface IUnionMembers
    {
        static Outcome<T> Create(T? value) => new(value);
        static Outcome<T> Create(Exception? value) => new(value);
        object? Value { get; }

        bool TryGetValue(out T value);
        bool TryGetValue(out Exception value);
    }

    object? IUnionMembers.Value => _value;
}

Can you add that as well?

[BarionLP]

@BillWagner while changing this i noticed the current example has a bug.
static Outcome<T> Create(T? value) => new(value); resolves to the copy constructor generated by the record resulting in a recursive Create invocation: static Outcome<T> Create([Nullable(2)] T value) => new Outcome<T>(Create(value));
the easiest fix would be to specify the argument name but that is probably confusing new(value: value).
How could this be fixed properly?

[BillWagner]

While changing this i noticed the current example has a bug. static Outcome<T> Create(T? value) => new(value); resolves to the copy constructor generated by the record resulting in a recursive Create invocation: static Outcome<T> Create([Nullable(2)] T value) => new Outcome<T>(Create(value)); the easiest fix would be to specify the argument name but that is probably confusing new(value: value). How could this be fixed properly?

@BarionLP This is interesting. I want to loop in @333fred and @AlekseyTs on this part. I had thought making the record type sealed would change the lowered code, but it didn't. On the other hand, I'm concerned about the named parameter is that it could introduce a subtle bug when the Create didn't get called for an object derived from T.

Everything looks good, but I'd like to get their thoughts before we finalize and merge this.

[AlekseyTs]

I suggest including explicit cast to (object?):

        static Outcome<T> Create(T? value) => new((object?)value);
        static Outcome<T> Create(Exception? value) => new((object?)value);

And the type should implement TryGetValue methods added to the interface

[BarionLP]

I suggest including explicit cast to (object?):

        static Outcome<T> Create(T? value) => new((object?)value);
        static Outcome<T> Create(Exception? value) => new((object?)value);

will do
generally, given the current example is the most straight forward way to do this, it could be a common problem people might have, maybe there should there be an analyzer or something to warn people about it or completely prevent it

[AlekseyTs]

Perhaps the example shouldn't use record. I think this is an unnecessary mix of features. A union doesn't need to be a record.

[BarionLP]

Perhaps the example shouldn't use record.

i turned it into a struct (which is what i would do in this case and most union use cases i think)
or do you think it should be a plain class?

[BillWagner]

Perhaps the example shouldn't use record.

i turned it into a struct (which is what i would do in this case and most union use cases i think) or do you think it should be a plain class?

Thanks @BarionLP
I gave this a final look, and I'll now.

[BillWagner]

Thanks for working through this with all of us @BarionLP

I'll now. You should see the changes on the live site in the next day or so, on our regular publishing cycle.