Skip to content

Commit 37ec9cd

Browse files
CyrusNajmabadiCyrusNajmabadi
authored
Fix issue where we were changing semantics when converting to a collection expr. (#77417)
Fixes #77416
2 parents 7809289 + d87dbf4 commit 37ec9cd

9 files changed

Lines changed: 121 additions & 28 deletions

File tree

‎src/Analyzers/CSharp/Analyzers/UseCollectionInitializer/CSharpUseCollectionInitializerAnalyzer.cs‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,11 @@ protected override bool HasExistingInvalidInitializerForCollection()
4848
protected override bool AnalyzeMatchesAndCollectionConstructorForCollectionExpression(
4949
ArrayBuilder<CollectionMatch<SyntaxNode>> preMatches,
5050
ArrayBuilder<CollectionMatch<SyntaxNode>> postMatches,
51+
out bool mayChangeSemantics,
5152
CancellationToken cancellationToken)
5253
{
54+
mayChangeSemantics = false;
55+
5356
// Constructor wasn't called with any arguments. Nothing to validate.
5457
var argumentList = _objectCreationExpression.ArgumentList;
5558
if (argumentList is null || argumentList.Arguments.Count == 0)
@@ -63,20 +66,21 @@ protected override bool AnalyzeMatchesAndCollectionConstructorForCollectionExpre
6366
if (this.SemanticModel.GetSymbolInfo(_objectCreationExpression, cancellationToken).Symbol is not IMethodSymbol
6467
{
6568
MethodKind: MethodKind.Constructor,
66-
Parameters.Length: 1,
69+
Parameters: [var firstParameter],
6770
} constructor)
6871
{
6972
return false;
7073
}
7174

72-
var ienumerableOfTType = this.SemanticModel.Compilation.IEnumerableOfTType();
73-
var firstParameter = constructor.Parameters[0];
74-
if (Equals(firstParameter.Type.OriginalDefinition, ienumerableOfTType) ||
75-
firstParameter.Type.AllInterfaces.Any(i => Equals(i.OriginalDefinition, ienumerableOfTType)))
75+
if (CanSpreadFirstParameter(constructor.ContainingType, firstParameter))
7676
{
7777
// Took a single argument that implements IEnumerable<T>. We handle this by spreading that argument as the
7878
// first thing added to the collection.
7979
preMatches.Add(new(argumentList.Arguments[0].Expression, UseSpread: true));
80+
81+
// Can't be certain that spreading the elements will be the same as passing to the constructor. So pass
82+
// that uncertainty up to the caller so they can inform the user.
83+
mayChangeSemantics = true;
8084
return true;
8185
}
8286
else if (firstParameter is { Type.SpecialType: SpecialType.System_Int32, Name: "capacity" })
@@ -195,5 +199,36 @@ protected override bool AnalyzeMatchesAndCollectionConstructorForCollectionExpre
195199
}
196200

197201
return false;
202+
203+
bool CanSpreadFirstParameter(INamedTypeSymbol constructedType, IParameterSymbol firstParameter)
204+
{
205+
var compilation = this.SemanticModel.Compilation;
206+
207+
var ienumerableOfTType = compilation.IEnumerableOfTType();
208+
if (!Equals(firstParameter.Type.OriginalDefinition, ienumerableOfTType) &&
209+
!firstParameter.Type.AllInterfaces.Any(i => Equals(i.OriginalDefinition, ienumerableOfTType)))
210+
{
211+
return false;
212+
}
213+
214+
// Looks like something passed to the constructor call that we could potentially spread instead. e.g. `new
215+
// HashSet(someList)` can become `[.. someList]`. However, check for certain cases we know where this is
216+
// wrong and we can't do this.
217+
218+
// BlockingCollection<T> and Collection<T> both take ownership of the collection passed to them. So adds to
219+
// them will add through to the original collection. They do not take the original collection and add their
220+
// elements to itself.
221+
222+
var collectionType = compilation.CollectionOfTType();
223+
var blockingCollectionType = compilation.BlockingCollectionOfTType();
224+
if (constructedType.GetBaseTypesAndThis().Any(
225+
t => Equals(collectionType, t.OriginalDefinition) ||
226+
Equals(blockingCollectionType, t.OriginalDefinition)))
227+
{
228+
return false;
229+
}
230+
231+
return true;
232+
}
198233
}
199234
}

