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

NullabilityInfoContext does not report the correct nullability for method in generic base class. #63555

Closed
madelson opened this issue Jan 9, 2022 · 8 comments · Fixed by #64143

Comments

@madelson
Copy link
Contributor

madelson commented Jan 9, 2022

Description

Let's say I have the following class defined in nullable enabled context:

private class NonNullableValuesDictionary : Dictionary<string, object> { }

If I try to add a null value to the object, I get a compiler error:

var dictionary = new NonNullableValuesDictionary();
dictionary.Add("a", null); // CS Cannot convert null literal to non-nullable reference type

However, I can't replicate this compiler behavior with NullabilityInfoContext even though it seems to me that the compiler must be reading the same metadata in order to issue that warning.

Reproduction Steps

public static void Main()
{
    var context = new NullabilityInfoContext();
    var addMethod = typeof(NonNullableValuesDictionary).GetMethod("Add")!;
    var info = context.Create(addMethod.GetParameters()[1]);
    Console.WriteLine(info.WriteState); // Nullable
}

private class NonNullableValuesDictionary : Dictionary<string, object> { }

Expected behavior

I would expect the code above to print "NotNull" instead.

Actual behavior

Prints "Nullable".

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

Some other fun cases:

class DerivedFromNonNullableValuesDictionary : NonNullableValuesDictionary { }
class GenericDerivedFromDerivedFromNonNullableValuesDictionary<T> : DerivedFromNonNullableValuesDictionary { }

class GenericDerivedFromDictionary<T, U> : Dictionary<T, T> { }
class GenericDerivedFromGenericDerivedFromDictionary<U> : GenericDerivedFromDictionary<object> { }
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Jan 9, 2022
@ghost
Copy link

ghost commented Jan 9, 2022

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

Issue Details

Description

Let's say I have the following class defined in nullable enabled context:

private class NonNullableValuesDictionary : Dictionary<string, object> { }

If I try to add a null value to the object, I get a compiler error:

var dictionary = new NonNullableValuesDictionary();
dictionary.Add("a", null); // CS Cannot convert null literal to non-nullable reference type

However, I can't replicate this compiler behavior with NullabilityInfoContext even though it seems to me that the compiler must be reading the same metadata in order to issue that warning.

Reproduction Steps

public static void Main()
{
    var context = new NullabilityInfoContext();
    var addMethod = typeof(NonNullableValuesDictionary).GetMethod("Add")!;
    var info = context.Create(addMethod.GetParameters()[1]);
    Console.WriteLine(info.WriteState); // Nullable
}

private class NonNullableValuesDictionary : Dictionary<string, object> { }

Expected behavior

I would expect the code above to print "NotNull" instead.

Actual behavior

Prints "Nullable".

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

No response

Author: madelson
Assignees: -
Labels:

area-System.Collections, untriaged

Milestone: -

@ghost
Copy link

ghost commented Jan 10, 2022

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

Issue Details

Description

Let's say I have the following class defined in nullable enabled context:

private class NonNullableValuesDictionary : Dictionary<string, object> { }

If I try to add a null value to the object, I get a compiler error:

var dictionary = new NonNullableValuesDictionary();
dictionary.Add("a", null); // CS Cannot convert null literal to non-nullable reference type

However, I can't replicate this compiler behavior with NullabilityInfoContext even though it seems to me that the compiler must be reading the same metadata in order to issue that warning.

Reproduction Steps

public static void Main()
{
    var context = new NullabilityInfoContext();
    var addMethod = typeof(NonNullableValuesDictionary).GetMethod("Add")!;
    var info = context.Create(addMethod.GetParameters()[1]);
    Console.WriteLine(info.WriteState); // Nullable
}

private class NonNullableValuesDictionary : Dictionary<string, object> { }

Expected behavior

I would expect the code above to print "NotNull" instead.

Actual behavior

Prints "Nullable".

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

No response

Author: madelson
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

@krwq
Copy link
Member

krwq commented Jan 10, 2022

cc: @buyaa-n

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 10, 2022

This is by design, at runtime there is no way to distinguish if the NonNullableValuesDictionary was Dictionary<string, object> or Dictionary<string, object?> at runtime the DeclaringType of refected method var addMethod = typeof(NonNullableValuesDictionary).GetMethod("Add")!; will be just Dictionary<string, object> without any nullability info we could determine object? was null.

Therefore in order to determine nullability of the Dictionary<string, object>.Add(string, object) we use the original generic definition of the type: Dictionary<TKey, TValue> .Add(TKey, TValue) where TKey not null and TValue is nullable

@buyaa-n buyaa-n closed this as completed Jan 10, 2022
@madelson
Copy link
Contributor Author

@buyaa-n if there is no way to tell, how does the compiler differentiate (or can the compiler only do it if the derived type is defined in the same compilation?)?

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 10, 2022

I guess i should have said that there is no way to distinguish that in reflection, because compiler does not provide any nullability info for those generic parameters (reflection uses compiler provided NullableAttribute or NullableContextAttribute to determine nullability, and it is impossible to provide that info current design)

The compiler nullability analysis and this reflection API are do not work same

@madelson
Copy link
Contributor Author

@buyaa-n but when I decompile my assembly with dotpeek it looks like the nullable metadata is there:

Original:

internal class NonNullableValuesDictionary : Dictionary<string, object> { }
internal class NullableValuesDictionary : Dictionary<string, object?> { }

Decompiled:

  [Nullable(new byte[] {(byte) 0, (byte) 1, (byte) 1})]
  internal class NonNullableValuesDictionary : Dictionary<string, object>
  {
  }

  [Nullable(new byte[] {(byte) 0, (byte) 1, (byte) 2})]
  internal class NullableValuesDictionary : Dictionary<string, object>
  {
  }

Aren't those attributes describing the nullability of the generic parameters for Dictionary?

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 11, 2022

Hm, that actually looks feasible compared to, reopening the issue, thanks!

    var nonNull= typeof(Dictionary<string, object>);
    var Nullable = typeof(Dictionary<string, object?>);

@buyaa-n buyaa-n reopened this Jan 11, 2022
@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Jan 11, 2022
@buyaa-n buyaa-n added this to the 7.0.0 milestone Jan 11, 2022
madelson added a commit to madelson/runtime that referenced this issue Jan 22, 2022
In several cases, NullabilityInfoContext would return values which were
out of sync with the C# compiler's interpretation of the nullability
metadata. This fixes the following cases:
* The `NullablePublicOnly` attribute was not considered when analyzing
private constructor parameters.
* CodeAnalysis attributes on indexer properties were not recognized.
* CodeAnalysis attributes were not recognized in a `#nullable disabled`
context.
* Private value-typed members lacking nullable annotations were not
properly marked as nullable/notnull.
* Mixing CodeAnalysis attributes with opposite meanings (e. g.
`AllowNull` and `DisallowNull` produced the wrong result.
* Analysis of members inherited from generic base types did not
incorporate the nullable metadata associated with the inheritance.

Fix dotnet#63555
Fix dotnet#63846
Fix dotnet#63847
Fix dotnet#63848
Fix dotnet#63849
Fix dotnet#63853
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 22, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 26, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants