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.Create throws IndexOutOfRangeException #68461

Closed
ropufu opened this issue Apr 24, 2022 · 11 comments · Fixed by #71851
Closed

NullabilityInfoContext.Create throws IndexOutOfRangeException #68461

ropufu opened this issue Apr 24, 2022 · 11 comments · Fixed by #71851

Comments

@ropufu
Copy link
Contributor

ropufu commented Apr 24, 2022

Description

NullabilityInfoContext will throw IndexOutOfRangeException when calling Create method on certain Dictionary-valued properties in generic classes. Seems to happen when the TValue of the dictionary is a generic class with one type parameter coinciding with the type parameter of the class that defines the property.

Reproduction Steps

// .NET 6.0 LTS Console App project with default settings (nullability context enabled).
using System.Reflection;

Foo<int>? bar = new();
NullabilityInfoContext context = new();
PropertyInfo x = bar.GetType().GetProperty(nameof(bar.Bad))!;
// The next line will throw.
NullabilityInfo info = context.Create(x);
Console.WriteLine(info.ReadState);

public class Foo<T>
{
    public Dictionary<int, List<T>>? Bad { get; set; }
    public Dictionary<int, IEquatable<T>>? AlsoBad { get; set; }
    public Dictionary<int, List<int>>? Good { get; set; }
    public Dictionary<int, T>? AlsoGood { get; set; }
}

Expected behavior

Program should successfully terminate with Nullable displayed on the console.

Actual behavior

Line NullabilityInfo info = context.Create(x); throws a System.IndexOutOfRangeException.

   at System.Reflection.NullabilityInfoContext.CheckGenericParameters(NullabilityInfo nullability, MemberInfo metaMember, Type metaType)
   at System.Reflection.NullabilityInfoContext.TryLoadGenericMetaTypeNullability(MemberInfo memberInfo, NullabilityInfo nullability)
   at System.Reflection.NullabilityInfoContext.GetNullabilityInfo(MemberInfo memberInfo, Type type, IList`1 customAttributes, Int32 index)
   at System.Reflection.NullabilityInfoContext.GetNullabilityInfo(MemberInfo memberInfo, Type type, IList`1 customAttributes, Int32 index)
   at System.Reflection.NullabilityInfoContext.Create(PropertyInfo propertyInfo)

Regression?

No response

Known Workarounds

No response

Configuration

This is a vanilla .NET 6.0 Long-term Support "Console App" project created in Visual Studio 2022 on a Windows 11 machine. Fails in both Debug and Release mode when targeting "Any CPU".

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

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels Apr 24, 2022
@ghost
Copy link

ghost commented Apr 24, 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

NullabilityInfoContext will throw IndexOutOfRangeException when calling Create method on certain Dictionary-valued properties in generic classes. Seems to happen when the TValue of the dictionary is a generic class with one type parameter coinciding with the type parameter of the class that defines the property.

Reproduction Steps

// .NET 6.0 LTS Console App project with default settings (nullability context enabled).
using System.Reflection;

Foo<int>? bar = new();
NullabilityInfoContext context = new();
PropertyInfo x = bar.GetType().GetProperty(nameof(bar.Bad))!;
// The next line will throw.
NullabilityInfo info = context.Create(x);
Console.WriteLine(info.ReadState);

public class Foo<T>
{
    public Dictionary<int, List<T>>? Bad { get; set; }
    public Dictionary<int, IEquatable<T>>? AlsoBad { get; set; }
    public Dictionary<int, List<int>>? Good { get; set; }
    public Dictionary<int, T>? AlsoGood { get; set; }
}

Expected behavior

Program should successfully terminate with Nullable displayed on the console.

Actual behavior

Line NullabilityInfo info = context.Create(x); throws a System.IndexOutOfRangeException.

   at System.Reflection.NullabilityInfoContext.CheckGenericParameters(NullabilityInfo nullability, MemberInfo metaMember, Type metaType)
   at System.Reflection.NullabilityInfoContext.TryLoadGenericMetaTypeNullability(MemberInfo memberInfo, NullabilityInfo nullability)
   at System.Reflection.NullabilityInfoContext.GetNullabilityInfo(MemberInfo memberInfo, Type type, IList`1 customAttributes, Int32 index)
   at System.Reflection.NullabilityInfoContext.GetNullabilityInfo(MemberInfo memberInfo, Type type, IList`1 customAttributes, Int32 index)
   at System.Reflection.NullabilityInfoContext.Create(PropertyInfo propertyInfo)

Regression?

No response

Known Workarounds

No response

Configuration

This is a vanilla .NET 6.0 Long-term Support "Console App" project created in Visual Studio 2022 on a Windows 11 machine. Fails in both Debug and Release mode when targeting "Any CPU".

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

Other information

No response

