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

[MaybeNull]: Cannot implement a generic method with a "default" return value with nullable references #30953

Open
ssg opened this issue Nov 4, 2018 · 13 comments

Comments

@ssg
Copy link

@ssg ssg commented Nov 4, 2018

I'm trying to implement methods that return an optional of a given type T but get a compiler error. Changing return type to T? only changes the error.

I understand why compiler provides that error, it makes sense, but I have no idea how I can get around it in the world of nullable references. Should I change the design of such methods?

Version Used:
Roslyn_Nullable_References_Preview_09112018

Steps to Reproduce:

        public static T FirstOrDefault<T>(this IList<T> list)
        {
            if (list.Count == 0)
            {
                return default; // CS8625
            }
            return list[0];
        }

Expected Behavior:
The code compiles fine without nullable references. I'd expect it to work the same.

Actual Behavior:
Compiler error CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter.

@jcouv

This comment has been minimized.

Copy link
Member

@jcouv jcouv commented Nov 5, 2018

Thanks. This is a known issue.

T? is currently only valid when T is constrained to be a reference type.
For unconstrained type parameters, the current plan from LDM is to use an attribute, [MaybeNull], to indicate that the value may be null when the type is a reference type.
But there are still language design issues with this: T local = list.FirstOrDefault(); would still produce a warning and attributes are not allowed on local declarations...

Tagging @cston since he was looking into this recently.

@jcouv jcouv removed their assignment Dec 9, 2018
@jcouv jcouv added this to the 16.0.P2 milestone Dec 9, 2018
@jcouv jcouv modified the milestones: 16.0.P2, 16.0.P3 Jan 11, 2019
@jaredpar jaredpar added this to Needs Design in Nullable Board Jan 28, 2019
@roji

This comment has been minimized.

Copy link
Member

@roji roji commented Jan 31, 2019

+1 on this, while working on annotating the Entity Framework Core codebase, I'm running into this quite a lot.

@jcouv jcouv changed the title Cannot implement a generic method with a "default" return value with nullable references [MaybeNull]: Cannot implement a generic method with a "default" return value with nullable references Jan 31, 2019
@ssg

This comment has been minimized.

Copy link
Author

@ssg ssg commented Feb 1, 2019

This workaround doesn't compile either (Compiler assumes they correspond to the same type signature, although the type constraints don't overlap):

    Foo<T>() where T : struct => default;
    T? Foo<T>() where T : class => null;
@roji

This comment has been minimized.

Copy link
Member

@roji roji commented Feb 1, 2019