‎src/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests.cs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using Microsoft.CodeAnalysis.CSharp.UseCollectionInitializer;
88
using Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions;
99
using Microsoft.CodeAnalysis.Test.Utilities;
10+
using Microsoft.CodeAnalysis.Testing;
1011
using Roslyn.Test.Utilities;
1112
using Xunit;
1213

@@ -1826,4 +1827,42 @@ void M(List<int>? list1)
18261827
LanguageVersion = LanguageVersion.CSharp12,
18271828
}.RunAsync();
18281829
}
1830+
1831+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77416")]
1832+
public async Task TestNoCollectionExpressionForBlockingCollection()
1833+
{
1834+
await new VerifyCS.Test
1835+
{
1836+
TestCode = """
1837+
using System;
1838+
using System.Collections.Concurrent;
1839+
1840+
class A
1841+
{
1842+
public void Main(ConcurrentQueue<int> queue)
1843+
{
1844+
BlockingCollection<int> bc = [|new|](queue);
1845+
[|bc.Add(|]42);
1846+
}
1847+
}
1848+
""",
1849+
FixedCode = """
1850+
using System;
1851+
using System.Collections.Concurrent;
1852+
1853+
class A
1854+
{
1855+
public void Main(ConcurrentQueue<int> queue)
1856+
{
1857+
BlockingCollection<int> bc = new(queue)
1858+
{
1859+
42
1860+
};
1861+
}
1862+
}
1863+
""",
1864+
LanguageVersion = LanguageVersion.CSharp13,
1865+
ReferenceAssemblies = ReferenceAssemblies.Net.Net90,
1866+
}.RunAsync();
1867+
}
18291868
}

‎src/Analyzers/Core/Analyzers/UseCollectionInitializer/AbstractObjectCreationExpressionAnalyzer.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ internal abstract class AbstractObjectCreationExpressionAnalyzer<
3737
{
3838
public readonly record struct AnalysisResult(
3939
ImmutableArray<TMatch> PreMatches,
40-
ImmutableArray<TMatch> PostMatches);
40+
ImmutableArray<TMatch> PostMatches,
41+
bool ChangesSemantics);
4142

4243
protected UpdateExpressionState<TExpressionSyntax, TStatementSyntax> State;
4344

@@ -48,7 +49,7 @@ public readonly record struct AnalysisResult(
4849
protected SemanticModel SemanticModel => this.State.SemanticModel;
4950

5051
protected abstract bool ShouldAnalyze(CancellationToken cancellationToken);
51-
protected abstract bool TryAddMatches(ArrayBuilder<TMatch> preMatches, ArrayBuilder<TMatch> postMatches, CancellationToken cancellationToken);
52+
protected abstract bool TryAddMatches(ArrayBuilder<TMatch> preMatches, ArrayBuilder<TMatch> postMatches, out bool changesSemantics, CancellationToken cancellationToken);
5253
protected abstract bool IsInitializerOfLocalDeclarationStatement(
5354
TLocalDeclarationStatementSyntax localDeclarationStatement, TObjectCreationExpressionSyntax rootExpression, [NotNullWhen(true)] out TVariableDeclaratorSyntax? variableDeclarator);
5455

@@ -87,10 +88,10 @@ protected AnalysisResult AnalyzeWorker(CancellationToken cancellationToken)
8788

8889
using var _1 = ArrayBuilder<TMatch>.GetInstance(out var preMatches);
8990
using var _2 = ArrayBuilder<TMatch>.GetInstance(out var postMatches);
90-
if (!TryAddMatches(preMatches, postMatches, cancellationToken))
91+
if (!TryAddMatches(preMatches, postMatches, out var mayChangeSemantics, cancellationToken))
9192
return default;
9293

93-
return new(preMatches.ToImmutableAndClear(), postMatches.ToImmutableAndClear());
94+
return new(preMatches.ToImmutableAndClear(), postMatches.ToImmutableAndClear(), mayChangeSemantics);
9495
}
9596

