dotnet / csharplang

The official repo for the design of the C# programming language
10.95k stars 999 forks source link

[API Proposal]: IsNullableAttribute to flow nullability of generic type parameters into methods #7975

Closed mikernet closed 2 months ago

mikernet commented 2 months ago

Background and motivation

There is currently no particularly nice way for methods to know if a reference type generic type parameter is nullable or not. This is desirable in a lot of situations. This new attribute could be applied to a bool parameter with a default value so the compiler provides the value.

What problem would this solve? Let's demonstrate the problem with this API:

class BinaryWriter
{
    public void Write<T>(T value);

    public void WriteList<T>(List<T> list)
    {
        Write(list.Count);

        foreach (var value in list)
        {
            Write(value);
        }
    }
}

We have a problem here if we want Write() to behave differently if T is nullable, i.e. let's say it needs to first write a 1 or 0 to indicate whether a value follows or not. So, to solve this issue given the current state of things, you might expand this to:

class BinaryWriter
{
    public void Write<T>(T value) where T : notnull;

    public void WriteNullable<T>(T? value) where T : class;

    public void WriteNullable<T>(T? value) where T : struct;

    public void WriteList<T>(List<T> list) where T : notnull;
    {
        Write(list.Count);

        foreach (var value in list)
        {
            Write(value);
        }
    }

    public void WriteListWithNullableValues<T>(List<T?> list) where T : struct
    {
        Write(list.Count);

        foreach (var value in list)
        {
            WriteNullable(value);
        }
    }

    public void WriteListWithNullableValues<T>(List<T?> list) where T : class
    {
        Write(list.Count);

        foreach (var value in list)
        {
            WriteNullable(value);
        }
    }
}

Alright, this is getting a bit unwieldy, but this example doesn't seem too bad yet. Things get worse on the reader side of things though, since methods cannot differ just by generic constraints, so there is no way to define the read methods without additional hacky workarounds:

class BinaryReader
{
    public T Read<T>() where T : notnull;

    public T? ReadNullable<T>() where T : class;

    // Cant declare this method without a hack like a dummy unused parameter or
    // giving it a different name
    public T? ReadNullable<T>(int unused = 0) where T : struct;

    public List<T> ReadList<T>() where T : notnull;

    public List<T?> ReadListWithNullableValues<T>() where T : struct;

    // Same hacky unused parameter
    public List<T?> ReadListWithNullableValues<T>(int unused = 0) where T : class
}

Unfortunately, we have now introduced an additional problem: let's say I have a data class that I want to write that can accept an unconstrained T:

class Data<T>
{
    public string? Label { get; set; }

    public List<T> Items { get; } = new();

    public void Write(BinaryWriter writer)
    {
        // need to worry about whether to call Write or WriteNullable here, so usability is affected
        writer.WriteNullable(Label);
        // error: T is not constrained, and we don't know which WriteList method to call even if we could
        writer.WriteList(Items);
    }
}

This is a simplified example of how things can quickly become unusable and a lot of unnecessary complexity starts to develop to handle nullability of generic parameters in some situations.

API Proposal

namespace System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.Parameter)]
public class IsNullableAttribute : Attribute
{
    public string TypeArgumentName { get; }

    public IsNullableAttribute(string typeArgumentName);
}

API Usage

If we could easily flow generic type nullability into methods, then this could be solved as follows:


class BinaryReader
{
    public T Read<T>([IsNullable(nameof(T))] bool nullable = default);

    public List<T> ReadList<T>([IsNullable(nameof(T))] bool nullableValues = default)
    {
        int count = Read<int>();
        var list = new List<T>(count);

        for (int i = 0; i < count; i++)
        {
            list.Add(Read<T>(nullableValues));
        }
    }
}

class BinaryWriter
{
    public void Write<T>(T value, [IsNullable(nameof(T))] bool nullable = default);

    public void WriteList<T>(List<T> list, [IsNullable(nameof(T))] bool nullableValues = default)
    {
        Write(list.Count);

        foreach (var value in list)
        {
            Write(value, nullableValues);
        }
    }
}

class Data<T>
{
    private bool _nullableValues;

    public string? Label { get; set; }

    public List<T> Items { get; } = new();

    public Data([IsNullable(nameof(T))] bool nullableValues = default)
    {
        // we can flow nullability of T using this field
        _nullableValues = nullableValues;
    }

    public void Write(BinaryWriter writer)
    {
        // Label is known to be nullable, so the compiler automatically passes nullable: true
        writer.Write(Label);
        // T is unconstrained, but we can flow the captured nullability from the field
        writer.WriteList(Items, _nullableValues);
    }
}

And now this works as expected:

// Writing: 

var writer = new BinaryWriter();

var list1 = new List<string> { ... };
writer.WriteList(list1); // nullableValues: false

var list2 = new List<string?> { ... };
writer.WriteList(list2);  // nullableValues: true

var data1 = new Data<string>(); // nullableValues: false
data1.Write(writer); // values are written as non-nullable

var data2 = new Data<string?>(); // nullableValues: true
data2.Write(writer); // values are written as nullable
// Reading:

var reader = new BinaryReader();

string value1 = reader.Read<string>(); // nullable: false
string? value2 = reader.Read<string?>(); // nullable: true

List<string> list1 = reader.ReadList<string>();  // nullableValues: false
List<string?> list2 = reader.ReadList<string?>(); // nullableValues: true

Alternative Designs

No response

Risks

No response

huoyaoyuan commented 2 months ago

Making api behavior varies with nullable annotation isn't really a good idea.

See DI container for an example: GetService<T> returns nullable. GetRequiredService<T> returns non-nullable and throws for not found. The two different behaviors are distinguished by different methods.

mikernet commented 2 months ago

@huoyaoyuan The problems with having different methods in cases like this was explained above.

vcsjones commented 2 months ago

At the very least what you are proposing is a language change. A new attribute by itself won't do anything.

Language changes and suggestions should start at https://github.com/dotnet/csharplang/discussions/new/choose

mikernet commented 2 months ago

@vcsjones Yes that's a good point. If someone wants to move it, that would be swell.