runtime icon indicating copy to clipboard operation
runtime copied to clipboard

Can not get NullabilityInfo of out/ref parameters

Open smdn opened this issue 2 years ago • 14 comments

Description

NullabilityInfo created from NullabilityInfoContext.Create(ParameterInfo) does not report about nullability of ByRef parameters' element type, the case like below.

  • void M(out int? para)
  • void M(out KeyValuePair<int, int?>? para)

Reproduction Steps

Demonstration code for reproduction.

using System.Reflection;

foreach (var method in typeof(C).GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)) {
  var para = method.GetParameters()[0];
  var info = new NullabilityInfoContext().Create(para);

  Console.WriteLine(method);
  Console.WriteLine($"ReadState: {info.ReadState}");
  Console.WriteLine($"ElementType: {info.ElementType?.ToString() ?? "(null)"}");
  Console.WriteLine($"GenericTypeArguments: {string.Join(", ", (object[])info.GenericTypeArguments)}");
  Console.WriteLine();
}

class C {
  public void M0(int para) => throw new NotImplementedException();
  public void M1(int? para) => throw new NotImplementedException();
  public void M2(out int para) => throw new NotImplementedException();
  public void M3(out int? para) => throw new NotImplementedException();

  public void M4(out KeyValuePair<int, int> para) => throw new NotImplementedException();
  public void M5(out KeyValuePair<int, int?> para) => throw new NotImplementedException();
  public void M6(out KeyValuePair<int, int>? para) => throw new NotImplementedException();
  public void M7(out KeyValuePair<int, int?>? para) => throw new NotImplementedException();
}

Project file configuration:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>
</Project>

Expected behavior

See 'Actual behavior'

Actual behavior

Void M0(Int32)
ReadState: NotNull // ✅ As expected
ElementType: (null)
GenericTypeArguments: 

Void M1(System.Nullable`1[System.Int32])
ReadState: Nullable // ✅ As expected
ElementType: (null)
GenericTypeArguments: 

Void M2(Int32 ByRef)
ReadState: Unknown // ❓This should be NotNull?
ElementType: (null) // ❓Or NullabilityInfo of element type should be shown?
GenericTypeArguments: 

Void M3(System.Nullable`1[System.Int32] ByRef)
ReadState: Unknown // ❓This should be Nullable?
ElementType: (null) // ❓Or NullabilityInfo of element type should be shown?
GenericTypeArguments: 