9697
protected UpdateExpressionState<TExpressionSyntax, TStatementSyntax>? TryInitializeState(

‎src/Analyzers/Core/Analyzers/UseCollectionInitializer/AbstractUseCollectionInitializerAnalyzer.cs‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,10 @@ internal abstract class AbstractUseCollectionInitializerAnalyzer<
5050
protected abstract bool IsComplexElementInitializer(SyntaxNode expression);
5151
protected abstract bool HasExistingInvalidInitializerForCollection();
5252
protected abstract bool AnalyzeMatchesAndCollectionConstructorForCollectionExpression(
53-
ArrayBuilder<CollectionMatch<SyntaxNode>> preMatches, ArrayBuilder<CollectionMatch<SyntaxNode>> postMatches, CancellationToken cancellationToken);
53+
ArrayBuilder<CollectionMatch<SyntaxNode>> preMatches,
54+
ArrayBuilder<CollectionMatch<SyntaxNode>> postMatches,
55+
out bool mayChangeSemantics,
56+
CancellationToken cancellationToken);
5457

5558
protected abstract IUpdateExpressionSyntaxHelper<TExpressionSyntax, TStatementSyntax> SyntaxHelper { get; }
5659

@@ -66,7 +69,7 @@ public AnalysisResult Analyze(
6669
return default;
6770

6871
this.Initialize(state.Value, objectCreationExpression, analyzeForCollectionExpression);
69-
var (preMatches, postMatches) = this.AnalyzeWorker(cancellationToken);
72+
var (preMatches, postMatches, mayChangeSemantics) = this.AnalyzeWorker(cancellationToken);
7073

7174
// If analysis failed entirely, immediately bail out.
7275
if (preMatches.IsDefault || postMatches.IsDefault)
@@ -81,15 +84,19 @@ public AnalysisResult Analyze(
8184
// other words, we don't want to suggest changing `new List<int>()` to `new List<int>() { }` as that's just
8285
// noise. So convert empty results to an invalid result here.
8386
if (analyzeForCollectionExpression)
84-
return new(preMatches, postMatches);
87+
return new(preMatches, postMatches, mayChangeSemantics);
8588

8689
// Downgrade an empty result to a failure for the normal collection-initializer case.
87-
return postMatches.IsEmpty ? default : new(preMatches, postMatches);
90+
return postMatches.IsEmpty ? default : new(preMatches, postMatches, mayChangeSemantics);
8891
}
8992

9093
protected sealed override bool TryAddMatches(
91-
ArrayBuilder<CollectionMatch<SyntaxNode>> preMatches, ArrayBuilder<CollectionMatch<SyntaxNode>> postMatches, CancellationToken cancellationToken)
94+
ArrayBuilder<CollectionMatch<SyntaxNode>> preMatches,
95+
ArrayBuilder<CollectionMatch<SyntaxNode>> postMatches,
96+
out bool mayChangeSemantics,
97+
CancellationToken cancellationToken)
9298
{
99+
mayChangeSemantics = false;
93100
var seenInvocation = false;
94101
var seenIndexAssignment = false;
95102

@@ -127,7 +134,10 @@ protected sealed override bool TryAddMatches(
127134
}
128135

129136
if (_analyzeForCollectionExpression)
130-
return AnalyzeMatchesAndCollectionConstructorForCollectionExpression(preMatches, postMatches, cancellationToken);
137+
{
138+
return AnalyzeMatchesAndCollectionConstructorForCollectionExpression(
139+
preMatches, postMatches, out mayChangeSemantics, cancellationToken);
140+
}
131141

132142
return true;
133143
}

