Merge tuple element names during type inference - #4005
Conversation
The C# standard does not mention tuple element names in type inference, but csc merges names across bounds that differ only by them: names are kept where all bounds agree and dropped where they conflict (Roslyn's MergeTupleNames). All three expectations are verified against csc. The two live tests are red at this commit: without merging, fixing keeps the first bound's element names verbatim. The multiple-exact- bounds case additionally requires AddExactBound to compare bounds modulo element names; it stays ignored until that is implemented. Assisted-by: Claude:claude-fable-5:Claude Code
christophwille
left a comment
There was a problem hiding this comment.
Review summary
The change is sound in the direction it takes: the three merge points do line up with Roslyn's Fix (exact candidates merged with each other, then with lower/upper bounds; lower/upper bounds merged among themselves), the name-merge rule (n1 == n2 ? n1 : null, all-null == default) matches MergeTupleNames, and it fixes a real pre-existing miscompile: exact (int a, string b) (from IList<T>) + lower (int a, string c) used to fix to (int a, string b) with no explicit type args, and the emitted .b access would then not compile against csc's (int a, string).
I ran the four new tests plus three probes against this branch (worktree at 2a63710). What I found, most important first:
- Lower and upper bounds are merged separately, so a lower/upper pair that differs only in names still fails to fix. Roslyn merges everything into one candidate dictionary. Probe:
M<T>(T x, Action<T> y)withIList<(int a, string b)>andAction<IList<(int a, string c)>>(reference-type argument, soAction<in T>gives an upper bound rather than an exact one) ->FindTypesInBoundssees two mutually identity-convertible candidates,Fixreturns false, csc infersIList<(int a, string)>. Not a regression (this failed before too), but it is the one shape the PR description says it collapses that it does not. Fix: runMergeShapeEquivalentBoundsonce overLowerBounds.Concat(UpperBounds)(or seed the upper pass with the merged lower list), which also gets rid of the two copy-pastedforeachloops in the exact path. Inline comment on the code. MergeTupleNamesmerges more than tuple names in the array branch and drops nullability.ArrayType.Equalscompares nullability but the branch only checksDimensionsand rebuilds with the defaultOblivious. Probe: tworefbounds(int a, string b)[]?/(int a, string c)[]?fix to(int a, string)[]withNullability.Oblivious. And with no tuple in sight,string[]?+string[](bounds keep their annotation wheneverU.Nullability != V.Nullability) now collapse to an obliviousstring[]. PassarrA.Nullabilitythrough (asArrayType.VisitChildrendoes), and decide whether nullability-only differences should merge at all.NullabilityAnnotatedTypewrappers are not unwrapped, so the merge is skipped whenever one bound carries an annotation. Probe:M<T>(T x, T y)withIList<(int a, string b)>?andIList<(int a, string c)>-> no merge, two candidates,Fixfails; csc infersIList<(int a, string)>?. Roslyn compares withIgnoreNullableModifiersForReferenceTypesbefore merging. Cheapest fix: gate onNormalizeTypeVisitor.IgnoreNullabilityAndTuples.EquivalentTypes(a, b)(already in the type system) and unwrapNullabilityAnnotatedTypein the walk. Inline comment.- Merged
TupleTypeis rebuilt withoutvalueTupleAssembly. Every other construction site threads it through (TupleType.VisitChildren,ApplyAttributeTypeVisitor,CallBuilder,TranslatedExpression); here the underlyingValueTupleis re-resolved viacompilation.FindType, which walks modules in order and can bind to a different definition when two realValueTupledefinitions are loaded (net461-era non-facadeSystem.ValueTuple.dllnext to a 4.7+ mscorlib, or a main module embedding its own copy; facades are fine, forwarders are followed). ThenTupleType.Equalsno longer matches the original bound, andIsAppropriateCallTarget(which compares the erased underlying type) rejects the candidate and forces explicit type arguments. Rare, one-argument fix:ta.GetDefinition()?.ParentModule. Inline comment. - Structure/altitude: one rule, three patch points, two merge strategies (pairwise fold in
AddExactBoundand the exact path ofFix; absorb-into-list in the bounds path). Doing the merge once atFixtime over exact + lower + upper (and keeping only theMultipleDifferentExactBoundsdecision inAddExactBound) matches Roslyn's structure and removes the duplicated loops. Nit-level:if (a == null || b == null)inMergeTupleNamesis unreachable (no caller passes null), and the doc comment onMergeTupleNamesrestates the same paragraph that already sits at all three call sites (and cites a lambda-parameter scenario none of the tests exercise) -- one sentence on the helper is enough.
Checked and not flagged: the dynamic/object half of Roslyn's MergeEquivalentTypes and pointer/function-pointer/modopt wrappers (can't be type arguments, or outside this PR's scope); the out var name-sensitive Equals check in CallBuilder.IsUnambiguousCall -- with a merged T it degrades out var x to an explicitly typed out (int a, string b) x, which is exactly what keeps a later x.b compiling, so that is an improvement, not a regression. CI is still pending at the time of writing.
Assisted-by: Claude:claude-fable-5:Claude Code
| // same shape as the exact bound except for tuple element names, the names are | ||
| // merged - kept where both sides agree, dropped where they conflict. See | ||
| // MergeTupleNames in Roslyn's MethodTypeInference.cs. | ||
| IType fixedTo = tp.ExactBound; |
There was a problem hiding this comment.
Structure nit: one rule now lives in three places with two different strategies (pairwise fold into ExactBound in AddExactBound and here; absorb-into-list in MergeShapeEquivalentBounds below). Doing the merge once at fix time over exact + lower + upper -- and keeping only the MultipleDifferentExactBounds decision in AddExactBound -- would mirror Roslyn's Fix and remove the two duplicated loops. Also: the // the exact bound will always be the result comment two lines down is no longer literally true; if (a == null || b == null) in MergeTupleNames is unreachable; and the doc comment on MergeTupleNames restates the paragraph that already sits at all three call sites (and cites a lambda-parameter scenario none of the tests exercise) -- one sentence on the helper is enough.
2a63710 to
298081b
Compare
When fixing a type parameter, Roslyn merges the tuple element names of bounds that are identical apart from those names: names are kept where all bounds agree and dropped where they conflict (MergeTupleNames in Roslyn's MethodTypeInference.cs). The C# standard does not describe this step. Without it, fixing either kept the first bound's names verbatim or, with two exact bounds differing only in names, failed outright - so inferred tuple types could carry names csc would not produce. All merged-name expectations are csc-verified. Nullability is deliberately not merged: Roslyn derives it from the variance of the position, which this implementation does not track, so bounds that differ in it stay distinct and fixing fails as before rather than inventing an annotation. Assisted-by: Claude:claude-fable-5:Claude Code Assisted-by: Claude:claude-opus-5[1m]:Claude Code
298081b to
01895fb
Compare
When fixing a type parameter, Roslyn merges the tuple element names of bounds that are
identical apart from those names: a name is kept where all bounds agree and dropped where
they conflict (
MergeTupleNames/MergeEquivalentTypesin Roslyn'sMethodTypeInference.cs).The C# standard's type-inference section does not describe this step, so
TypeInferencedidnot implement it.
Without it, fixing either kept the first bound's element names verbatim, or - with two exact
bounds that differ only in names - failed outright, because
AddExactBoundcompares boundswith name-sensitive equality. Either way, inferred tuple types could carry element names that
csc would never produce.
This adds the merge in the three places a bound set is reduced to a single type:
AddExactBound- two exact bounds that differ only in element names are merged instead ofbeing recorded as conflicting.
Fix(exact-bound path) - the exact bound is merged with each lower/upper bound of the sameshape.
Fix(bounds path) - shape-equivalent bounds are collapsed beforeFindTypesInBounds, sothey do not survive as distinct candidates.
The merge recurses through tuple elements, generic type arguments, and array element types,
matching Roslyn; anything not equal modulo element names is left alone, so non-tuple inference
is unaffected.
No decompiler-visible misdecompilation was demonstrated for this - the change aligns the
resolver's inference with Roslyn rather than fixing a reported output bug.
All expectations in the new tests are verified against csc.