Void M4(System.Collections.Generic.KeyValuePair`2[System.Int32,System.Int32] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Void M5(System.Collections.Generic.KeyValuePair`2[System.Int32,System.Nullable`1[System.Int32]] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Void M6(System.Nullable`1[System.Collections.Generic.KeyValuePair`2[System.Int32,System.Int32]] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Void M7(System.Nullable`1[System.Collections.Generic.KeyValuePair`2[System.Int32,System.Nullable`1[System.Int32]]] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Regression?

No response

Known Workarounds

  • #issuecomment-1186243510 (in/out/ref parameters)
  • #issuecomment-1188934503 (ref properties)

Configuration

.NET SDK (reflecting any global.json):
 Version:   6.0.302
 Commit:    c857713418

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  20.04
 OS Platform: Linux
 RID:         ubuntu.20.04-x64
 Base Path:   /usr/share/dotnet/sdk/6.0.302/

global.json file:
  Not found

Host:
  Version:      6.0.7
  Architecture: x64
  Commit:       0ec02c8c96

*snip*

Other information

There are same problems in case of:

  • NullabilityInfoContext.Create(ParameterInfo) with ref return parameters
  • NullabilityInfoContext.Create(PropertyInfo) with ref properties

There may be same problems in case of:

  • NullabilityInfoContext.Create(FieldInfo) with C#11 ref fields (?)

smdn avatar Jul 16 '22 14:07 smdn

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

Issue Details

Description

NullabilityInfo created from NullabilityInfoContext.Create(ParameterInfo) does not report about nullability of ByRef parameters' element type, the case like below.

  • void M(out int? para)
  • void M(out KeyValuePair<int, int?>? para)

Reproduction Steps

Demonstration code for reproduction.

using System.Reflection;

foreach (var method in typeof(C).GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)) {
  var para = method.GetParameters()[0];
  var info = new NullabilityInfoContext().Create(para);

  Console.WriteLine(method);
  Console.WriteLine($"ReadState: {info.ReadState}");
  Console.WriteLine($"ElementType: {info.ElementType?.ToString() ?? "(null)"}");
  Console.WriteLine($"GenericTypeArguments: {string.Join(", ", (object[])info.GenericTypeArguments)}");
  Console.WriteLine();
}

class C {
  public void M0(int para) => throw new NotImplementedException();
  public void M1(int? para) => throw new NotImplementedException();
  public void M2(out int para) => throw new NotImplementedException();
  public void M3(out int? para) => throw new NotImplementedException();

  public void M4(out KeyValuePair<int, int> para) => throw new NotImplementedException();
  public void M5(out KeyValuePair<int, int?> para) => throw new NotImplementedException();
  public void M6(out KeyValuePair<int, int>? para) => throw new NotImplementedException();
  public void M7(out KeyValuePair<int, int?>? para) => throw new NotImplementedException();
}

Project file configuration:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>
</Project>

Expected behavior

See 'Actual behavior'

Actual behavior

Void M0(Int32)
ReadState: NotNull // ✅ As expected
ElementType: (null)
GenericTypeArguments: 

Void M1(System.Nullable`1[System.Int32])
ReadState: Nullable // ✅ As expected
ElementType: (null)
GenericTypeArguments: 

Void M2(Int32 ByRef)
ReadState: Unknown // ❓This should be NotNull?
ElementType: (null) // ❓Or NullabilityInfo of element type should be shown?
GenericTypeArguments: 

Void M3(System.Nullable`1[System.Int32] ByRef)
ReadState: Unknown // ❓This should be Nullable?
ElementType: (null) // ❓Or NullabilityInfo of element type should be shown?
GenericTypeArguments: 

Void M4(System.Collections.Generic.KeyValuePair`2[System.Int32,System.Int32] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Void M5(System.Collections.Generic.KeyValuePair`2[System.Int32,System.Nullable`1[System.Int32]] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Void M6(System.Nullable`1[System.Collections.Generic.KeyValuePair`2[System.Int32,System.Int32]] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Void M7(System.Nullable`1[System.Collections.Generic.KeyValuePair`2[System.Int32,System.Nullable`1[System.Int32]]] ByRef)
ReadState: Unknown // ❓No information about element type of ByRef types
ElementType: (null) // ❓No information about element type of ByRef types
GenericTypeArguments: 

Regression?

No response

Known Workarounds

No response

Configuration

.NET SDK (reflecting any global.json):
 Version:   6.0.302
 Commit:    c857713418

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  20.04
 OS Platform: Linux
 RID:         ubuntu.20.04-x64
 Base Path:   /usr/share/dotnet/sdk/6.0.302/

global.json file:
  Not found

Host:
  Version:      6.0.7
  Architecture: x64
  Commit:       0ec02c8c96

*snip*

Other information

There may be same problems in case of

  • NullabilityInfoContext.Create(ParameterInfo) with ref return parameters
  • NullabilityInfoContext.Create(FieldInfo) with C#11 ref fields (?)
Author: smdn
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

msftbot[bot] avatar Jul 16 '22 14:07 msftbot[bot]

It's a bit of a gross hack, but a workaround I found is to create a wrapper ParameterInfo class that overrides the APIs used by NullabilityInfoContext.Create, but returns ParameterType.GetElementType():

#nullable enable
using System;
using System.Reflection;
using System.Collections.Generic;

foreach (var method in typeof(C).GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)) {
  var para = new ParameterElementTypeInfo(method.GetParameters()[0]);
  var info = new NullabilityInfoContext().Create(para);

  Console.WriteLine(method);
  Console.WriteLine($"ReadState: {info.ReadState}");
  Console.WriteLine($"ElementType: {info.ElementType?.ToString() ?? "(null)"}");
  Console.WriteLine($"GenericTypeArguments: {string.Join(", ", (object[])info.GenericTypeArguments)}");
  Console.WriteLine();
}

class C {
  public void M0(int para) => throw new NotImplementedException();
  public void M1(int? para) => throw new NotImplementedException();
  public void M2(out int para) => throw new NotImplementedException();
  public void M3(out int? para) => throw new NotImplementedException();

  public void M4(out KeyValuePair<int, int> para) => throw new NotImplementedException();
  public void M5(out KeyValuePair<int, int?> para) => throw new NotImplementedException();
  public void M6(out KeyValuePair<int, int>? para) => throw new NotImplementedException();
  public void M7(out KeyValuePair<int, int?>? para) => throw new NotImplementedException();
}

sealed class ParameterElementTypeInfo : ParameterInfo
{
    public ParameterInfo Base { get; }
    
    public override MemberInfo Member => Base.Member;

    public override IList<CustomAttributeData> GetCustomAttributesData() => Base.GetCustomAttributesData();

    public ParameterElementTypeInfo(ParameterInfo param) => Base = param;

    public override Type ParameterType
    {
        get
        {
            if (Base.ParameterType.HasElementType)
                return Base.ParameterType.GetElementType()!;
        
            return Base.ParameterType;
        }
    }
}

SharpLab Demo

DaZombieKiller avatar Jul 16 '22 17:07 DaZombieKiller

@DaZombieKiller, I tried your workaround and that worked fine in my case. Thank you for your comment!

smdn avatar Jul 17 '22 09:07 smdn

I'd be interested in working on this @buyaa-n .

We'd need to decide on the behavior, though. Should we treat T& as a "scalar" type like we do Nullable<T> or as a compound type with an element like T[]? What about Nullable<T>&? Should in vs. out change the result in anyway (I don't think so).