Author: ropufu
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

@ropufu
Copy link
Contributor Author

ropufu commented Apr 24, 2022

Here are some more cases that will fail, this time without referencing lists or dictionaries.

class Foo<T>
{
    // Nesting 1-inside-2.
    public Two<int, One<T>>? Bad1 { get; set; }
    // Order does not matter.
    public Two<One<T>, int>? Bad2 { get; set; }
    // Nesting 1-inside-3.
    public Three<int, int, One<T>>? Bad3 { get; set; }
    // Nesting 2-inside-3.
    public Three<int, int, Two<T, T>>? Bad4 { get; set; }
}

class Three<T, U, V> { }
class Two<T, U> { }
class One<T> { }

@danmoseley
Copy link
Member

Thanks for the report @ropufu . Any interest in debugging a bit, or offering a fix?

@ropufu
Copy link
Contributor Author

ropufu commented Apr 24, 2022

I'll give it a shot :^) However, I'm very new to creating PR and almost hopeless at git. So this will take me a while to read through all relevant instructions.

@danmoseley
Copy link
Member

Sure - take your time and let me know if you get stuck. There are pretty good workflow instructions though.

@am11
Copy link
Member

am11 commented Apr 25, 2022

Nice report, @ropufu. I can reproduce it with net7.0 as well. A quick fix is to change this line:


as for (int i = 0; i < genericArguments.Length && i < nullability.GenericTypeArguments.Length; i++) . However, I think it is not the right fix as nullability.GenericTypeArguments should account for same number of generic arguments as metadata.GetGenericArguments() so the checks will run for each nested argument as well, right?

using System.Reflection;

Foo<int> bar = new();
NullabilityInfoContext context = new();

PropertyInfo x0 = bar.GetType().GetProperty(nameof(bar.Prop0))!;
NullabilityInfo info0 = context.Create(x0); // fine

PropertyInfo x1 = bar.GetType().GetProperty(nameof(bar.Prop1))!;
NullabilityInfo info1 = context.Create(x1); // throws IOoRE

class Foo<T>
{
    public Two<int, T> Prop0 { get; set; }
    public Two<int, One<T>> Prop1 { get; set; }
}

class Two<T, U> { }
class One<T> { }

@madelson, do you remember seeing this case while working on #64143?

@madelson
Copy link
Contributor

madelson commented Apr 25, 2022

@am11 I don't recall running into this. I'm surprised that the test cases don't cover this given that the repro seems fairly simple.

However, I think it is not the right fix as nullability.GenericTypeArguments should account for same number of generic arguments as metadata.GetGenericArguments()

Agreed. We need to figure out why these differ.

I'd be happy to take this if @ropufu doesn't end up wanting to.

@buyaa-n buyaa-n added this to the 7.0.0 milestone Apr 27, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 27, 2022
@buyaa-n
Copy link
Member

buyaa-n commented Jul 6, 2022

I'll give it a shot

Hello @ropufu? Are you still working on it? It better to be fixed within 7.0 preview 7 and not many days left

@ropufu
Copy link
Contributor Author

ropufu commented Jul 6, 2022

Thank you for the nudge. I wasn't for a while: I could not follow the Contributing/Workflow guidelines to set up debugging for the issue in conjunction with System.Private.CoreLib back then... and I still can't.

If there is a deadline for me to aim at--that would be helpful (assuming that leaves you folks enough time to crack it if I fail). And if you have other references (like a minimal example/walkthrough how to set up a debuggable project in VS that targets the preview .NET) that would be most welcome. My apologies for not speaking up the first time I could not do it.

@buyaa-n
Copy link
Member

buyaa-n commented Jul 6, 2022

After building clr+libs+libs.tests as mentioned in the Contributing/Workflow guidelines you could be able to add tests into https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System/Reflection/NullabilityInfoContextTests.cs and debug from that System.Runtime which references code in System.Private.CoreLib

If there is a deadline for me to aim at--that would be helpful

The preview 7 deadline is next Monday, so if you could do the fix within this week

@ropufu
Copy link
Contributor Author

ropufu commented Jul 8, 2022

@buyaa-n Thank you so much for your help. Right to the point. Saved me a lot of frustration and pain. Thank you!

I think I managed to get the thing fixed. Reflection tests look good; I'll try to get the proper submission done in an hour or two. My only concern is that I am not so fluent in the NullabilityInfoContext class as to understand all of the potential ramifications of my change (although I don't think there should be any).

ropufu added a commit to ropufu/runtime that referenced this issue Jul 8, 2022
The TryLoadGenericMetaTypeNullability method was called with the same
member info but varying nullability across the entire nullability
hierarchy. Moved it one level up where nullability and member info are
aligned.

Added a test to cover this issue.

Fix dotnet#68461
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 11, 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.

5 participants