NullabilityInfoContext reports null/notnull for unspecified generic parameters

[madelson]

Description

For a generic parameter T on a class with no type constraints indicating nullability, Unknown would seem most appropriate nullability state to return from NullabilityInfoContext because there are no constraints on that type with respect to nullability.

However, the value returned seems to depend on whether the type is declared in a nullable or non-nullable context

Reproduction Steps

public static void Main()
    {
        var context = new NullabilityInfoContext();

        var fooInfo = context.Create(typeof(Foo<string?>).GetConstructors().Single().GetParameters().Single());
        Console.WriteLine(fooInfo.WriteState); // NotNull (I would expect Unknown; NotNull is clearly wrong here given that I can make Foo<string?>)

        var barInfo = context.Create(typeof(Bar<string?>).GetConstructors().Single().GetParameters().Single());
        Console.WriteLine(barInfo.WriteState); // Unknown
    }

#nullable disable
    private class Foo<T> { public Foo(T t) {  } }
#nullable enable

#nullable disable
    private class Bar<T> { public Bar(T t) { } }
#nullable enable

Expected behavior

Consistently returns Unknown.

Actual behavior

Sometimes returns NotNull.

Regression?

No response

Known Workarounds

No response

Configuration

VS 17.0.2, .NET 6, Windows 10 x64.

I don't have any reason to believe this is configuration specific.

Other information

C#9 allows for both T and T? to be used in generic classes even without any constraint that indicates whether or not T can be null:

class Generic<T> { public void Foo(T a, T? b) { } }

It's unclear what NullabilityInfoContext should return when reflecting over such types. On one hand, both could be either non-nullable or nullable depending on the particular generic argument that ends up getting used, which makes Unknown seem reasonable for both from that perspective.

On the other hand, it seems nice to be able to differentiate between the two in some way. Perhaps it makes sense to add another null state that explicitly represents the case of NotNullIfGenericArgumentIsNonNullableReferenceType?

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

For a generic parameter T on a class with no type constraints indicating nullability, Unknown would seem most appropriate nullability state to return from NullabilityInfoContext because there are no constraints on that type with respect to nullability.

However, the value returned seems to depend on whether the type is declared in a nullable or non-nullable context

Reproduction Steps

public static void Main()
    {
        var context = new NullabilityInfoContext();

        var fooInfo = context.Create(typeof(Foo<string?>).GetConstructors().Single().GetParameters().Single());
        Console.WriteLine(fooInfo.WriteState); // NotNull (I would expect Unknown; NotNull is clearly wrong here given that I can make Foo<string?>)

        var barInfo = context.Create(typeof(Bar<string?>).GetConstructors().Single().GetParameters().Single());
        Console.WriteLine(barInfo.WriteState); // Unknown
    }

#nullable disable
    private class Foo<T> { public Foo(T t) {  } }
#nullable enable

#nullable disable
    private class Bar<T> { public Bar(T t) { } }
#nullable enable

Expected behavior

Consistently returns Unknown.

Actual behavior

Sometimes returns NotNull.

Regression?

No response

Known Workarounds

No response

Configuration

VS 17.0.2, .NET 6, Windows 10 x64.

I don't have any reason to believe this is configuration specific.

Other information

C#9 allows for both T and T? to be used in generic classes even without any constraint that indicates whether or not T can be null:

class Generic<T> { public void Foo(T a, T? b) { } }

It's unclear what NullabilityInfoContext should return when reflecting over such types. On one hand, both could be either non-nullable or nullable depending on the particular generic argument that ends up getting used, which makes Unknown seem reasonable for both from that perspective.

On the other hand, it seems nice to be able to differentiate between the two in some way. Perhaps it makes sense to add another null state that explicitly represents the case of NotNullIfGenericArgumentIsNonNullableReferenceType?

Author:	madelson
Assignees:	-
Labels:	area-System.Reflection, untriaged
Milestone:	-

