Skip to content

Commit

Permalink
Nullable conversion of collection expressions (#69852)
Browse files Browse the repository at this point in the history
  • Loading branch information
jcouv authored Sep 14, 2023
1 parent 819625f commit 7808feb
Show file tree
Hide file tree
Showing 14 changed files with 1,175 additions and 108 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -72,43 +72,53 @@ public static bool CanReplaceWithCollectionExpression(
return false;
}

// Looks good as something to replace. Now check the semantics of making the replacement to see if there would
// any issues. To keep things simple, all we do is replace the existing expression with the `[]` literal. This
// is an 'untyped' collection expression literal, so it tells us if the new code will have any issues moving to
// something untyped. This will also tell us if we have any ambiguities (because there are multiple destination
// types that could accept the collection expression).
var speculationAnalyzer = new SpeculationAnalyzer(
topMostExpression,
s_emptyCollectionExpression,
semanticModel,
cancellationToken,
skipVerificationForReplacedNode,
failOnOverloadResolutionFailuresInOriginalCode: true);

if (speculationAnalyzer.ReplacementChangesSemantics())
return false;
// HACK: Workaround lack of compiler information for collection expression conversions with casts.
// Specifically, hardcode in knowledge that a cast to a constructible collection type of the empty collection
// expression will always succeed, and there's no need to actually validate semantics there.
// Tracked by https://github.com/dotnet/roslyn/issues/68826
if (!IsAlwaysSafeCastReplacement())
{
// Looks good as something to replace. Now check the semantics of making the replacement to see if there would
// any issues. To keep things simple, all we do is replace the existing expression with the `[]` literal. This
// is an 'untyped' collection expression literal, so it tells us if the new code will have any issues moving to
// something untyped. This will also tell us if we have any ambiguities (because there are multiple destination
// types that could accept the collection expression).
var speculationAnalyzer = new SpeculationAnalyzer(
topMostExpression,
s_emptyCollectionExpression,
semanticModel,
cancellationToken,
skipVerificationForReplacedNode,
failOnOverloadResolutionFailuresInOriginalCode: true);

if (speculationAnalyzer.ReplacementChangesSemantics())
return false;

// Ensure that we have a collection conversion with the replacement. If not, this wasn't a legal replacement
// (for example, we're trying to replace an expression that is converted to something that isn't even a
// collection type).
//
// Note: an identity conversion is always legal without needing any more checks.
var conversion = speculationAnalyzer.SpeculativeSemanticModel.GetConversion(speculationAnalyzer.ReplacedExpression, cancellationToken);
if (conversion.IsIdentity)
return true;
// Ensure that we have a collection conversion with the replacement. If not, this wasn't a legal replacement
// (for example, we're trying to replace an expression that is converted to something that isn't even a
// collection type).
//
// Note: an identity conversion is always legal without needing any more checks.
var conversion = speculationAnalyzer.SpeculativeSemanticModel.GetConversion(speculationAnalyzer.ReplacedExpression, cancellationToken);
if (conversion.IsIdentity)
return true;

if (!conversion.IsCollectionExpression)
return false;
if (!conversion.IsCollectionExpression)
return false;

// The new expression's converted type has to equal the old expressions as well. Otherwise, we're now
// converting this to some different collection type unintentionally.
var replacedTypeInfo = speculationAnalyzer.SpeculativeSemanticModel.GetTypeInfo(speculationAnalyzer.ReplacedExpression, cancellationToken);
if (!originalTypeInfo.ConvertedType.Equals(replacedTypeInfo.ConvertedType))
return false;
// The new expression's converted type has to equal the old expressions as well. Otherwise, we're now
// converting this to some different collection type unintentionally.
var replacedTypeInfo = speculationAnalyzer.SpeculativeSemanticModel.GetTypeInfo(speculationAnalyzer.ReplacedExpression, cancellationToken);
if (!originalTypeInfo.ConvertedType.Equals(replacedTypeInfo.ConvertedType))
return false;
}

return true;

bool IsConstructibleCollectionType(ITypeSymbol type)
bool IsAlwaysSafeCastReplacement()
=> parent is CastExpressionSyntax && IsConstructibleCollectionType(semanticModel.GetTypeInfo(parent, cancellationToken).Type);

bool IsConstructibleCollectionType(ITypeSymbol? type)
{
// Arrays are always a valid collection expression type.
if (type is IArrayTypeSymbol)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,24 @@ void M()
}.RunAsync();
}

