[.NET6 fix port] NullabilityInfoContext.Create throws IndexOutOfRangeException

[plachor]

Description

Hi @danmoseley,

Issue #68461 was resolved in .NET7 however the fix was not applied to .NET6.
Since .NET6 is still under support till November 12, 2024 and is called LTS perhaps you could fix .NET6 as-well.

Any chances for that ?

Reproduction Steps

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

namespace Reproduction;

public record GenericType<TType>(IReadOnlyDictionary<string, TType> Dictionary);

public class NullableContextIssue
{
    [Fact]
    public void ShouldNotThrow()
    {
        var property = typeof(GenericType<IReadOnlyCollection<bool>>)
                                     .GetProperty(nameof(GenericType<IReadOnlyCollection<bool>>.Dictionary));
        var action = () => new NullabilityInfoContext().Create(property);

        action.Should().NotThrow();
    }
}

Expected behavior

Should not throw exception.

Actual behavior

System.IndexOutOfRangeException is being observed.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

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

Issue Details

---

Description

Hi @danmoseley,

Issue #68461 was resolved in .NET7 however the fix was not applied to .NET6.
Since .NET6 is still under support till November 12, 2024 and is called LTS perhaps you could fix .NET6 as-well.

Any chances for that ?

Reproduction Steps

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

namespace Reproduction;

public record GenericType<TType>(IReadOnlyDictionary<string, TType> Dictionary);

public class NullableContextIssue
{
    [Fact]
    public void ShouldNotThrow()
    {
        var property = typeof(GenericType<IReadOnlyCollection<bool>>).GetProperty(nameof(GenericType<IReadOnlyCollection<bool>>.Dictionary));
        var action = () => new NullabilityInfoContext().Create(property);

        action.Should().NotThrow();
    }
}

Expected behavior

Should not throw exception.

Actual behavior

System.IndexOutOfRangeException is being observed.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	plachor
Assignees:	-
Labels:	area-System.Reflection
Milestone:	-

[buyaa-n]

There was a big refactoring before #68461 fix, therefore #64143 needs to be ported together, #64143 is also fixing several bugs. So, this will be quite a lot of change, not sure management would approve it porting CC @jeffhandley. Though there is low risk for porting both, the fixes well tested and have been merged for a while, the fixes would be good to have in .NET 6. Putting into future for now.

[jeffhandley]

@buyaa-n If we were to entertain backporting the fixes, but not the refactoring (thus, a fresh implementation of the fixes on top of the old factoring), would that make the net changes to release/6.0 more or less straightforward/risky?

[buyaa-n]

Not sure if it is answers your question: I tried to backport the #68461 fix only and got conflict. Because after .NET 6.0 a refactoring done with bug some fixes which merged before #68461, so #68461 needs those changes

[jeffhandley]

Thanks, @buyaa-n. I'm wondering about a fresh fix based on release/6.0 that would address the bug without introducing the refactoring. That would mean making the fix directly into the branch instead of trying to merge the fix from main.

It's quite likely that we'd consider that to be riskier since it wouldn't have been proven already, but if it could come in as a very targeted fix that is easy to assess, it's worth thinking about.

[buyaa-n]

I'm wondering about a fresh fix based on release/6.0 that would address the bug without introducing the refactoring

Yes, we can do that if needed.

[plachor]

Would be great if .NET6 would receive all fixes for issues that affect it as it is long-term supported release.

[buyaa-n]

Would be great if .NET6 would receive all fixes for issues that affect it as it is long-term supported release.

I agree, but we need a good reason for getting approval for porting both of them, like how badly this bug is affecting customers, what is their scenario, why the porting is necessary, and assess how risky the porting will be. @plachor following questions would help us to determine customer impact and risk assessment:

1. What is your scenario that requires to port this fix, are you blocked by this bug and how?
2. Could you upgrade your project into 7.0 instead? If not, why?
3. Did you tried 7.0 bits with your scenario, and do you see any issue?

This issue has been marked needs-author-action and may be missing some important information.

[plachor]

Hi @buyaa-n, I assume that the whole idea behind LTS releases is to grant stability for people so they know they will receive bug fixes and stable API for longer period of time.

Our system is consisting of over 50 microservices developed by different teams. We upgrade major versions of SDK in a controlled manner once we have good opportunity and release is stable for longer period of time already. Otherwise we to stick to latest patch release of currently used SDK. When release is announced LTS we assume we do not need to rush.

1. What is your scenario that requires to port this fix, are you blocked by this bug and how?

We started to use NullabilityInfoContext in order to support nullable reference types properly across different parts of our system such as:

• JsonSchema generation for our DTOs
• Query string parameters conditionality annotation in exposed OpenApi
• For some paths even aggressive validation of required properties during deserialization of DTOs

We plan to use everywhere were we must know at runtime that some property is optional even if it is a reference type.

So recently we found out that if consumers of our base-components library use it with such combination as in example:

public record GenericType<TType>(IReadOnlyDictionary<string, TType> Dictionary);

it breaks with exception at runtime and makes certain parts of our product unusable.

I deliver a shared components library for those microservices. So I cannot fully predict how end developers will use it and what types they will declare.

We already relay on this functionality and it is not a good option for us to redesigning everything to handle some exceptions from framework that should not occur in first place.

2. Could you upgrade your project into 7.0 instead? If not, why?

Most probably we will, however I do not yet know when a good operational window will be for such transition. This is always a complex process for us as we use large set of external libraries that must be compatible with each other and framework its self. Usually when you release a major it is a good opportunity for other to introduce a major release with breaking changes on their side as well (like Npgsql dependent libraries).

3. Did you tried 7.0 bits with your scenario, and do you see any issue?

If you ask if I tested .NET7 against my sample, than yes. And it is working there correctly. How ever I have not tested my project on which I am working. As it wont be just single line change in csproj to do so.