[Shane32]

Somewhat related issue here - see below PR just to fix Tuple<Tuple<string?, string?>, string> to work right:

• Fix NullabilityInfoContext.Create to properly analyze types with nested generic types #63556

[Shane32]

As it relates to this issue, it appears that code was added to inspect the generic type (e.g. perhaps it was a closed type), and return the correct nullability status. But the code is so broken that the value is completely useless. It would be more useful if it did not do any inspection and simply returned the nullability flag of T given a method Foo<T>(). (meaning, if it was declared T or T?) That information is readily available with no deep inspection required.

I have not spent the time to write a fix and put a PR here (besides the small fix above), but I have written a great many tests that demonstrate the wide variety of ways that T could be declared and what the code must account for. (And hence the great many situations in which it currently fails.) Either that, or it should completely ignore what T is and return the nullability flag declared on the member.

See:

• https://github.com/Shane32/NullabilityInfo/tree/with_net6_code

[Shane32]

On one hand, both could be either non-nullable or nullable depending on the particular generic argument that ends up getting used, which makes Unknown seem reasonable for both from that perspective.

I agree. I would say that "Unknown" should be returned for T when T is a unrestricted generic type, as it could be a nullable reference type. But "Not-Null" should be returned for T when T is restricted to notnull, a non-null reference type, or a struct type. And "Nullable" should be returned for T? regardless. Oh, and if it's a closed generic, then the proper nullability status should be returned. All of the above is possible with reflection for public members/types, and for private members/types when the necessary attributes are not suppressed by the compiler. I believe this would include closed generic types generated at compile time (like List<int>), but not closed generic types generated at runtime. But I have not tested that yet. At least the code should provide what information it can.

[buyaa-n]

C#9 allows for both T and T? to be used in generic classes even without any constraint that indicates whether or not T can be null:

class Generic<T> { public void Foo(T a, T? b) { } }

It's unclear what NullabilityInfoContext should return when reflecting over such types. On one hand, both could be either non-nullable or nullable depending on the particular generic argument that ends up getting used, which makes Unknown seem reasonable for both from that perspective.

T can be nullable in case refr type used for the generic argument, will be non null if value type used, but T? will always be nullable even for value type.

Therefore the nullability result of T? always will be Nullable for any concrete type used as type parameter, but for T it depened from:

1. if nullability context disabled Unknown for ref types
2. if nullability context enabled and if a ref type used as type parameter (or open generic) it is Nullable and NotNull for value type

With that being said i see Foo(T t) is returning NotNull when nullability enabled and string type is used as type parameter, that is a bug, it should be Nullable by current design

On the other hand, it seems nice to be able to differentiate between the two in some way. Perhaps it makes sense to add another null state that explicitly represents the case of NotNullIfGenericArgumentIsNonNullableReferenceType?

That is interesting idea

[buyaa-n]

As it relates to this issue, it appears that code was added to inspect the generic type (e.g. perhaps it was a closed type), and return the correct nullability status. But the code is so broken that the value is completely useless. It would be more useful if it did not do any inspection and simply returned the nullability flag of T given a method Foo(). (meaning, if it was declared T or T?) That information is readily available with no deep inspection required.

We are doing that inspection exactly for returning the nullability flag of T given a method Foo(). (meaning, if it was declared T or T?) , but that information is not readily available, only available if it is open generic. When Foo<string?> is used we would see only Foo<string>, the compiler would not retain the ?, therefore we inspect to the original generic type declaration to get the nullability info. Though I agree that that inspection has bugs need to be fixed

[Shane32]

Thanks for the input. I will update my test project to match your description of C# semantics and internals.

[madelson]

Based on https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references, there is zero difference between T and T? in unrestricted generic types except in the case that T is bound to a non-nullable reference type:

Type	T	T?
int	non-nullable	non-nullable
int?	nullable	nullable
object	non-nullable	nullable
object?	nullable	nullable