@ssg generic type constraints aren't part of the method signature, so this can't work (see dotnet/csharplang#2013).

@sharwell

This comment has been minimized.

Copy link
Member

@sharwell sharwell commented May 31, 2019

For unconstrained type parameters, the current plan from LDM is to use an attribute, [MaybeNull], to indicate that the value may be null when the type is a reference type.

Why have an attribute when T? literally means "maybe null"? The new language syntax is semantically identical to the attribute, and does not have limitations on placement.

@roji

This comment has been minimized.

Copy link
Member

@roji roji commented May 31, 2019

@sharwell T? is very different from default. In the case of value types, T? means Nullable<T> which is a completely different type from T, whereas default just means the default value (e.g. 0 for int). The idea here is to allow writing generic methods which return nullable reference types (e.g. string?) when returning references, and regular value types when returning values - this is currently not possible.

See also dotnet/csharplang#2194 which is about allowing generic methods to return T? (which would mean Nullable<T> for value types and a nullable reference type for reference types).

@sharwell

This comment has been minimized.

Copy link
Member

@sharwell sharwell commented May 31, 2019

@roji I am not suggesting that T? ever be Nullable<T> if it starts out as an unconstrained generic type. I don't see much value in such a proposal for the following reasons:

  1. The described coding pattern cannot be implemented in C# today
  2. The described coding pattern cannot be directly lowered to CLR 4 capabilities, so either the implementation creates multiple methods with special names that get hidden or it needs CLR changes

On the other hand, FirstOrDefault<T> is a coding pattern already used frequently today. The interpretation of T? for unconstrained generic types matches the behavior already used in all existing code cases.

@roji

This comment has been minimized.

Copy link
Member

@roji roji commented May 31, 2019

As I wrote in dotnet/csharplang#2194, it seems extremely problematic to change the meaning of T? to mean anything other than Nullable<T> for value types, in any context.

Methods like FirstOrDefault<T> could be simply be annotated with the [MaybeNull] proposed here for the same effect.

@ssg

This comment has been minimized.

Copy link
Author

@ssg ssg commented May 31, 2019

For whatever it's worth, I was able to implement this without any compiler errors on .NET Core 3.0 preview 5 and it seems to be working fine (except for non-nullable references, of course):

    static class A
    {
        public static T FirstOrDefault<T>(this IList<T> list) where T: struct
        {
            if (list.Count == 0)
            {
                return default;
            }
            return list[0];
        }
    }

    static class B
    {
        public static T? FirstOrDefault<T>(this IList<T?> list) where T: class
        {
            if (list.Count == 0)
            {
                return default;
            }
            return list[0];
        }
    }

When you use it, it uses the correct overload depending on the type of the list item.

@YohDeadfall

This comment has been minimized.

Copy link

@YohDeadfall YohDeadfall commented Jun 1, 2019

@ssg The thing you are asking (an attribute) already implemented, but only for reference types. See dotnet/csharplang#2194 (comment).

@jcouv Any reason to add another one attribute when [Nullable] exists and which has the same meaning if I understand it correctly?

@canton7

This comment has been minimized.

Copy link
Contributor

@canton7 canton7 commented Jun 1, 2019

@YohDeadfall The LDM writeup is here. It's part of a wider suite of attributes.

@YohDeadfall

This comment has been minimized.

Copy link

@YohDeadfall YohDeadfall commented Jun 2, 2019

Oh, thank you! It looks like I missed a lot of things going on... So it looks like Standard Annotation Language which Microsoft uses in its products written in C/C++. It really helps to find bugs, and it would be great to see something in C#.

While writing the comment and doing some tests I found the idea of MaybeNullAttribute and why it's required. The compiler perfectly deduce the return type in the following case and shows a warning in the appropriate place:

public static T Get<T>(T value)
{
    return value;
}

// this is how the compiler applies attributes
[return: Nullable(1)]
public static T Get<[Nullable(2)] T>([Nullable(1)] T value)
{
    return value;
}
string? v = null;
string? r = Get(v);
string? v = null;
string r = Get(v); // warning CS8600: Converting null literal or possible null value to non-nullable type.
string v2 = "value";
string r2 = Get(v2);

In the case which @ssg provided, the return type should be a nullable reference, but it isn't marked as annotated nullable. But could the compiler add MayBeNullAttribute automatically in such cases? It can detect the default expression in the method body, or even more the return type can be deduced based on deeper code analysis. For example:

public static T GetDefault<T>()
{
    return default;
}

// will produce

[return: MaybeNull]
public static T GetDefault<[Nullable(2)] T>()
{
    return default; // because of this line
}
public static T GetDefault<T>()
{
    return GetDefaultInternal<T>();
}

private static T GetDefaultInternal<T>()
{
    return default;
}

// will produce

[return: MaybeNull]
public static T GetDefault<[Nullable(2)] T>()
{
    return GetDefaultInternal<T>(); // because GetDefaultInternal may return null
}

[return: MaybeNull]
private static T GetDefaultInternal<[Nullable(2)] T>()
{
    return default; // because of this line
}

This could be a great step forward since the compiler already deduce the right return type in the first example, so this sound like a continuation. Otherwise, @roji's proposal can be syntactic sugar for MaybeNullAttribute.

@jcouv

This comment has been minimized.

Copy link
Member

@jcouv jcouv commented Jul 8, 2019

Our current plan is for nullability attributes like [return: MaybeNull] not to affect method bodies. So return default; would still warn for now. We will look at refining this post C# 8 RTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.