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.

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

Detect more fluent cases for converting collection expressions #75267

Merged
merged 26 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@
<Compile Include="$(MSBuildThisFileDirectory)UseCoalesceExpression\CSharpUseCoalesceExpressionForNullableTernaryConditionalCheckDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCoalesceExpression\UseCoalesceExpressionHelpers.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CollectionExpressionMatch.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForBuilderDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForFluentDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForEmptyDiagnosticAnalyzer.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.CodeAnalysis.Shared.CodeStyle;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.UseCollectionExpression;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;
Expand Down Expand Up @@ -57,7 +58,7 @@ private void AnalyzeArrayCreationExpression(SyntaxNodeAnalysisContext context, I
ReportArrayCreationDiagnostics(context, syntaxTree, option.Notification, arrayCreationExpression, changesSemantics);
}

public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetMatches(
public static ImmutableArray<CollectionMatch<StatementSyntax>> TryGetMatches(
Copy link
Member Author

Choose a reason for hiding this comment

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

renamed this and replace another type with this type.

SemanticModel semanticModel,
ArrayCreationExpressionSyntax expression,
CollectionExpressionSyntax replacementExpression,
Expand Down Expand Up @@ -114,7 +115,7 @@ public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetM
return matches;
}

public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetMatches(
public static ImmutableArray<CollectionMatch<StatementSyntax>> TryGetMatches(
SemanticModel semanticModel,
ImplicitArrayCreationExpressionSyntax expression,
CollectionExpressionSyntax replacementExpression,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.CodeStyle;
using Microsoft.CodeAnalysis.UseCollectionExpression;
using Microsoft.CodeAnalysis.UseCollectionInitializer;
using Roslyn.Utilities;

Expand Down Expand Up @@ -159,7 +160,7 @@ void FadeOutCode(SyntaxNodeAnalysisContext context, AnalysisResult analysisResul
identifier,
initializedSymbol: semanticModel.GetDeclaredSymbol(declarator, cancellationToken));

using var _ = ArrayBuilder<Match<StatementSyntax>>.GetInstance(out var matches);
using var _ = ArrayBuilder<CollectionMatch<SyntaxNode>>.GetInstance(out var matches);

// Now walk all the statement after the local declaration.
using var enumerator = state.GetSubsequentStatements().GetEnumerator();
Expand Down Expand Up @@ -249,6 +250,6 @@ public readonly record struct AnalysisResult(
Location DiagnosticLocation,
LocalDeclarationStatementSyntax LocalDeclarationStatement,
InvocationExpressionSyntax CreationExpression,
ImmutableArray<Match<StatementSyntax>> Matches,
ImmutableArray<CollectionMatch<SyntaxNode>> Matches,
bool ChangesSemantics);
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using Microsoft.CodeAnalysis.Shared.CodeStyle;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.UseCollectionExpression;
using Microsoft.CodeAnalysis.UseCollectionInitializer;
using Roslyn.Utilities;

Expand Down Expand Up @@ -133,46 +134,57 @@ private void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, INamedTypeSy
{
// Because we're recursing from top to bottom in the expression tree, we build up the matches in reverse. Right
// before returning them, we'll reverse them again to get the proper order.
using var _ = ArrayBuilder<CollectionExpressionMatch<ArgumentSyntax>>.GetInstance(out var matchesInReverse);
if (!AnalyzeInvocation(text, state, invocation, addMatches ? matchesInReverse : null, out var existingInitializer, cancellationToken))
using var _1 = ArrayBuilder<CollectionMatch<ArgumentSyntax>>.GetInstance(out var preMatchesInReverse);
using var _2 = ArrayBuilder<CollectionMatch<ArgumentSyntax>>.GetInstance(out var postMatchesInReverse);
if (!AnalyzeInvocation(
text, state, invocation,
addMatches ? preMatchesInReverse : null,
addMatches ? postMatchesInReverse : null,
out var existingInitializer, cancellationToken))
{
return null;
}

if (!CanReplaceWithCollectionExpression(
state.SemanticModel, invocation, expressionType, isSingletonInstance: false, allowSemanticsChange, skipVerificationForReplacedNode: true, cancellationToken, out var changesSemantics))
{
return null;
}

matchesInReverse.ReverseContents();
return new AnalysisResult(existingInitializer, invocation, matchesInReverse.ToImmutable(), changesSemantics);
preMatchesInReverse.ReverseContents();
postMatchesInReverse.ReverseContents();
return new AnalysisResult(existingInitializer, invocation, preMatchesInReverse.ToImmutable(), postMatchesInReverse.ToImmutable(), changesSemantics);
}

private static bool AnalyzeInvocation(
SourceText text,
FluentState state,
InvocationExpressionSyntax invocation,
ArrayBuilder<CollectionExpressionMatch<ArgumentSyntax>>? matchesInReverse,
ArrayBuilder<CollectionMatch<ArgumentSyntax>>? preMatchesInReverse,
ArrayBuilder<CollectionMatch<ArgumentSyntax>>? postMatchesInReverse,
out InitializerExpressionSyntax? existingInitializer,
CancellationToken cancellationToken)
{
var semanticModel = state.SemanticModel;
var compilation = semanticModel.Compilation;

existingInitializer = null;
if (invocation.Expression is not MemberAccessExpressionSyntax memberAccess)
return false;

// Topmost invocation must be a syntactic match for one of our collection manipulation forms. At the top level
// we don't want to end with a linq method as that would be lazy, and a collection expression will eagerly
// realize the collection.
if (!IsMatch(state, memberAccess, invocation, allowLinq: false, matchesInReverse, out var isAdditionMatch, cancellationToken))
if (!IsMatch(state, memberAccess, invocation, allowLinq: false, postMatchesInReverse, out var isAdditionMatch, cancellationToken))
return false;

// We don't want to offer this feature on top of some builder-type. They will commonly end with something like
// `builder.ToImmutable()`. We want that case to be handled by the 'ForBuilder' analyzer instead.
var expressionType = state.SemanticModel.GetTypeInfo(memberAccess.Expression, cancellationToken).Type;
var expressionType = semanticModel.GetTypeInfo(memberAccess.Expression, cancellationToken).Type;
if (expressionType is null || expressionType.Name.EndsWith("Builder", StringComparison.Ordinal))
return false;

var semanticModel = state.SemanticModel;
var compilation = semanticModel.Compilation;
var ienumerableOfTType = compilation.IEnumerableOfTType();

using var _1 = ArrayBuilder<ExpressionSyntax>.GetInstance(out var stack);
stack.Push(memberAccess.Expression);
Expand All @@ -187,7 +199,7 @@ private static bool AnalyzeInvocation(
// left hand side of the expression. In the inner expressions we can have things like `.Concat/.Append`
// calls as the outer expressions will realize the collection.
if (current is InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax currentMemberAccess } currentInvocation &&
IsMatch(state, currentMemberAccess, currentInvocation, allowLinq: true, matchesInReverse, out _, cancellationToken))
IsMatch(state, currentMemberAccess, currentInvocation, allowLinq: true, postMatchesInReverse, out _, cancellationToken))
Copy link
Contributor

Choose a reason for hiding this comment

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

IsMatch

I think this would be easier for me to read if the while loop ended after this if block

Copy link
Member Author

Choose a reason for hiding this comment

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

will make that chnage.

Copy link
Member Author

Choose a reason for hiding this comment

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

this got much nicer. thanks.

{
copiedData = true;
stack.Push(currentMemberAccess.Expression);
Expand Down Expand Up @@ -224,21 +236,44 @@ private static bool AnalyzeInvocation(
{
if (objectCreation is not
{
ArgumentList: null or { Arguments.Count: 0 },
Initializer: null or { RawKind: (int)SyntaxKind.CollectionInitializerExpression }
})
{
return false;
}

existingInitializer = objectCreation.Initializer;

if (!IsLegalInitializer(objectCreation.Initializer))
return false;

if (!IsListLike(current))
return false;

existingInitializer = objectCreation.Initializer;
return true;
if (objectCreation.ArgumentList is { Arguments.Count: 1 })
{
// Can take a single argument if that argument is itself a collection.
var argumentType = semanticModel.GetTypeInfo(objectCreation.ArgumentList.Arguments[0].Expression, cancellationToken).Type;
if (argumentType is null)
return false;

if (!Equals(argumentType.OriginalDefinition, ienumerableOfTType) &&
!argumentType.AllInterfaces.Any(i => Equals(i.OriginalDefinition, ienumerableOfTType)))
{
return false;
}

// Add the arguments to the pre-matches. They will execute before the initializer values are added.
AddArgumentsInReverse(preMatchesInReverse, objectCreation.ArgumentList.Arguments, useSpread: true);
Copy link
Member Author

Choose a reason for hiding this comment

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

new logic. if the user wrote new List<...>(someValuesToCopy) we'll create a collection expr starting with [.. someValuesToCopy. Any existing initializer values will be added after this. And any more fluent additions will go after that.

return true;
}
else if (objectCreation.ArgumentList is null or { Arguments.Count: 0 })
{
// Otherwise, we have to have an empty argument list.
return true;
}

return false;
}

// Forms like `ImmutableArray.Create(...)` or `ImmutableArray.CreateRange(...)` are fine base cases.
Expand All @@ -248,7 +283,7 @@ private static bool AnalyzeInvocation(
if (!IsListLike(current))
return false;

AddArgumentsInReverse(matchesInReverse, GetArguments(currentInvocationExpression, unwrapArgument), useSpread: false);
AddArgumentsInReverse(postMatchesInReverse, GetArguments(currentInvocationExpression, unwrapArgument), useSpread: false);
return true;
}

Expand Down Expand Up @@ -277,13 +312,13 @@ private static bool AnalyzeInvocation(

void AddFinalMatch(ExpressionSyntax expression)
{
if (matchesInReverse is null)
if (postMatchesInReverse is null)
return;

// We're only adding one item to the final collection. So we're ending up with `[.. <expr>]`. If this
// originally was wrapped over multiple lines in a fluent fashion, and we're down to just a single wrapped
// line, then unwrap.
if (matchesInReverse.Count == 0 &&
if (postMatchesInReverse.Count == 0 &&
expression is InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax memberAccess } innerInvocation &&
text.Lines.GetLineFromPosition(expression.SpanStart).LineNumber + 1 == text.Lines.GetLineFromPosition(expression.Span.End).LineNumber &&
memberAccess.Expression.GetTrailingTrivia()
Expand All @@ -292,7 +327,7 @@ void AddFinalMatch(ExpressionSyntax expression)
.All(static t => t.IsWhitespaceOrEndOfLine()))
{
// Remove any whitespace around the `.`, making the singly-wrapped fluent expression into a single line.
matchesInReverse.Add(new CollectionExpressionMatch<ArgumentSyntax>(
postMatchesInReverse.Add(new CollectionMatch<ArgumentSyntax>(
Argument(innerInvocation.WithExpression(
memberAccess.Update(
memberAccess.Expression.WithoutTrailingTrivia(),
Expand All @@ -302,7 +337,7 @@ void AddFinalMatch(ExpressionSyntax expression)
return;
}

matchesInReverse.Add(new CollectionExpressionMatch<ArgumentSyntax>(Argument(expression), UseSpread: true));
postMatchesInReverse.Add(new CollectionMatch<ArgumentSyntax>(Argument(expression), UseSpread: true));
}

// We only want to offer this feature when the original collection was list-like (as opposed to being something
Expand Down Expand Up @@ -359,12 +394,13 @@ static bool IsLegalInitializer(InitializerExpressionSyntax? initializer)
return false;
}
}

return true;
}
}

private static void AddArgumentsInReverse(
ArrayBuilder<CollectionExpressionMatch<ArgumentSyntax>>? matchesInReverse,
ArrayBuilder<CollectionMatch<ArgumentSyntax>>? matchesInReverse,
SeparatedSyntaxList<ArgumentSyntax> arguments,
bool useSpread)
{
Expand All @@ -388,7 +424,7 @@ private static bool IsMatch(
MemberAccessExpressionSyntax memberAccess,
InvocationExpressionSyntax invocation,
bool allowLinq,
ArrayBuilder<CollectionExpressionMatch<ArgumentSyntax>>? matchesInReverse,
ArrayBuilder<CollectionMatch<ArgumentSyntax>>? matchesInReverse,
out bool isAdditionMatch,
CancellationToken cancellationToken)
{
Expand Down Expand Up @@ -467,12 +503,15 @@ static bool HasAnySuffix(string name)
/// help determine the best collection expression final syntax.</param>
/// <param name="CreationExpression">The location of the code like <c>builder.ToImmutable()</c> that will actually be
/// replaced with the collection expression</param>
/// <param name="Matches">The arguments being added to the collection that will be converted into elements in the
/// final collection expression.</param>
/// <param name="PreMatches">The arguments being added to the collection that will be converted into elements in
/// the final collection expression *before* the existing initializer elements.</param>
/// <param name="PostMatches">The arguments being added to the collection that will be converted into elements in
/// the final collection expression *after* the existing initializer elements.</param>
public readonly record struct AnalysisResult(
// Location DiagnosticLocation,
InitializerExpressionSyntax? ExistingInitializer,
InvocationExpressionSyntax CreationExpression,
ImmutableArray<CollectionExpressionMatch<ArgumentSyntax>> Matches,
ImmutableArray<CollectionMatch<ArgumentSyntax>> PreMatches,
ImmutableArray<CollectionMatch<ArgumentSyntax>> PostMatches,
bool ChangesSemantics);
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Shared.CodeStyle;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.UseCollectionExpression;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;

Expand Down Expand Up @@ -119,7 +120,7 @@ private void AnalyzeExplicitStackAllocExpression(SyntaxNodeAnalysisContext conte
additionalUnnecessaryLocations: additionalUnnecessaryLocations));
}

public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetMatches(
public static ImmutableArray<CollectionMatch<StatementSyntax>> TryGetMatches(
SemanticModel semanticModel,
StackAllocArrayCreationExpressionSyntax expression,
INamedTypeSymbol? expressionType,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.UseCollectionExpression;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;
Expand Down Expand Up @@ -814,7 +815,7 @@ private static bool ShouldReplaceExistingExpressionEntirely(
return false;
}

public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetMatches<TArrayCreationExpressionSyntax>(
public static ImmutableArray<CollectionMatch<StatementSyntax>> TryGetMatches<TArrayCreationExpressionSyntax>(
SemanticModel semanticModel,
TArrayCreationExpressionSyntax expression,
CollectionExpressionSyntax replacementExpression,
Expand All @@ -835,7 +836,7 @@ public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetM
if (getType(expression) is not ArrayTypeSyntax { RankSpecifiers: [{ Sizes: [var size] }, ..] })
return default;

using var _ = ArrayBuilder<CollectionExpressionMatch<StatementSyntax>>.GetInstance(out var matches);
using var _ = ArrayBuilder<CollectionMatch<StatementSyntax>>.GetInstance(out var matches);

var initializer = getInitializer(expression);
if (size is OmittedArraySizeExpressionSyntax)
Expand Down
Loading
Loading