‎src/Analyzers/Core/Analyzers/UseCollectionInitializer/AbstractUseCollectionInitializerDiagnosticAnalyzer.cs‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,7 @@ public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
6868
isUnnecessary: true);
6969

7070
protected AbstractUseCollectionInitializerDiagnosticAnalyzer()
71-
: base(
72-
[
73-
(s_descriptor, CodeStyleOptions2.PreferCollectionInitializer)
74-
])
71+
: base([(s_descriptor, CodeStyleOptions2.PreferCollectionInitializer)])
7572
{
7673
}
7774

@@ -117,9 +114,9 @@ private void OnCompilationStart(CompilationStartAnalysisContext context)
117114
// as a non-local diagnostic and would not participate in lightbulb for computing code fixes.
118115
var expressionType = context.Compilation.ExpressionOfTType();
119116
context.RegisterCodeBlockStartAction<TSyntaxKind>(blockStartContext =>
120-
blockStartContext.RegisterSyntaxNodeAction(
121-
nodeContext => AnalyzeNode(nodeContext, ienumerableType, expressionType),
122-
matchKindsArray));
117+
blockStartContext.RegisterSyntaxNodeAction(
118+
nodeContext => AnalyzeNode(nodeContext, ienumerableType, expressionType),
119+
matchKindsArray));
123120
}
124121

125122
private void AnalyzeNode(
@@ -206,13 +203,13 @@ private void AnalyzeNode(
206203
if (!preferInitializerOption.Value)
207204
return null;
208205

209-
var (_, matches) = analyzer.Analyze(semanticModel, syntaxFacts, objectCreationExpression, analyzeForCollectionExpression: false, cancellationToken);
206+
var (_, matches, changesSemantics) = analyzer.Analyze(semanticModel, syntaxFacts, objectCreationExpression, analyzeForCollectionExpression: false, cancellationToken);
210207

211208
// If analysis failed, we can't change this, no matter what.
212209
if (matches.IsDefault)
213210
return null;
214211

215-
return (matches, shouldUseCollectionExpression: false, changesSemantics: false);
212+
return (matches, shouldUseCollectionExpression: false, changesSemantics);
216213
}
217214

218215
(ImmutableArray<CollectionMatch<SyntaxNode>> matches, bool shouldUseCollectionExpression, bool changesSemantics)? GetCollectionExpressionMatches()
@@ -224,18 +221,18 @@ private void AnalyzeNode(
224221
if (!this.AreCollectionExpressionsSupported(context.Compilation))
225222
return null;
226223

227-
var (preMatches, postMatches) = analyzer.Analyze(semanticModel, syntaxFacts, objectCreationExpression, analyzeForCollectionExpression: true, cancellationToken);
224+
var (preMatches, postMatches, changesSemantics1) = analyzer.Analyze(semanticModel, syntaxFacts, objectCreationExpression, analyzeForCollectionExpression: true, cancellationToken);
228225

229226
// If analysis failed, we can't change this, no matter what.
230227
if (preMatches.IsDefault || postMatches.IsDefault)
231228
return null;
232229

233230
// Check if it would actually be legal to use a collection expression here though.
234231
var allowSemanticsChange = preferExpressionOption.Value == CollectionExpressionPreference.WhenTypesLooselyMatch;
235-
if (!CanUseCollectionExpression(semanticModel, objectCreationExpression, expressionType, preMatches, allowSemanticsChange, cancellationToken, out var changesSemantics))
232+
if (!CanUseCollectionExpression(semanticModel, objectCreationExpression, expressionType, preMatches, allowSemanticsChange, cancellationToken, out var changesSemantics2))
236233
return null;
237234

238-
return (preMatches.Concat(postMatches), shouldUseCollectionExpression: true, changesSemantics);
235+
return (preMatches.Concat(postMatches), shouldUseCollectionExpression: true, changesSemantics1 || changesSemantics2);
239236
}
240237
}
241238