madelson avatar Jul 19 '22 00:07 madelson

I think that the ByRef type itself never have nullability informations as far as I know, so it could be treated as a scalar type like in case of Nullable<T>. (Assuming that T& is represented as like ByRef<T>, both Nullable<> and ByRef<> themselves never have nullability information)

However, IMO, even if T& can be represented like a scalar type, it is better to treat it as a compound type.

In the System.Type class, T[], T&, and T* are all represented as compound types which have element type T. And NullabilityInfo has NullabilityInfo.ElementType property like as well as Type.GetElementType(). So, I think it seems more natural to treat T& in the same way as T[] in the NullabilityInfo class.

smdn avatar Jul 19 '22 02:07 smdn

There's a same problem with ref properties, and @DaZombieKiller 's workaround also works.

Run on SharpLab

#nullable enable
#define WORKAROUND

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Reflection;

foreach (var property in typeof(C).GetProperties(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)) {
#if WORKAROUND
  var info = new NullabilityInfoContext().Create(new PropertyElementTypeInfo(property));
#else
  var info = new NullabilityInfoContext().Create(property);
#endif

  Console.WriteLine(property);
  Console.WriteLine($"ReadState: {info.ReadState}");
  Console.WriteLine($"Type: {info.Type}");
  Console.WriteLine($"ElementType: {info.ElementType?.ToString() ?? "(null)"}");
  Console.WriteLine();
}

class C {
  public int P0 => throw new NotImplementedException();
  public int? P1 => throw new NotImplementedException();
  public ref int PRef0 => throw new NotImplementedException();
  public ref int? PRef1 => throw new NotImplementedException();
  public ref (int, int) PRefValueTuple0 => throw new NotImplementedException();
  public ref (int, int?) PRefValueTuple1 => throw new NotImplementedException();
  public ref (int, int)? PRefValueTuple2 => throw new NotImplementedException();
  public ref (int, int?)? PRefValueTuple3 => throw new NotImplementedException();
}

#if WORKAROUND
sealed class PropertyElementTypeInfo : PropertyInfo {
  public PropertyInfo BaseProperty { get; }

  public PropertyElementTypeInfo(PropertyInfo baseProperty)
  {
    BaseProperty = baseProperty;
  }

  public override Type PropertyType {
    get {
      if (BaseProperty.PropertyType.HasElementType)
        return BaseProperty.PropertyType.GetElementType()!;

      return BaseProperty.PropertyType;
    }
  }

