Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explicitly filter down to constructible types before offering fixes #69542

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Explicitly filter down to constructible types before offering fixes #69542

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
39408a4
Explicitly filter down to constructible types before offering fixes
CyrusNajmabadi Aug 16, 2023
43ac523
simplify
CyrusNajmabadi Aug 16, 2023
ac98875
REfined check
CyrusNajmabadi Aug 16, 2023
7ed2977
fix test
CyrusNajmabadi Aug 16, 2023
f17aab8
rename
CyrusNajmabadi Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 47 additions & 22 deletions src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ I can't find a specific location where abstract types are disallowed. In most cases, the parameterless constructor of an abstract type would not be accessible, but there are specific cases where it is (e.g. if you are currently in the implementation of a derived type, or if the constructor has been explicitly declared public).

}
}

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
Open in desktop
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 collectionType = """

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|];
}
}
""" + collectionType,
FixedCode = """
using System;
using System.Collections;
using System.Collections.Generic;

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

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

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|];
}
}
""" + collectionType,
LanguageVersion = LanguageVersion.CSharp12,
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

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

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|];
}
}
""" + collectionType,
FixedCode = """
using System;
using System.Collections;
using System.Collections.Generic;

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

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