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

[release/6.0] Fix NullabilityInfoContext.Create throws IndexOutOfRangeException and other bugs #81291

Merged
merged 4 commits into from
Feb 11, 2023

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Jan 27, 2023

Manual backport of #71851 and #64143
Fixes: #81022

Context

These bugs have been reported previously and fixed in 7.0. A customer requested to port #71851 into 6.0 as it is LTS, but #71851 fix is based on #64143 and #64143 also has fixes for uncovered edge case scenarios related to generic type parameters nullability. Overall, these 2 PRs completes Expose Nullability info from Reflection feature we added in .NET 6.0

Customer Impact

The customer started to use NullabilityInfoContext in order to support nullable reference types properly across different parts of their system and plan to use it everywhere were they must know at runtime that some property is optional. They found out that NullabilityInfoContext breaks with exception at runtime and makes certain parts of their product unusable.

This could happen for any customer that uses NullabilityInfoContext with types having generic type parameters and there is no work around.

Testing

  • The backports has complete unit tests coverage
  • Manual validation of the repro scenario with the bits build from this PR.

Risk

Low, the NullabilityInfoContext feature doesn't affect any other library functionality, the fixes reviewed and unit tested completely. The bugs were fixed in 7.0 a while ago and it's been stable since then.

CC @jeffhandley @steveharter

madelson and others added 2 commits January 24, 2023 14:54
* Fix several bugs in NullabilityInfoContext.
* Moved call to TryLoadGenericMetaTypeNullability

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.
@ghost
Copy link

ghost commented Jan 27, 2023

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

Issue Details

Manual backport of #71851 and #64143
Fixes: #81022

Context

These bugs have been reported previously and fixed in 7.0. A customer requested to port #71851 into 6.0 as it is LTS, but #71851 fix is based on #64143 and #64143 also has fixes for uncovered edge case scenarios related to generic type parameters nullability. Overall, these 2 PRs completes Expose Nullability info from Reflection feature we added in .NET 6.0

Customer Impact

The customer started to use NullabilityInfoContext in order to support nullable reference types properly across different parts of their system and plan to use it everywhere were they must know at runtime that some property is optional. They found out that NullabilityInfoContext breaks with exception at runtime and makes certain parts of their product unusable.

This could happen for any customer that uses NullabilityInfoContext with types having generic type parameters and there is no work around.

Testing

  • The backports has complete unit tests coverage
  • Manual validation of a repro scenario.

Risk

Low, the NullabilityInfoContext feature doesn't affect any other library functionality, the fixes unit tested completely. The bugs were fixed in 7.0 a while ago and it's been stable since then.

CC @jeffhandley @steveharter

Author: buyaa-n
Assignees: buyaa-n
Labels:

area-System.Reflection

Milestone: -

@buyaa-n buyaa-n added the Servicing-consider Issue for next servicing release review label Jan 27, 2023
@buyaa-n buyaa-n added this to the 6.0.x milestone Jan 27, 2023
@jeffhandley
Copy link
Member

@buyaa-n Some of the failing tests look like they involve Reflection; can you take a look please?

https://github.com/dotnet/runtime/pull/81291/checks?check_run_id=10941557543

@jeffhandley
Copy link
Member

Once we confirm this is coming in clean and no tests regressed, then this has my support. I know we considered several approaches for backporting the fix and I agree with the conclusion that the best approach is to backport the refactoring and other fixes along with the fix for the specific issue reported.

I looked back at our risk taxonomy to check that Low is appropriate; it is.

Low – The risk is non-negligible. The most common level of risk for bugs we take. Likely acceptable if the value otherwise meets the bar. Risk mitigated with tests, bonus points if a customer has tried it out.

I echo the "bonus points if a customer has tried it out" statement. Since there are unrelated changes as described, getting the customer's validation that the net8.0 bits as-is work in their scenarios would be reassuring.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Jan 30, 2023

Some of the failing tests look like they involve Reflection; can you take a look please?

This is not related, I see System.Reflection.Metadata.MetadataUpdater.IsSupported=false which is hot reload related. Doesn't seem there is issue created for those failures, so created one. Also want to mention that this reflection NullabilityInfoContext doesn't affect any other reflection/runtime libraries

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Jan 30, 2023

All other failures unrelated and has related issue or opened new issues: #74588, #81388, #80476, #81391

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Jan 30, 2023

I echo the "bonus points if a customer has tried it out" statement. Since there are unrelated changes as described, getting the customer's validation that the net8.0 bits as-is work in their scenarios would be reassuring.

I asked for that, the response was 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.

@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 31, 2023
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.15 Jan 31, 2023
@ericstj ericstj added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Jan 31, 2023
@carlossanlop
Copy link
Member

Closing and reopening because the CI couldn't find the obsolete helix queues and a lot of test coverage was lost. I already fixed that and merged it: #81807

@carlossanlop carlossanlop reopened this Feb 8, 2023
@buyaa-n buyaa-n closed this Feb 9, 2023
@buyaa-n buyaa-n reopened this Feb 9, 2023
@carlossanlop carlossanlop merged commit dfcb339 into dotnet:release/6.0 Feb 11, 2023
@buyaa-n buyaa-n deleted the issue-68461 branch February 12, 2023 04:31
@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants