Fix nullability analysis of property indexers - #126835
Conversation
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-reflection |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect NullabilityInfoContext results for indexer parameters by ensuring indexer parameter nullability is read from the appropriate accessor (getter/setter) parameter metadata, and adds regression coverage for the reported tuple-property interaction.
Changes:
- Update
NullabilityInfoContext.Create(ParameterInfo)to remap indexerParameterInfofromPropertyInfoto the corresponding getter/setter parameter. - Add tests covering indexers with and without an additional value-tuple-returning property (regression scenario).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/NullabilityInfoContextTests.cs | Adds regression tests validating indexer parameter nullability states, including the tuple-property scenario. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/NullabilityInfoContext.cs | Fixes indexer-parameter handling by switching to accessor parameters and respecting nullablePublicOnly behavior. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "bbe30cc8ec0498a50735230d67449b164ccad458",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "98bfa17c9f3910eec30099b7ac140d8a09df6227",
"last_reviewed_commit": "bbe30cc8ec0498a50735230d67449b164ccad458",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "98bfa17c9f3910eec30099b7ac140d8a09df6227",
"last_recorded_worker_run_id": "29676140108",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "bbe30cc8ec0498a50735230d67449b164ccad458",
"review_id": 4730523036
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Fixes #101071. When calling NullabilityInfoContext.Create(ParameterInfo) on an indexer's index parameter, the parameter's Member is a PropertyInfo rather than a MethodBase. The existing code only detected annotation-disabling for MethodBase members, so indexer parameters fell into CreateParser, but the property-level CustomAttributeData did not carry the per-parameter [Nullable] metadata that lives on the accessor method's parameter. This intermittently surfaced as Unknown (notably when an unrelated value-tuple property shifted the assembly-level nullable context stream), when the correct result is NotNull.
Approach: In Create(ParameterInfo), when the member is a PropertyInfo, the code now redirects parameterInfo to the corresponding parameter of the getter (preferred) or setter, selected by parameterInfo.Position, and only marks annotations disabled when both accessors are absent or private/internal-with-public-only. The non-property MethodBase path retains the prior IsPrivateOrInternalMethodAndAnnotationDisabled behavior. Redirecting the ParameterInfo is the right fix because it also routes the subsequent CheckParameterMetadataType through the MethodInfo switch case, so the accessor's real parameter metadata is consulted. The position mapping is sound: index parameters correspond 1:1 to the leading accessor parameters (the setter's trailing value parameter is never indexed here).
Summary: A well-scoped, correct fix with good test coverage (get-only, set-only, and the value-tuple regression trigger for both), using [Theory]/[MemberData] consistent with the file's conventions. No behavioral regressions for non-indexer parameters. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 56.7 AIC · ⌖ 10.1 AIC · ⊞ 10K
Fixes #101071