Skip to content

Commit

Permalink
Better Collection Expression Conversion From Expression (#74993)
Browse files Browse the repository at this point in the history
* Initial prototype work

* Correctly handle spreads

* Update test baselines

* Add dedicated interpolated string handler test

* Implement params changes and update tests

* Support falling back to C# 12 rules.

* Add more tests, including the table from https://github.com/dotnet/csharplang/blob/main/proposals/collection-expressions-better-conversion.md and an explicit test for #73857.

* Add tests to the CSharp 12 list as well

* Update language feature status.

* Cleanup

* Correct spread handling

* Only include expected output on .NET Core

* PR feedback.

* Add suggested numeric test.

* Document breaking changes

* More feedback.

* Add examples

* Add example of interpolated string breaking change

* PR feedback

* Update language feature status.

* More tests around dynamic and tuples

* Add collection expressions to test plan

* Spread testing
  • Loading branch information
333fred committed Sep 20, 2024
1 parent d799b05 commit 433ae40
Show file tree
Hide file tree
Showing 6 changed files with 1,663 additions and 198 deletions.
1 change: 1 addition & 0 deletions docs/Language Feature Status.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ efforts behind them.
| [Overload Resolution Priority](https://github.com/dotnet/csharplang/issues/7706) | main | [Merged to 17.12p1](https://github.com/dotnet/roslyn/issues/74131) | [333fred](https://github.com/333fred) | [jcouv](https://github.com/jcouv), [cston](https://github.com/cston) | Not yet assigned | [333fred](https://github.com/333fred) |
| [Partial properties](https://github.com/dotnet/csharplang/issues/6420) | [partial-properties](https://github.com/dotnet/roslyn/tree/features/partial-properties) | [Merged into 17.11p3](https://github.com/dotnet/roslyn/issues/73090) | [RikkiGibson](https://github.com/RikkiGibson) | [jcouv](https://github.com/jcouv), [333fred](https://github.com/333fred) | [Cosifne](https://github.com/Cosifne) | [333fred](https://github.com/333fred), [RikkiGibson](https://github.com/RikkiGibson) |
| [Ref Struct Interfaces](https://github.com/dotnet/csharplang/issues/7608) | [RefStructInterfaces](https://github.com/dotnet/roslyn/tree/features/RefStructInterfaces) | [Merged into 17.11p2](https://github.com/dotnet/roslyn/issues/72124) | [AlekseyTs](https://github.com/AlekseyTs) | [cston](https://github.com/cston), [jjonescz](https://github.com/jjonescz) | [ToddGrun](https://github.com/ToddGrun) | [agocke](https://github.com/agocke), [jaredpar](https://github.com/jaredpar) |
| [Collection expression better conversion from expression](https://github.com/dotnet/csharplang/issues/8374) | main | [Merged into 17.12p3](https://github.com/dotnet/roslyn/pull/74993) | [333fred](https://github.com/333fred) | [cston](https://github.com/cston), [AlekseyTs](https://github.com/AlekseyTs) | (no IDE impact) | [333fred](https://github.com/333fred), [CyrusNajmabadi](https://github.com/CyrusNajmabadi) |

# C# 12.0

Expand Down
49 changes: 49 additions & 0 deletions docs/compilers/CSharp/Compiler Breaking Changes - DotNet 9.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,52 @@ unsafe class C // unsafe context
```

You can work around the break simply by adding the `unsafe` modifier to the local function.

## Collection expression breaking changes with overload resolution in C# 13 and newer

***Introduced in Visual Studio 2022 Version 17.12 and newer when using C# 13+***

There are a few changes in collection expression binding in C# 13. Most of these are turning ambiguities into successful compilations,
but a couple are breaking changes that either result in a new compilation error, or are a behavior breaking change. They are detailed
below.

### Empty collection expressions no longer use whether an API is a span to tiebreak on overloads

When an empty collection expression is provided to an overloaded method, and there isn't a clear element type, we no longer use whether
an API takes a `ReadOnlySpan<T>` or a `Span<T>` to decide whether to prefer that API. For example:

```cs
class C
{
static void M(ReadOnlySpan<int> ros) {}
static void M(Span<object> s) {}

static void Main()
{
M([]); // C.M(ReadOnlySpan<int>) in C# 12, error in C# 13.
}
}
```

### Exact element type is preferred over all else

In C# 13, we prefer an exact element type match, looking at conversions from expressions. This can result in a behavior change when involving
constants:

```cs
class C
{
static void M1(ReadOnlySpan<byte> ros) {}
static void M1(Span<int> s) {}

static void M2(ReadOnlySpan<string> ros) {}
static void M2(Span<CustomInterpolatedStringHandler> ros) {}

static void Main()
{
M1([1]); // C.M(ReadOnlySpan<byte>) in C# 12, C.M(Span<int>) in C# 13
M2([$"{1}"]); // C.M(ReadOnlySpan<string>) in C# 12, C.M(Span<CustomInterpolatedStringHandler>) in C# 13
}
}
```
2 changes: 2 additions & 0 deletions docs/contributing/Compiler Test Plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ This document provides guidance for thinking about language interactions and tes
- extension based Dispose, DisposeAsync, GetEnumerator, GetAsyncEnumerator, Deconstruct, GetAwaiter etc.
- UTF8 String Literals (string literals with 'u8' or 'U8' type suffix).
- Inline array element access and slicing.
- Collection expressions and spread elements

# Misc
- reserved keywords (sometimes contextual)
Expand Down Expand Up @@ -345,6 +346,7 @@ __makeref( x )
- Function type (in type inference comparing function types of lambdas or method groups)
- UTF8 String Literal (string constant value to ```byte[]```, ```Span<byte>```, or ```ReadOnlySpan<byte>``` types)
- Inline arrays (conversions to Span and ReadOnlySpan)
- Collection expression conversions

## Types

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2416,13 +2416,10 @@ private BetterResult BetterFunctionMember<TMember>(

if (!Conversions.HasIdentityConversion(t1, t2))
{
if (IsBetterParamsCollectionType(t1, t2, ref useSiteInfo))
var betterResult = BetterParamsCollectionType(t1, t2, ref useSiteInfo);
if (betterResult != BetterResult.Neither)
{
return BetterResult.Left;
}
if (IsBetterParamsCollectionType(t2, t1, ref useSiteInfo))
{
return BetterResult.Right;
return betterResult;
}
}
}
Expand Down Expand Up @@ -2850,35 +2847,157 @@ private BetterResult BetterConversionFromExpression(BoundExpression node, TypeSy
if (conv1.Kind == ConversionKind.CollectionExpression &&
conv2.Kind == ConversionKind.CollectionExpression)
{
if (IsBetterCollectionExpressionConversion(t1, conv1, t2, conv2, ref useSiteInfo))
return BetterCollectionExpressionConversion((BoundUnconvertedCollectionExpression)node, t1, conv1, t2, conv2, ref useSiteInfo);
}

// - T1 is a better conversion target than T2 and either C1 and C2 are both conditional expression
// conversions or neither is a conditional expression conversion.
return BetterConversionTarget(node, t1, conv1, t2, conv2, ref useSiteInfo, out okToDowngradeToNeither);
}

private BetterResult BetterCollectionExpressionConversion(
BoundUnconvertedCollectionExpression collectionExpression,
TypeSymbol t1, Conversion conv1,
TypeSymbol t2, Conversion conv2,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
var kind1 = conv1.GetCollectionExpressionTypeKind(out TypeSymbol elementType1, out _, out _);
var kind2 = conv2.GetCollectionExpressionTypeKind(out TypeSymbol elementType2, out _, out _);

if (Compilation.LanguageVersion < LanguageVersion.CSharp13)
{
if (IsBetterCollectionExpressionConversion_CSharp12(t1, kind1, elementType1, t2, kind2, elementType2, ref useSiteInfo))
{
return BetterResult.Left;
}
if (IsBetterCollectionExpressionConversion(t2, conv2, t1, conv1, ref useSiteInfo))
if (IsBetterCollectionExpressionConversion_CSharp12(t2, kind2, elementType2, t1, kind1, elementType1, ref useSiteInfo))
{
return BetterResult.Right;
}

return BetterResult.Neither;
}

// - T1 is a better conversion target than T2 and either C1 and C2 are both conditional expression
// conversions or neither is a conditional expression conversion.
return BetterConversionTarget(node, t1, conv1, t2, conv2, ref useSiteInfo, out okToDowngradeToNeither);
else
{
return BetterCollectionExpressionConversion(
collectionExpression.Elements,
t1, kind1, elementType1, conv1.UnderlyingConversions,
t2, kind2, elementType2, conv2.UnderlyingConversions,
ref useSiteInfo);
}
}

// Implements the rules for
// - E is a collection expression and one of the following holds: ...
private bool IsBetterCollectionExpressionConversion(TypeSymbol t1, Conversion conv1, TypeSymbol t2, Conversion conv2, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
private BetterResult BetterCollectionExpressionConversion(
ImmutableArray<BoundNode> collectionExpressionElements,
TypeSymbol t1, CollectionExpressionTypeKind kind1, TypeSymbol elementType1, ImmutableArray<Conversion> underlyingElementConversions1,
TypeSymbol t2, CollectionExpressionTypeKind kind2, TypeSymbol elementType2, ImmutableArray<Conversion> underlyingElementConversions2,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
TypeSymbol elementType1;
var kind1 = conv1.GetCollectionExpressionTypeKind(out elementType1, out _, out _);
TypeSymbol elementType2;
var kind2 = conv2.GetCollectionExpressionTypeKind(out elementType2, out _, out _);
// Given:
// - `E` is a collection expression with element expressions `[EL₁, EL₂, ..., ELₙ]`
// - `T₁` and `T₂` are collection types
// - `E₁` is the element type of `T₁`
// - `E₂` is the element type of `T₂`
// - `CE₁ᵢ` are the series of conversions from `ELᵢ` to `E₁`
// - `CE₂ᵢ` are the series of conversions from `ELᵢ` to `E₂`

var t1IsSpanType = kind1 is CollectionExpressionTypeKind.ReadOnlySpan or CollectionExpressionTypeKind.Span;
var t2IsSpanType = kind2 is CollectionExpressionTypeKind.ReadOnlySpan or CollectionExpressionTypeKind.Span;

// `C₁` is a ***better collection conversion from expression*** than `C₂` if:
// - Both T₁ and T₂ are not *span types*, and `T₁` is implicitly convertible to `T₂`, and `T₂` is not implicitly convertible to `T₁`, or
if (!t1IsSpanType && !t2IsSpanType)
{
var t1IsConvertibleToT2 = Conversions.ClassifyImplicitConversionFromType(t1, t2, ref useSiteInfo).IsImplicit;
var t2IsConvertibleToT1 = Conversions.ClassifyImplicitConversionFromType(t2, t1, ref useSiteInfo).IsImplicit;

switch (t1IsConvertibleToT2, t2IsConvertibleToT1)
{
case (true, false):
return BetterResult.Left;
case (false, true):
return BetterResult.Right;
}
}

// - `E₁` does not have an identity conversion to `E₂`, and the element conversions to `E₁` are better than the element conversions to `E₂`, or
// - `E₁` has an identity conversion to `E₂`, and one of the following holds:

// `E₁` is compared to `E₂` as follows:
// If there is an identity conversion from `E₁` to `E₂`, then the element conversions are as good as each other. Otherwise, the element conversions
// to `E₁` are better than the element conversions to `E₂` if:
// - For every `ELᵢ`, `CE₁ᵢ` is at least as good as `CE₂ᵢ`, and
// - There is at least one i where `CE₁ᵢ` is better than `CE₂ᵢ`
// Otherwise, neither set of element conversions is better than the other, and they are also not as good as each other.
// 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.

if (!Conversions.HasIdentityConversion(elementType1, elementType2))
{
var betterResult = BetterResult.Neither;
Debug.Assert(underlyingElementConversions1.Length == underlyingElementConversions2.Length && underlyingElementConversions1.Length == collectionExpressionElements.Length);

for (int i = 0; i < underlyingElementConversions1.Length; i++)
{
// 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 element = collectionExpressionElements[i];
var conversionToE1 = underlyingElementConversions1[i];
var conversionToE2 = underlyingElementConversions2[i];

return IsBetterCollectionExpressionConversion(t1, kind1, elementType1, t2, kind2, elementType2, ref useSiteInfo);
BetterResult elementBetterResult;
if (element is BoundCollectionExpressionSpreadElement spread)
{
elementBetterResult = BetterConversionTarget(spread, elementType1, conversionToE1, elementType2, conversionToE2, ref useSiteInfo, okToDowngradeToNeither: out _);
}
else
{
elementBetterResult = BetterConversionFromExpression((BoundExpression)element, elementType1, conversionToE1, elementType2, conversionToE2, ref useSiteInfo, okToDowngradeToNeither: out _);
}

if (elementBetterResult == BetterResult.Neither)
{
continue;
}

if (betterResult != BetterResult.Neither)
{
if (betterResult != elementBetterResult)
{
return BetterResult.Neither;
}
}
else
{
betterResult = elementBetterResult;
}
}

return betterResult;
}

// - `T₁` is `System.ReadOnlySpan<E₁>`, and `T₂` is `System.Span<E₂>`, or
// - `T₁` is `System.ReadOnlySpan<E₁>` or `System.Span<E₁>`, and `T₂` is an *array_or_array_interface* with *element type* `E₂`

if (t1IsSpanType || t2IsSpanType)
{
switch ((kind1, kind2))
{
case (CollectionExpressionTypeKind.ReadOnlySpan, CollectionExpressionTypeKind.Span):
case (CollectionExpressionTypeKind.ReadOnlySpan or CollectionExpressionTypeKind.Span, _) when IsSZArrayOrArrayInterface(t2, out _):
return BetterResult.Left;

case (CollectionExpressionTypeKind.Span, CollectionExpressionTypeKind.ReadOnlySpan):
case (_, CollectionExpressionTypeKind.ReadOnlySpan or CollectionExpressionTypeKind.Span) when IsSZArrayOrArrayInterface(t1, out _):
return BetterResult.Right;
}
}

return BetterResult.Neither;
}

private bool IsBetterCollectionExpressionConversion(
private bool IsBetterCollectionExpressionConversion_CSharp12(
TypeSymbol t1, CollectionExpressionTypeKind kind1, TypeSymbol elementType1,
TypeSymbol t2, CollectionExpressionTypeKind kind2, TypeSymbol elementType2,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
Expand Down Expand Up @@ -2914,12 +3033,26 @@ bool hasImplicitConversion(TypeSymbol source, TypeSymbol destination, ref Compou
Conversions.ClassifyImplicitConversionFromType(source, destination, ref useSiteInfo).IsImplicit;
}

private bool IsBetterParamsCollectionType(TypeSymbol t1, TypeSymbol t2, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
private BetterResult BetterParamsCollectionType(TypeSymbol t1, TypeSymbol t2, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
CollectionExpressionTypeKind kind1 = ConversionsBase.GetCollectionExpressionTypeKind(Compilation, t1, out TypeWithAnnotations elementType1);
CollectionExpressionTypeKind kind2 = ConversionsBase.GetCollectionExpressionTypeKind(Compilation, t2, out TypeWithAnnotations elementType2);

return IsBetterCollectionExpressionConversion(t1, kind1, elementType1.Type, t2, kind2, elementType2.Type, ref useSiteInfo);
if (kind1 is CollectionExpressionTypeKind.CollectionBuilder or CollectionExpressionTypeKind.ImplementsIEnumerable)
{
_binder.TryGetCollectionIterationType(CSharpSyntaxTree.Dummy.GetRoot(), t1, out elementType1);
}

if (kind2 is CollectionExpressionTypeKind.CollectionBuilder or CollectionExpressionTypeKind.ImplementsIEnumerable)
{
_binder.TryGetCollectionIterationType(CSharpSyntaxTree.Dummy.GetRoot(), t2, out elementType2);
}

return BetterCollectionExpressionConversion(
collectionExpressionElements: [],
t1, kind1, elementType1.Type, underlyingElementConversions1: [],
t2, kind2, elementType2.Type, underlyingElementConversions2: [],
ref useSiteInfo);
}

private static bool IsSZArrayOrArrayInterface(TypeSymbol type, out TypeSymbol elementType)
Expand Down Expand Up @@ -3142,7 +3275,7 @@ private BetterResult BetterConversionTargetCore(
}

private BetterResult BetterConversionTarget(
BoundExpression node,
BoundNode node,
TypeSymbol type1,
Conversion conv1,
TypeSymbol type2,
Expand All @@ -3154,7 +3287,7 @@ private BetterResult BetterConversionTarget(
}

private BetterResult BetterConversionTargetCore(
BoundExpression node,
BoundNode node,
TypeSymbol type1,
Conversion conv1,
TypeSymbol type2,
Expand Down
Loading

0 comments on commit 433ae40

Please sign in to comment.