‎src/Analyzers/Core/Analyzers/UseObjectInitializer/UseNamedMemberInitializerAnalyzer.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,10 @@ protected sealed override bool ShouldAnalyze(CancellationToken cancellationToken
6969
protected sealed override bool TryAddMatches(
7070
ArrayBuilder<Match<TExpressionSyntax, TStatementSyntax, TMemberAccessExpressionSyntax, TAssignmentStatementSyntax>> preMatches,
7171
ArrayBuilder<Match<TExpressionSyntax, TStatementSyntax, TMemberAccessExpressionSyntax, TAssignmentStatementSyntax>> postMatches,
72+
out bool changesSemantics,
7273
CancellationToken cancellationToken)
7374
{
75+
changesSemantics = false;
7476
using var _1 = PooledHashSet<string>.GetInstance(out var seenNames);
7577

7678
var initializer = this.SyntaxFacts.GetInitializerOfBaseObjectCreationExpression(_objectCreationExpression);

‎src/Analyzers/Core/CodeFixes/UseCollectionInitializer/AbstractUseCollectionInitializerCodeFixProvider.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ protected sealed override async Task FixAsync(
7676
using var analyzer = GetAnalyzer();
7777

7878
var useCollectionExpression = properties.ContainsKey(UseCollectionInitializerHelpers.UseCollectionExpressionName) is true;
79-
var (preMatches, postMatches) = analyzer.Analyze(
79+
var (preMatches, postMatches, _) = analyzer.Analyze(
8080
semanticModel, syntaxFacts, objectCreation, useCollectionExpression, cancellationToken);
8181

8282
if (preMatches.IsDefault || postMatches.IsDefault)

‎src/Analyzers/VisualBasic/Analyzers/UseCollectionInitializer/VisualBasicCollectionInitializerAnalyzer.vb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.UseCollectionInitializer
4141
Protected Overrides Function AnalyzeMatchesAndCollectionConstructorForCollectionExpression(
4242
preMatches As ArrayBuilder(Of CollectionMatch(Of SyntaxNode)),
4343
postMatches As ArrayBuilder(Of CollectionMatch(Of SyntaxNode)),
44+
ByRef changesSemantics As Boolean,
4445
cancellationToken As CancellationToken) As Boolean
4546
' Only called for collection expressions, which VB does not support
4647
Throw ExceptionUtilities.Unreachable()

‎src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ICompilationExtensions.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System;
6+
using System.Collections.Concurrent;
67
using System.Collections.Generic;
78
using System.Collections.Immutable;
9+
using System.Collections.ObjectModel;
810
using System.ComponentModel;
911
using System.Diagnostics;
1012
using System.Diagnostics.CodeAnalysis;
@@ -81,6 +83,12 @@ public static ImmutableArray<IAssemblySymbol> GetReferencedAssemblySymbols(this
8183
public static INamedTypeSymbol? AttributeType(this Compilation compilation)
8284
=> compilation.GetTypeByMetadataName(typeof(Attribute).FullName!);
8385

86+
public static INamedTypeSymbol? BlockingCollectionOfTType(this Compilation compilation)
87+
=> compilation.GetTypeByMetadataName(typeof(BlockingCollection<>).FullName!);
88+
89+
public static INamedTypeSymbol? CollectionOfTType(this Compilation compilation)
90+
=> compilation.GetTypeByMetadataName(typeof(Collection<>).FullName!);
91+
8492
public static INamedTypeSymbol? ExceptionType(this Compilation compilation)
8593
=> compilation.GetTypeByMetadataName(typeof(Exception).FullName!);
8694

0 commit comments

Comments
 (0)