From 39408a4d56e0c383c1fc296dbb3b787839057c15 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 16 Aug 2023 08:25:24 -0700 Subject: [PATCH 1/5] Explicitly filter down to constructible types before offering fixes --- .../UseCollectionExpressionHelpers.cs | 61 ++++++++++------- .../UseCollectionExpressionForEmptyTests.cs | 66 +++++++++++++++---- 2 files changed, 91 insertions(+), 36 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs b/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs index a82452bd5c2cd..94836419d3d09 100644 --- a/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs +++ b/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; using System.Threading; @@ -51,11 +52,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(compilation, originalTypeInfo.ConvertedType.OriginalDefinition)) return false; - } // Conservatively, avoid making this change if the original expression was itself converted. Consider, for // example, `IEnumerable x = new List()`. If we change that to `[]` we will still compile, @@ -104,29 +102,48 @@ 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). 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) + static bool IsConstructibleCollectionType(Compilation compilation, 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 and ReadOnlySpan 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 capacityConstructor = namedType.Constructors.FirstOrDefault(c => !c.IsStatic && c.Parameters is [{ Name: "capacity", Type.SpecialType: SpecialType.System_Int32 }]); + if (capacityConstructor != null && capacityConstructor.IsAccessibleWithin(compilation.Assembly)) + return true; + + var noArgConstructor = namedType.Constructors.FirstOrDefault(c => !c.IsStatic && c.Parameters.All(p => p.IsOptional || p.IsParams)); + if (noArgConstructor != null && noArgConstructor.IsAccessibleWithin(compilation.Assembly)) + { + // 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) 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; + } + } + } - // Otherwise, presume this is unsafe to use with collection expressions. + // Anything else is not constructible. return false; } } diff --git a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs index ec9e7c56ef4a1..340f6b2d37988 100644 --- a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs +++ b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs @@ -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 = """ @@ -670,19 +669,7 @@ class C { void M() { - ImmutableList v = ImmutableList.[|Empty|]; - } - } - """, - FixedCode = """ - using System; - using System.Collections.Immutable; - - class C - { - void M() - { - ImmutableList v = {|CS1729:[]|}; + ImmutableList v = ImmutableList.Empty; } } """, @@ -793,6 +780,57 @@ public void Add(T x) { } }.RunAsync(); } + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")] + public async Task ForValueTypeWithCapacityConstructor() + { + var collectionType = """ + + struct V : IEnumerable + { + public static readonly V Empty = default; + + public V(int capacity) { } + + public IEnumerator 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 v = V.[|Empty|]; + } + } + """ + collectionType, + FixedCode = """ + using System; + using System.Collections; + using System.Collections.Generic; + + class C + { + void M() + { + V v = []; + } + } + """ + collectionType, + LanguageVersion = LanguageVersion.CSharp12, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }.RunAsync(); + } + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")] public async Task ForValueTypeWithNoArgConstructorAndWithoutCollectionBuilderAttribute() { From 43ac52390c0a30cd87ccc05bdf233210e6390746 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 16 Aug 2023 08:26:14 -0700 Subject: [PATCH 2/5] simplify --- .../UseCollectionExpression/UseCollectionExpressionHelpers.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs b/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs index 94836419d3d09..c7c53d557955c 100644 --- a/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs +++ b/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using System; -using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; using System.Threading; From ac98875eeba3c5e464c4d58ded3152cb58399005 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 16 Aug 2023 08:39:36 -0700 Subject: [PATCH 3/5] REfined check --- .../UseCollectionExpressionHelpers.cs | 21 +++-- .../UseCollectionExpressionForEmptyTests.cs | 90 +++++++++++++++++++ 2 files changed, 105 insertions(+), 6 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs b/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs index c7c53d557955c..1141b27195ee1 100644 --- a/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs +++ b/src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs @@ -51,7 +51,7 @@ public static bool CanReplaceWithCollectionExpression( if (originalTypeInfo.ConvertedType is null or IErrorTypeSymbol) return false; - if (!IsConstructibleCollectionType(compilation, originalTypeInfo.ConvertedType.OriginalDefinition)) + if (!IsConstructibleCollectionType(originalTypeInfo.ConvertedType.OriginalDefinition)) return false; // Conservatively, avoid making this change if the original expression was itself converted. Consider, for @@ -101,7 +101,7 @@ public static bool CanReplaceWithCollectionExpression( return true; - static bool IsConstructibleCollectionType(Compilation compilation, ITypeSymbol type) + bool IsConstructibleCollectionType(ITypeSymbol type) { // Arrays are always a valid collection expression type. if (type is IArrayTypeSymbol) @@ -124,12 +124,15 @@ static bool IsConstructibleCollectionType(Compilation compilation, ITypeSymbol t if (namedType.AllInterfaces.Contains(compilation.IEnumerableType()!)) { // If they have an accessible `public C(int capacity)` constructor, the lang prefers calling that. - var capacityConstructor = namedType.Constructors.FirstOrDefault(c => !c.IsStatic && c.Parameters is [{ Name: "capacity", Type.SpecialType: SpecialType.System_Int32 }]); - if (capacityConstructor != null && capacityConstructor.IsAccessibleWithin(compilation.Assembly)) + var constructors = namedType.Constructors; + var capacityConstructor = GetAccessibleInstanceConstructor(constructors, c => c.Parameters is [{ Name: "capacity", Type.SpecialType: SpecialType.System_Int32 }]); + if (capacityConstructor != null) return true; - var noArgConstructor = namedType.Constructors.FirstOrDefault(c => !c.IsStatic && c.Parameters.All(p => p.IsOptional || p.IsParams)); - if (noArgConstructor != null && noArgConstructor.IsAccessibleWithin(compilation.Assembly)) + 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 @@ -145,6 +148,12 @@ static bool IsConstructibleCollectionType(Compilation compilation, ITypeSymbol t // Anything else is not constructible. return false; } + + IMethodSymbol? GetAccessibleInstanceConstructor(ImmutableArray constructors, Func 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) diff --git a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs index 340f6b2d37988..63eb3caa1e56e 100644 --- a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs +++ b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs @@ -831,6 +831,96 @@ void M() }.RunAsync(); } + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")] + public async Task NotForValueTypeWithInvalidCapacityConstructor() + { + var collectionType = """ + + struct V : IEnumerable + { + public static readonly V Empty = default; + + public V(string capacity) { } + + public IEnumerator 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 v = V.[|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 : IEnumerable + { + public static readonly V Empty = default; + + public V(string capacity) { } + public V() { } + + public IEnumerator 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 v = V.[|Empty|]; + } + } + """ + collectionType, + FixedCode = """ + using System; + using System.Collections; + using System.Collections.Generic; + + class C + { + void M() + { + V v = []; + } + } + """ + collectionType, + LanguageVersion = LanguageVersion.CSharp12, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }.RunAsync(); + } + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")] public async Task ForValueTypeWithNoArgConstructorAndWithoutCollectionBuilderAttribute() { From 7ed2977e90ec3b1a49fc3640b0c117926762f7af Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 16 Aug 2023 09:49:17 -0700 Subject: [PATCH 4/5] fix test --- .../UseCollectionExpressionForEmptyTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs index 63eb3caa1e56e..3a3fe0d9c2d74 100644 --- a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs +++ b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs @@ -860,7 +860,7 @@ class C { void M() { - V v = V.[|Empty|]; + V v = V.Empty; } } """ + collectionType, From f17aab8b747898ccd6d18c8253161b8deb304e91 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 16 Aug 2023 09:51:35 -0700 Subject: [PATCH 5/5] rename --- .../UseCollectionExpressionForEmptyTests.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs index 3a3fe0d9c2d74..a93e3f5964913 100644 --- a/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs +++ b/src/Analyzers/CSharp/Tests/UseCollectionExpression/UseCollectionExpressionForEmptyTests.cs @@ -783,7 +783,7 @@ public void Add(T x) { } [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")] public async Task ForValueTypeWithCapacityConstructor() { - var collectionType = """ + var collectionDefinition = """ struct V : IEnumerable { @@ -812,7 +812,7 @@ void M() V v = V.[|Empty|]; } } - """ + collectionType, + """ + collectionDefinition, FixedCode = """ using System; using System.Collections; @@ -825,7 +825,7 @@ void M() V v = []; } } - """ + collectionType, + """ + collectionDefinition, LanguageVersion = LanguageVersion.CSharp12, ReferenceAssemblies = ReferenceAssemblies.Net.Net80, }.RunAsync(); @@ -834,7 +834,7 @@ void M() [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")] public async Task NotForValueTypeWithInvalidCapacityConstructor() { - var collectionType = """ + var collectionDefinition = """ struct V : IEnumerable { @@ -863,7 +863,7 @@ void M() V v = V.Empty; } } - """ + collectionType, + """ + collectionDefinition, LanguageVersion = LanguageVersion.CSharp12, ReferenceAssemblies = ReferenceAssemblies.Net.Net80, }.RunAsync(); @@ -872,7 +872,7 @@ void M() [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69507")] public async Task ForValueTypeWithInvalidCapacityButValidEmptyConstructor() { - var collectionType = """ + var collectionDefinition = """ struct V : IEnumerable { @@ -902,7 +902,7 @@ void M() V v = V.[|Empty|]; } } - """ + collectionType, + """ + collectionDefinition, FixedCode = """ using System; using System.Collections; @@ -915,7 +915,7 @@ void M() V v = []; } } - """ + collectionType, + """ + collectionDefinition, LanguageVersion = LanguageVersion.CSharp12, ReferenceAssemblies = ReferenceAssemblies.Net.Net80, }.RunAsync();