  public override string Name => BaseProperty.Name;
  public override PropertyAttributes Attributes => BaseProperty.Attributes;
  public override bool CanRead => BaseProperty.CanRead;
  public override bool CanWrite => BaseProperty.CanWrite;
  public override Type? DeclaringType => BaseProperty.DeclaringType;
  public override Type? ReflectedType => BaseProperty.ReflectedType;
  public override IList<CustomAttributeData> GetCustomAttributesData() => BaseProperty.GetCustomAttributesData();
  public override object[] GetCustomAttributes(bool inherit) => BaseProperty.GetCustomAttributes(inherit);
  public override object[] GetCustomAttributes(Type attributeType, bool inherit) => BaseProperty.GetCustomAttributes(attributeType, inherit);
  public override bool IsDefined(Type attributeType, bool inherit) => BaseProperty.IsDefined(attributeType, inherit);
  public override MethodInfo[] GetAccessors(bool nonPublic) => BaseProperty.GetAccessors(nonPublic);
  public override MethodInfo? GetGetMethod(bool nonPublic) => BaseProperty.GetGetMethod(nonPublic);
  public override MethodInfo? GetSetMethod(bool nonPublic) => BaseProperty.GetSetMethod(nonPublic);
  public override ParameterInfo[] GetIndexParameters() => BaseProperty.GetIndexParameters();
  public override object? GetValue(object? obj, BindingFlags invokeAttr, Binder? binder, object?[]? index, CultureInfo? culture)
    => BaseProperty.GetValue(obj, invokeAttr, binder, index, culture);
  public override void SetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, object?[]? index, CultureInfo? culture)
    => BaseProperty.SetValue(obj, value, invokeAttr, binder, index, culture);
}
#endif // WORKAROUND

smdn avatar Jul 19 '22 11:07 smdn

We'd need to decide on the behavior, though. Should we treat T& as a "scalar" type like we do Nullable<T> or as a compound type with an element like T[]? What about Nullable<T>&? Should in vs. out change the result in anyway (I don't think so).

Good question, @smdn's point makes sense to me, but it seems works as reference types as a scalar types without any workaround (did not test all scenarios though)., that means if we change it now to work as compound type we could broke somebody who were using the API for out/ref parameters having reference types. Therefore might be better to continue with treating as scalar type, any thoughts @terrajobst?

buyaa-n avatar Jul 20 '22 17:07 buyaa-n

@buyaa-n / @terrajobst any update on this? I'm hesitant to start working on it without a clear spec of the intended behavior. It would be nice to start from @smdn 's original test case table. I also wonder if in vs. out vs. ref should affect anything (perhaps in would not have a WriteState?).

madelson avatar Jul 28 '22 11:07 madelson

If we treat them as compound type and put their nullability info in info.ElementType what would the info.ReadState infor.WriteState would express? Further how many users would know they need to check the info.ElementType nullability instead of using info.ReadState or infor.WriteState directly to get the answer? The more I think about it, the more feel it should be treated as scalar type

I also wonder if in vs. out vs. ref should affect anything (perhaps in would not have a WriteState?).

You mean to have WriteState.Unknown? Not sure if that necessary, it might cause an unnecessary check for the user to check if it was in parameter, probably OK to have the same Read/Write state

buyaa-n avatar Jul 28 '22 18:07 buyaa-n

@buyaa-n

Further how many users would know they need to check the info.ElementType nullability instead of using info.ReadState or infor.WriteState directly to get the answer?

I agree that it would be better to treat it as a scalar type for the most of users.

If we treat them as compound type and put their nullability info in info.ElementType what would the info.ReadState infor.WriteState would express?

How about adding a new state to NullabilityState for this question? For example, a state like NullabilityState.ElementTypeMayHaveNullabilityState. (although the name needs to be discussed) This state suggests that the NullabilityInfo does not directly represent the NullabilityState, and should refer to the ElementType instead.

Also, how about treating the NullabilityState of in/out/ref with a new state as well? A new state like NullabilityState.Inaccessible. (Since NullabilityState.Unknown should represents Unknown state of nullability metadata, so I have a concern about whether it is appropriate to use this value to mean something else.)

ParameterInfo info.ReadState info.WriteState
in T Nullable/NotNull or ElementTypeMayHaveNullabilityState Inaccessible
out T Inaccessible Nullable/NotNull or ElementTypeMayHaveNullabilityState
ref T Nullable/NotNull or ElementTypeMayHaveNullabilityState Nullable/NotNull or ElementTypeMayHaveNullabilityState

..but if we add new states, should we treat the properties the same way? (This would be a destructive change since it is already reported as Unknown in PropertyInfo. So this is probably unacceptable.)

PropertyInfo info.ReadState info.WriteState
public T { get; } Nullable/NotNull Inaccessible
public T { set; } Inaccessible Nullable/NotNull
public T { get; set; } Nullable/NotNull Nullable/NotNull

smdn avatar Jul 29 '22 11:07 smdn

If we treat them as compound type and put their nullability info in info.ElementType what would the info.ReadState infor.WriteState would express?