That's why I feel it is misleading to return either NotNull or Nullable when examining a generic parameter of type T without further information.

Curious whether we agree on what NullabilityInfoContext should return in these different cases:

// classes
#nullable disable
class ObliviousGeneric<T> { T A; }
#nullable enable
class Generic<T> { T B; T? C; }
class HasGeneric { ObliviousGeneric<object> D; ObliviousGeneric<object?> E; Generic<object> F; Generic<object?> G; }
class DerivesFromObliviousNonNullGeneric : ObliviousGeneric<object> { }
class DerivesFromObliviousNullGeneric : ObliviousGeneric<object?> { }
class DerivesFromNonNullGeneric : Generic<object> { }
class DerivesFromNullGeneric : Generic<object?> { }
class NotNullGeneric<T> where T : notnull { T I; T? J; }
class ClassGeneric<T> where T : class { T K; T? L; }
class NullClassGeneric<T> where T : class? { T M; T? N; }
class StructGeneric<T> where T : struct { T O; T? P; }
class NotNullGenericDerivesFromObliviousGeneric<T> : ObliviousGeneric<T> where T : notnull { }
class DerivesFromNotNullGenericDerivesFromObliviousGeneric : NotNullGenericDerivesFromObliviousGeneric<object> { }
class ClassRestrictedGeneric<T> where T : Class1 { T K; T? L; }
class NullClassRestrictedGeneric<T> where T : Class1? { T M; T? N; }
class Class1 { }

Case	Desired result	Notes
typeof(ObliviousGeneric<>).A	Unknown
typeof(ObliviousGeneric<object>).A	Unknown	It would be nice if this could return NotNull, but we won't have any metadata to determine that
typeof(Generic<>).B	Unknown	I could also see an argument for the more nuanced NotNullIfGenericArgumentIsNonNullableReferenceType here
typeof(Generic<>).C	Unknown	I could also see an argument for the more nuanced NullableIfGenericArgumentIsNonNullableReferenceType here
typeof(HasGeneric).D (generic parameter)	NotNull
typeof(HashGeneric).E (generic parameter)	Nullable
typeof(HasGeneric).F (generic parameter)	NotNull
typeof(HashGeneric).G (generic parameter)	Nullable
typeof(DerivesFromObliviousNonNullGeneric).A	NotNull	Metadata available on DerivesFromObliviousNonNullGeneric
typeof(DerivesFromObliviousNullGeneric ).A	Nullable	Metadata available on DerivesFromObliviousNullGeneric
typeof(DerivesFromNonNullGeneric).B	NotNull	Metadata available on DerivesFromNonNullGeneric
typeof(DerivesFromNonNullGeneric).C	Nullable	Metadata available on DerivesFromNonNullGeneric
typeof(DerivesFromNullGeneric).B	NotNull	Metadata available on DerivesFromNullGeneric
typeof(DerivesFromNullGeneric).C	Nullable	Metadata available on DerivesFromNullGeneric
typeof(NotNullGeneric<>).I	NotNull
typeof(NotNullGeneric<>).J	Unknown	Could be NullableIfGenericArgumentIsNonNullableReferenceType. If T ends up being int then J's type will be int not int? so Nullable feels inaccurrate
typeof(NotNullGeneric<object>).I	NotNull
typeof(NotNullGeneric<object>).J	Nullable
typeof(NotNullGeneric<int>).I	NotNull
typeof(NotNullGeneric<int>).J	NotNull
typeof(ClassGeneric<>).K	NotNull
typeof(ClassGeneric<>).L	Nullable
typeof(NullClassGeneric<>).M	Unknown	Could be NotNullIfGenericArgumentIsNonNullableReferenceType
typeof(NullClassGeneric<>).N	Nullable
typeof(StructGeneric<>).O	NotNull
typeof(StructGeneric<>).P	Nullable
typeof(NotNullGenericDerivesFromObliviousGeneric<>).A	NotNull
typeof(DerivesFromNotNullGenericDerivesFromObliviousGeneric).A	NotNull
typeof(ClassRestrictedGeneric<>).K	NotNull	Unclear where the metadata is
typeof(ClassRestrictedGeneric<>).L	Nullable	Unclear where the metadata is
typeof(NullClassRestrictedGeneric<>).M	Unknown	Could be NotNullIfGenericArgumentIsNonNullableReferenceType
typeof(NullClassRestrictedGeneric<>).N	Nullable	Unclear where the metadata is

