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

Consider representing nullable flow state as: MaybeNull, NotNull, or T #38638

Closed
cston opened this issue Sep 11, 2019 · 5 comments · Fixed by #39778
Closed

Consider representing nullable flow state as: MaybeNull, NotNull, or T #38638

cston opened this issue Sep 11, 2019 · 5 comments · Fixed by #39778

Comments

@cston
Copy link
Member

cston commented Sep 11, 2019

Consider adding a distinct nullable flow state, T, to allow distinguishing "maybe null when the type allows null" from "maybe null even when type disallows null".

internal enum NullableFlowState : byte
{
    NotNull,  // not null
    T,        // maybe null when type allows
    MaybeNull // maybe null even when type disallows
}
@cston cston added this to the 16.4 milestone Sep 11, 2019
@cston cston self-assigned this Sep 11, 2019
@cston cston changed the title Consider representing nullable as: MaybeNull, NotNull, or T Consider representing nullable flow state as: MaybeNull, NotNull, or T Sep 11, 2019
@jcouv
Copy link
Member

jcouv commented Sep 12, 2019

FYI for @jasonmalinowski

@jcouv
Copy link
Member

jcouv commented Sep 27, 2019

Even if we just add the third state, without adding a corresponding type/annotation for locals, some scenarios would be improved.
Here's one reported by Sam:

#nullable enable

using System;
using System.Diagnostics.CodeAnalysis;

public class C<T> {
    [MaybeNull] T value;
    public void M() {
        if (value is null)
            return;
    }
}

@saucecontrol
Copy link
Member

This example just started giving me warnings in VS 16.4 Preview 2

[return: MaybeNull]
public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dic, TKey key, TValue defaultValue = default) where TKey : notnull =>
    dic.TryGetValue(key, out var value) ? value : defaultValue;

Results in warning CS8717: A member returning a [MaybeNull] value introduces a null value when 'TValue' is a non-nullable reference type

Would this allow the [MaybeNull] from TryGetValue to flow through?

@jnm2
Copy link
Contributor

jnm2 commented Oct 16, 2019

CS8717 feels subpar to me. The warning should follow the (mis)usage, not the declaration. As it stands, CS8717 only shows up on valid (even ideal) code.

void M<T>()
{
    var x = new Dictionary<int, T>();

    // CS8717 A member returning a [MaybeNull] value introduces a null value when 'TResult' is a
    // non-nullable reference type.
    //                        ↓
    if (x.TryGetValue(42, out var someValue))
    {
    }
}

@CyrusNajmabadi
Copy link
Member

FYI, this is super unpleasant. I honestly don't know how to write code in some cases that is actually NRT correct. I thought MaybeNull was intended for this, but @jcouv informs me that for the actual uninstantiated cases (i.e. while you're writing generic code), you're just SOL here :-/

davkean added a commit to davkean/project-system that referenced this issue Oct 29, 2019
This suppresses:  A member returning a [MaybeNull] value introduces a null value when 'T' is a non-nullable reference type, which is unavoidable with compiler work to track state more accurately. This is being tracked by:

dotnet/roslyn#38638
@agocke agocke modified the milestones: 16.4, 16.5 Nov 1, 2019
mavasani added a commit to dotnet/roslyn-analyzers that referenced this issue Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants