Conversation
Add a first-pass implementation of pair-aware generic inference and overload betterness. Expanded test coverage will follow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c677fcb0-6afa-4edf-a463-af61e2c538fe
Cover nullable constraints and annotations, dependent and structured inference, multi-element overload comparisons, collection targets, and version/params/with interactions.\n\nBaseline the nullable output-inference gap with explicit controls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bb093008-371d-4dd0-8be1-aa5d3c575c95
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Nullable reinference loses component conversions and flow states, producing incorrect annotations for output and List-target inference.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Implements KVP component type inference and overload resolution for preview dictionary expressions.
Changes:
- Adds input/output inference for KVP components and spreads.
- Adds component-aware better-conversion rules.
- Adds extensive inference, nullability, overload, and language-version tests.
Assessment: The core approach matches the proposal, but nullable reinference remains incomplete.
| File | Description |
|---|---|
DictionaryExpressionTests.cs |
Adds comprehensive feature tests. |
NullableWalker.cs |
Preserves component visit results for reinference. |
OverloadResolution.cs |
Compares KVP component conversions. |
MethodTypeInference.cs |
Implements KVP input/output inference. |
ConversionsBase.cs |
Adds annotation-preserving KVP decomposition. |
| VisitRvalue(keyValuePair.Value); | ||
| var valueResult = _visitResult; | ||
|
|
||
| _visitResult = new VisitResult(default, default, [keyResult, valueResult]); |
There was a problem hiding this comment.
Is default, default fine to use here? There is not a risk that information about e.g. reinferred nullability within the Key and Value types would be missed?
| } | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
Is there a reason we don't want to do the next part of the inference when the element type is a KeyValuePair?
I was concerned this would cause us to fail to contribute bounds in a scenario like the following:
void M<T>(IEnumerable<T> e) { }
void M1()
{
M([KeyValuePair.Create("a", "b")]);
}
| } | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
Similar question as for the other return statement above
| // Conversion comparisons are made using better conversion from expression if `ELᵢ` is not a spread element. If `ELᵢ` is a spread element, we use better conversion from the element type of the spread collection to `E₁` or `E₂`, respectively. | ||
|
|
||
| var keyValueTypes1 = ConversionsBase.TryGetCollectionKeyValuePairTypes(Compilation, elementType1); | ||
| var keyValueTypes2 = ConversionsBase.TryGetCollectionKeyValuePairTypes(Compilation, elementType2); |
There was a problem hiding this comment.
These seem like they should be pulled inside the if (!Conversions.HasIdentityConversion...) body
| else | ||
| { | ||
| elementBetterResult = BetterConversionFromExpression((BoundExpression)element, elementType1, conversionToE1, elementType2, conversionToE2, ref useSiteInfo, okToDowngradeToNeither: out _); | ||
| elementBetterResult = BetterResult.Neither; |
There was a problem hiding this comment.
When do we enter this branch?
| // conversion from type from Kᵢ to Kₑ and from Vᵢ to Vₑ. | ||
| case BoundCollectionExpressionSpreadElement spread when | ||
| spread.EnumeratorInfoOpt is { ElementType: { } sourceType } && | ||
| ConversionsBase.IsKeyValuePairType(Compilation, sourceType, out var sourceKeyType, out var sourceValueType): |
There was a problem hiding this comment.
Consider visually breaking up the end of the when clause from the start of the body in some way.
| break; | ||
|
|
||
| default: | ||
| conflictingConversions = false; |
There was a problem hiding this comment.
When would we reach this branch?
|
|
||
| if (matchesTarget1 != matchesTarget2) | ||
| { | ||
| return matchesTarget1 ? BetterResult.Left : BetterResult.Right; |
There was a problem hiding this comment.
Does presence of this code path affect behavior? Or is it essentially optimization?
| } | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: delete extraneous blank line.
Implements KVP type inference for dictionary expressions.
Spec: https://github.com/dotnet/csharplang/blob/main/proposals/dictionary-expressions.md#type-inference
Test plan: #81860
@RikkiGibson @jjonescz for review
Microsoft Reviewers: Open in CodeFlow