[Fact]
public async Task TestTargetTypedToComplexCast2()
{
await new VerifyCS.Test
{
TestCode = """
class C
{
void M()
{
var c = {|CS0030:(int)new int[] { 1 }|};
}
}
""",
LanguageVersion = LanguageVersion.CSharp12,
}.RunAsync();
}

[Fact]
public async Task TestNotTargetTypedWithIdentifierCast()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,64 @@ ImmutableArray<int> M()
}.RunAsync();
}

[Theory, MemberData(nameof(SuccessCreationPatterns))]
public async Task TestCast(string pattern)
{
await new VerifyCS.Test
{
TestCode = $$"""
using System.Collections.Immutable;
class C
{
void M()
{
{{pattern}}
[|builder.Add(|]0);
var v = (ImmutableArray<int>)builder.ToImmutable();
}
}
""" + s_arrayBuilderApi,
FixedCode = """
using System.Collections.Immutable;
class C
{
void M()
{
var v = (ImmutableArray<int>)[0];
}
}
""" + s_arrayBuilderApi,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

[Theory, MemberData(nameof(FailureCreationPatterns))]
public async Task TestIdentifierCast(string pattern)
{
await new VerifyCS.Test
{
TestCode = $$"""
using System.Collections.Immutable;
using X = System.Collections.Immutable.ImmutableArray<int>;
class C
{
void M()
{
{{pattern}}
builder.Add(0);
var v = (X)builder.ToImmutable();
}
}
""" + s_arrayBuilderApi,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

[Theory, MemberData(nameof(SuccessCreationPatterns))]
public async Task TestPassToArgument(string pattern)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,55 @@ class C
}.RunAsync();
}

[Fact]
public async Task TestCast()
{
await new VerifyCS.Test
{
TestCode = """
class C
{
void M()
{
var i = (MyCollection<int>)[|MyCollection.[|Create|]<int>(|]);
}
}
""" + s_collectionBuilderApi + s_basicCollectionApi,
FixedCode = """
class C
{
void M()
{
var i = (MyCollection<int>)[];
}
}
""" + s_collectionBuilderApi + s_basicCollectionApi,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net70,
}.RunAsync();
}

[Fact]
public async Task TestIdentifierCast()
{
await new VerifyCS.Test
{
TestCode = """
using X = MyCollection<int>;
class C
{
void M()
{
var i = (X)MyCollection.Create<int>();
}
}
""" + s_collectionBuilderApi + s_basicCollectionApi,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net70,
}.RunAsync();
}

[Fact]
public async Task TestOneElement()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,58 @@ void M()
}.RunAsync();
}

[Fact]
public async Task TestCast()
{
await new VerifyCS.Test
{
TestCode = """
using System;
class C
{
void M()
{
var v = (int[])Array.[|Empty|]<int>();
}
}
""",
FixedCode = """
using System;
class C
{
void M()
{
var v = (int[])[];
}
}
""",
LanguageVersion = LanguageVersion.CSharp12,
}.RunAsync();
}

[Fact]
public async Task TestIdentifierCast()
{
await new VerifyCS.Test
{
TestCode = """
using System;
using X = int[];
class C
{
void M()
{
var v = (X)Array.Empty<int>();
}
}
""",
LanguageVersion = LanguageVersion.CSharp12,
}.RunAsync();
}

[Fact]
public async Task TestTrivia()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,63 @@ void M()
}.RunAsync();
}

[Fact]
public async Task TestCast()
{
await new VerifyCS.Test
{
TestCode = """
using System.Linq;
using System.Collections.Generic;
class C
{
void M()
{
var list = (List<int>)new[] { 1, 2, 3 }.[|ToList|]();
}
}
""",
FixedCode = """
using System.Linq;
using System.Collections.Generic;
class C
{
void M()
{
var list = (List<int>)[1, 2, 3];
}
}
""",
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net70,
}.RunAsync();
}

[Fact]
public async Task TestIdentifierCast()
{
await new VerifyCS.Test
{
TestCode = """
using System.Linq;
using System.Collections.Generic;
using X = System.Collections.Generic.List<int>;
class C
{
void M()
{
var list = (X)new[] { 1, 2, 3 }.ToList();
}
}
""",
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net70,
}.RunAsync();
}

[Fact]
public async Task TestOnlyOnOutermost()
{
Expand Down
Loading

0 comments on commit 7808feb

Please sign in to comment.