Skip to content

Commit

Permalink
Merge pull request #69542 from CyrusNajmabadi/constructibleTypes
Browse files Browse the repository at this point in the history 
Explicitly filter down to constructible types before offering fixes
  • Loading branch information
@CyrusNajmabadi
CyrusNajmabadi authored Aug 16, 2023
2 parents 221323d + f17aab8 commit 3ffcdc4
Show file tree
Hide file tree
Showing 2 changed files with 189 additions and 36 deletions.
69 changes: 47 additions & 22 deletions src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,8 @@ public static bool CanReplaceWithCollectionExpression(
if (originalTypeInfo.ConvertedType is null or IErrorTypeSymbol)
return false;

if (originalTypeInfo.ConvertedType.OriginalDefinition is INamedTypeSymbol { IsValueType: true } structType &&
!IsValidStructType(compilation, structType))
{
if (!IsConstructibleCollectionType(originalTypeInfo.ConvertedType.OriginalDefinition))
return false;
}

// Conservatively, avoid making this change if the original expression was itself converted. Consider, for
// example, `IEnumerable<string> x = new List<string>()`. If we change that to `[]` we will still compile,
Expand Down Expand Up @@ -104,31 +101,59 @@ public static bool CanReplaceWithCollectionExpression(

return true;

// Special case. Block value types without an explicit no-arg constructor, which also do not have the
// collection-builder-attribute on them. This prevents us from offering to convert value-type-collections who
// are invalid in their `default` state (like ImmutableArray<T>). The presumption is that if the
// collection-initializer pattern is valid for these value types that they will supply an explicit constructor
// to initialize themselves.
static bool IsValidStructType(Compilation compilation, INamedTypeSymbol structType)
bool IsConstructibleCollectionType(ITypeSymbol type)
{
// Span<> and ReadOnlySpan<> are always valid targets.
if (structType.Equals(compilation.SpanOfTType()) || structType.Equals(compilation.ReadOnlySpanOfTType()))
// Arrays are always a valid collection expression type.
if (type is IArrayTypeSymbol)
return true;

// If we have a real no-arg constructor, then presume the type is intended to be new-ed up and used as a
// collection initializer.
var noArgConstructor = structType.Constructors.FirstOrDefault(c => !c.IsStatic && c.Parameters.Length == 0);
if (noArgConstructor is { IsImplicitlyDeclared: false })
return true;
// Has to be a real named type at this point.
if (type is INamedTypeSymbol namedType)
{
// Span<T> and ReadOnlySpan<T> are always valid collection expression types.
if (namedType.Equals(compilation.SpanOfTType()) || namedType.Equals(compilation.ReadOnlySpanOfTType()))
return true;

// If it has a [CollectionBuilder] attribute on it, it can definitely be used for a collection expression.
var collectionBuilderType = compilation.CollectionBuilderAttribute();
if (structType.GetAttributes().Any(a => a.AttributeClass?.Equals(collectionBuilderType) is true))
return true;
// If it has a [CollectionBuilder] attribute on it, it is a valid collection expression type.
var collectionBuilderType = compilation.CollectionBuilderAttribute();
if (namedType.GetAttributes().Any(a => a.AttributeClass?.Equals(collectionBuilderType) is true))
return true;

// At this point, all that is left are collection-initializer types. These need to derive from
// System.Collections.IEnumerable, and have an accessible no-arg constructor.
if (namedType.AllInterfaces.Contains(compilation.IEnumerableType()!))
{
// If they have an accessible `public C(int capacity)` constructor, the lang prefers calling that.
var constructors = namedType.Constructors;
var capacityConstructor = GetAccessibleInstanceConstructor(constructors, c => c.Parameters is [{ Name: "capacity", Type.SpecialType: SpecialType.System_Int32 }]);
if (capacityConstructor != null)
return true;

// Otherwise, presume this is unsafe to use with collection expressions.
var noArgConstructor =
GetAccessibleInstanceConstructor(constructors, c => c.Parameters.IsEmpty) ??
GetAccessibleInstanceConstructor(constructors, c => c.Parameters.All(p => p.IsOptional || p.IsParams));
if (noArgConstructor != null)
{
// If we have a struct, and the constructor we find is implicitly declared, don't consider this
// a constructible type. It's likely the user would just get the `default` instance of the
// collection (like with ImmutableArray<T>) which would then not actually work. If the struct
// does have an explicit constructor though, that's a good sign it can actually be constructed
// safely with the no-arg `new S()` call.
if (!(namedType.TypeKind == TypeKind.Struct && noArgConstructor.IsImplicitlyDeclared))
return true;
}
}
}

// Anything else is not constructible.
return false;
}

IMethodSymbol? GetAccessibleInstanceConstructor(ImmutableArray<IMethodSymbol> constructors, Func<IMethodSymbol, bool> predicate)
{
var constructor = constructors.FirstOrDefault(c => !c.IsStatic && predicate(c));
return constructor is not null && constructor.IsAccessibleWithin(compilation.Assembly) ? constructor : null;
}
}

private static bool IsInTargetTypedLocation(SemanticModel semanticModel, ExpressionSyntax expression, CancellationToken cancellationToken)
Expand Down
156 changes: 142 additions & 14 deletions src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,6 @@ void M()
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")]
public async Task NotForImmutableListNet70()
{
// This should fail once https://github.com/dotnet/roslyn/issues/69521 is fixed
await new VerifyCS.Test
{
TestCode = """
Expand All @@ -670,19 +669,7 @@ class C
{
void M()
{
ImmutableList<int> v = ImmutableList<int>.[|Empty|];
}
}
""",
FixedCode = """
using System;
using System.Collections.Immutable;

class C
{
void M()
{
ImmutableList<int> v = {|CS1729:[]|};
ImmutableList<int> v = ImmutableList<int>.Empty;
}
}
""",
Expand Down Expand Up @@ -793,6 +780,147 @@ public void Add(T x) { }
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")]
public async Task ForValueTypeWithCapacityConstructor()
{
var collectionDefinition = """

struct V<T> : IEnumerable<T>
{
public static readonly V<T> Empty = default;

public V(int capacity) { }

public IEnumerator<T> GetEnumerator() => default;
IEnumerator IEnumerable.GetEnumerator() => default;

public void Add(T x) { }
}
""";

await new VerifyCS.Test
{
TestCode = """
using System;
using System.Collections;
using System.Collections.Generic;

class C
{
void M()
{
V<int> v = V<int>.[|Empty|];
}
}
""" + collectionDefinition,
FixedCode = """
using System;
using System.Collections;
using System.Collections.Generic;

class C
{
void M()
{
V<int> v = [];
}
}
""" + collectionDefinition,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")]
public async Task NotForValueTypeWithInvalidCapacityConstructor()
{
var collectionDefinition = """

struct V<T> : IEnumerable<T>
{
public static readonly V<T> Empty = default;

public V(string capacity) { }

public IEnumerator<T> GetEnumerator() => default;
IEnumerator IEnumerable.GetEnumerator() => default;

public void Add(T x) { }
}
""";

await new VerifyCS.Test
{
TestCode = """
using System;
using System.Collections;
using System.Collections.Generic;

class C
{
void M()
{
V<int> v = V<int>.Empty;
}
}
""" + collectionDefinition,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")]
public async Task ForValueTypeWithInvalidCapacityButValidEmptyConstructor()
{
var collectionDefinition = """

struct V<T> : IEnumerable<T>
{
public static readonly V<T> Empty = default;

public V(string capacity) { }
public V() { }

public IEnumerator<T> GetEnumerator() => default;
IEnumerator IEnumerable.GetEnumerator() => default;

public void Add(T x) { }
}
""";

await new VerifyCS.Test
{
TestCode = """
using System;
using System.Collections;
using System.Collections.Generic;

class C
{
void M()
{
V<int> v = V<int>.[|Empty|];
}
}
""" + collectionDefinition,
FixedCode = """
using System;
using System.Collections;
using System.Collections.Generic;

class C
{
void M()
{
V<int> v = [];
}
}
""" + collectionDefinition,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")]
public async Task ForValueTypeWithNoArgConstructorAndWithoutCollectionBuilderAttribute()
{
Expand Down

0 comments on commit 3ffcdc4

Please sign in to comment.