I agree that this would be confusing, since .NET doesn't have a way to represent a nullable reference (although you can do Unsafe.NullRef<T>()). It's also consistent with how we're already treating cases like this MyMethod([AllowNull] ref string s).

but if we add new states, should we treat the properties the same way? This would be a destructive change...

@smdn I agree that perhaps differentiating between n/a and unknown in the API would be helpful, but only if it were applied universally and not just in this one case. Seems unlikely that such a breaking change would be made.

I think the argument for not differentiating between out, in, and ref for parameters is that all parameters get both read from and written to just at different points. So, the nullability applies in all cases. For example consider this case with out:

void MyMethod([AllowNull] out string s);

string? s;
MyMethod(out s); // no warning because WriteState is nullable
Console.WriteLine(s.Length); // no warning because ReadState is non-nullable

madelson avatar Aug 02 '22 11:08 madelson

@smdn I agree that perhaps differentiating between n/a and unknown in the API would be helpful, but only if it were applied universally and not just in this one case. Seems unlikely that such a breaking change would be made.

Right adding a new state in NullabilityState could be useful for those scenarios, but could break people that was already using old state for those scenarios

buyaa-n avatar Aug 04 '22 00:08 buyaa-n

@madelson

For example consider this case with out:

Thank you for pointing that out. I had not considered about nullability from the caller's viewpoint.

@buyaa-n, @madelson There's a concern that the changes to the NullabilityState would break existing code like you mentioned. So I have come up with another proposal.

Here, we add the following three members to NullabilityInfo.

class NullabilityInfo {
  public bool HasElementType { get; }
  public bool HasReadState { get; }
  public bool HasWriteState { get; }
}

First off, HasElementType indicates that it is a compound type and that info.ElementType must be checked to get the actual nullability. This is similar to Type.HasElementType, so I think that NullabilityInfo.HasElementType also should be true for arrays. T?(Nullable<T>) is already treated as a scalar type, so it is false.

info.Type info.HasElementType info.ElementType
T& true NullabilityInfo of T
T[] true NullabilityInfo of T
T? false null

Next, HasReadState/HasWriteState indicates whether the ReadState/WriteState are actually have a meaningful value. These are similar to Nullable<T>.HasValue.

info info.HasElementType info.HasReadState info.HasWriteState
T& true false false
T of T& (info.ElementType) true/false true true

HasElementType would help determining if ElementType should be checked or not, and HasReadState/HasWriteState would help whether ReadState/WriteState has a meaningful value. HasReadState/HasWriteState may be just simplified to HasState. And this change would not be a breaking changes to the existing code since we just add new members.

Additionally, it may be good to add [MemberNotNullWhen] to ElementType.

class NullabilityInfo {
  public bool HasElementType { get; }

  [MemberNotNullWhen(true, nameof(HasElementType))]
  public NullabilityInfo? ElementType { get; }
}

Of course, this proposal is for the case when treating T& as a compound type.

I still think it is better to treat it as a compound type, but I can't say for sure which one it should be. However, treating T& as a scalar type means that the information that it is a compound type will be dropped, and also means that it will be difficult to find some workaround to deal with problems in the future. Therefore, I think it is better to treat it as a compound type, meaning that the original type information is kept as much as possible.

smdn avatar Aug 05 '22 13:08 smdn

Therefore, I think it is better to treat it as a compound type

Here's the case I would make for scalar type:

Compat: today, you can check the nullability of an out/ref member and it is sometimes correct, especially if explicit attributes are present. The result does not have an element type. If we change to compound, anyone relying on this behavior will break.

Unnecessary: with arrays, it makes sense to have element type since nullability affects the array and element separately (e.g. T[] vs T?[] vs T[]? vs T?[]?). In contrast, with ByRef types today we have T?& but no T&? (there is Unsafe.NullRef<T>() but nullable annotations don't support this having a different type today AFAIK).

Simplicity: I think that callers are probably more likely to make mistakes if a parameter like ref object? o always reports as being non-nullable and only the element type is nullable (or perhaps it would always report as nullable at the top level because you can pass Unsafe.NullRef<object?>() without a compiler warning...).

To me the most compelling argument in favor of compound approach is that if .NET were to adopt the concept of T&? to represent Unsafe.NullRef<T>() then it would be difficult for a scalar API to go back and support that. Curious whether you see that as at all likely @buyaa-n .

madelson avatar Aug 05 '22 19:08 madelson