[Shane32]

@buyaa-n If you can review and confirm the list provided above by @madelson , it will give us a chance to test the existing functionality, and if necessary, write a pull request that fixes it.

[madelson]

@Shane32 good catch! I've updated my list and added a few more cases too.

From poking around the NullabilityInfoContext code, I think the root cause(s) are that:

• We never inspect the ReflectedType of the passed in parameter/member which would allow us to leverage information layered on by derived types
• We seem to override nullability info based on nullability info from generic parameters, whereas I think that generic parameter nullability should act as more of a constraining factor layered on top (even if concepts like NotNullIfGenericArgumentIsNonNullableReferenceType aren't exposed via the API, I think we may need them internally while building up to the answer).

[Shane32]

On the other hand, it seems nice to be able to differentiate between the two in some way. Perhaps it makes sense to add another null state that explicitly represents the case of NotNullIfGenericArgumentIsNonNullableReferenceType?

May be useful internally even if not exposed so as to not change or complicate the existing API. The use case isn't likely to be writing a new code editor, but rather simple tasks that just needs a yes/no/unknown answer. Yet, I'm not against extending the API either with a few more enums. (We could hardly say it was a breaking change considering that the existing API does not return reliable results anyway.)

[buyaa-n]

@madelson's first table looks correct, i was mistaken about the int for T?

For the other table mainly looks good, i pasted below the diff between your table and the current result, looks main difference i prefer Nullable than Unknown 😆

Case	Current result	Notes
typeof(Generic<>).B	Nullable because of the compiler generated NullableAttribute	agree that NotNullIfGenericArgumentIsNonNullableReferenceType would be better
typeof(Generic<>).C	Nullable	don't agree with NullableIfGenericArgumentIsNonNullableReferenceType part for T?
typeof(NotNullGeneric<>).J	Nullable	For avoiding NRE Nullable is accurate here, it can be notnull for int as you said but Nullability analysis is more about ref types i would say
typeof(NullClassGeneric<>).M	Nullable	should be nullable until NotNullIfGenericArgumentIsNonNullableReferenceType added

@buyaa-n If you can review and confirm the list provided above by @madelson , it will give us a chance to test the existing functionality, and if necessary, write a pull request that fixes it.

That would be great, thanks @Shane32 !

[Shane32]

Can we add these conditions and results also in the list @madelson ?

class ClassRestrictedGeneric<T> where T : Class1 { T K; T? L; }
class NullClassRestrictedGeneric<T> where T : Class1? { T M; T? N; }
class Class1 { }

They should match the results of these existing samples:

class ClassGeneric<T> where T : class { T K; T? L; }
class NullClassGeneric<T> where T : class? { T M; T? N; }

However, my experimentation shows that the compiler is incorrectly applying an 'unknown' flag in the NullableAttribute to the type constraints of both, rather than applying non-null and nullable, respectively, as it does with class and class?. The compiler refuses to allow a class constraint on top of the Class1, because it infers the nullability from the Class1 (vs Class1?). This can be seen via intellisense, etc. But using reflection it would seem that the attribute is not saved correctly. So this may be a bug with the compiler rather than with this interpretation class. Can you verify this @buyaa-n ? I'm really not sure I'm diagnosing this correctly. I can provide a code sample, open a new issue, etc, as you direct.

[madelson]

@Shane32 I agree that this case is weird. The compiler does successfully warn on new ClassRestrictedGeneric<Class1?>() and not on new NullClassRestrictedGeneric<Class1?>(), but I can't find the attribute metadata that it's using.

In contrast, for NullClassGeneric<T> I see [NullableContext(2), Nullable(0)] and for ClassGeneric<T> I see [NullableContext(2), Nullable(0)].

I've added these to the table.

[Shane32]

It’s an attribute under the generic type parameter’s type definition. But it uses the context of the parent type. Took me a while to find it also.

[Shane32]

But it only is being written correctly for “class” or “class?” - I think

[madelson]

@Shane32 so you can find the metadata to differentiate : class and : class? but you cannot find the metadata to differentiate : Class1 vs : Class1?, is that correct?

[Shane32]

Well, I can find it, but it’s set to 0 (unknown). I’ll post some code.

[Shane32]

var aType = typeof(A<>);
var a = aType.GetCustomAttributes(); // it uses the NullableContext from here
var aConstraint = aType.GetGenericArguments()[0].GetCustomAttributes(); // and here is where the type constraint nullability is stored
public class A<B> where B : class { }

Add a few more members to force the context to be either nullable or non-nullable, and then experiment with the type constraint. Of course value types would be known to be a value type, but the proper nullability byte should be set for reference types.

[Shane32]

Anyway, maybe I'm missing something, and there's actually somewhere else it's being stored also. @buyaa-n can you review?

[buyaa-n]

Well, I can find it, but it’s set to 0 (unknown). I’ll post some code.
Anyway, maybe I'm missing something, and there's actually somewhere else it's being stored also. @buyaa-n can you review?

Thanks for the examples @Shane32, unfortunately i did not found any attributes anywhere for distinguishing : Class1 or : Class1?, even don't see the one with value of 0 (unknown)

So this may be a bug with the compiler rather than with this interpretation class. Can you verify this @buyaa-n ? I'm really not sure I'm diagnosing this correctly. I can provide a code sample, open a new issue, etc, as you direct.

Agree it could be a compiler bug, you can file an issue in roslyn repo to find out for sure, i don't see any info for related to it in their doc, it is unfortunate that we could not give better answer than Unknown in this case

[buyaa-n]

@madelson it seems all of the issues mentioned here fixed by your PR except the : Class1 or : Class1? case. Is there any unresolved scenario left?

class ClassRestrictedGeneric<T> where T : Class1 { T K; T? L; }
class NullClassRestrictedGeneric<T> where T : Class1? { T M; T? N; }
class Class1 { }

[Shane32]

FYI, when I went back to this I couldn't reproduce the issue; I'm not sure why. (It seemed pretty clear at the time.) Can anyone else reproduce the issue with that particular case? I have not checked recently.

[madelson]

@buyaa-n isnt the base issue here around unrestricted generic parameters (the original bug report at the top of this thread) still unresolved?

I think you maybe felt strongly that the current behavior should stay as is which is why I didn’t attempt to address it. Maybe this gets closed as by design?

[buyaa-n]

FYI, when I went back to this I couldn't reproduce the issue; I'm not sure why. (It seemed pretty clear at the time.)Can anyone else reproduce the issue with that particular case? I have not checked recently.

Strange, i could not repro either, now seems correct NullableAttribute values emmitted by the compiler

@buyaa-n isnt the base issue here around unrestricted generic parameters (the original bug report at the top of this thread) still unresolved?

Seems you were proposing to return Unknown for unrestricted generic parameters. When nullability context is enabled we should return something better than unknown, i.e it should return nullable for ref type and not null for value type. There were some bugs around that, but i beleive all of them fixed by your PR now.

I think you maybe felt strongly that the current behavior should stay as is which is why I didn’t attempt to address it. Maybe this gets closed as by design?

Yes i feel current behavior should stay, thanks for clarification @madelson, closing the issue as by design