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

Add support for generating the .. x ? [y] : [] pattern in a collection-expression #69280

Merged
merged 12 commits into from
Aug 2, 2023
1 change: 1 addition & 0 deletions src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForEmptyDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\UseCollectionExpressionHelpers.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionInitializer\CSharpUseCollectionInitializerAnalyzer.cs" />
Copy link
Member Author

Choose a reason for hiding this comment

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

there used to be a single UseCollectionInitializerAnalyzer type that was used for both VB and C#. HOwever, it was getting crushed under the weight of having to have special logic (esp. logic that was really only for C#'s collection expressions).

We now use inheritance here, which cleans up a few things (especially around type arguments).

<Compile Include="$(MSBuildThisFileDirectory)UseCompoundAssignment\CSharpUseCompoundAssignmentDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCompoundAssignment\CSharpUseCompoundCoalesceAssignmentDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCompoundAssignment\Utilities.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.UseCollectionInitializer;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionInitializer;

internal sealed class CSharpUseCollectionInitializerAnalyzer : AbstractUseCollectionInitializerAnalyzer<
ExpressionSyntax,
StatementSyntax,
BaseObjectCreationExpressionSyntax,
MemberAccessExpressionSyntax,
InvocationExpressionSyntax,
ExpressionStatementSyntax,
ForEachStatementSyntax,
IfStatementSyntax,
VariableDeclaratorSyntax,
CSharpUseCollectionInitializerAnalyzer>
{
protected override bool IsComplexElementInitializer(SyntaxNode expression)
=> expression.IsKind(SyntaxKind.ComplexElementInitializerExpression);

protected override void GetPartsOfForeachStatement(
ForEachStatementSyntax statement,
out SyntaxToken identifier,
out ExpressionSyntax expression,
out IEnumerable<StatementSyntax> statements)
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 is a better way than how i was previously doing things iwth ISYntaxFacts. The problem is that there are too many foreach-forms across C# and VB (and in C# itself). So syntaxfacts didn't really know how to deal with things.

Here though we know exactly what form we have, and we can produce a good output result for that for C# no matter what.

{
identifier = statement.Identifier;
expression = statement.Expression;
statements = ExtractEmbeddedStatements(statement.Statement);
}

protected override void GetPartsOfIfStatement(
IfStatementSyntax statement,
out ExpressionSyntax condition,
out IEnumerable<StatementSyntax> whenTrueStatements,
out IEnumerable<StatementSyntax>? whenFalseStatements)
Copy link
Member Author

Choose a reason for hiding this comment

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

same issue. C# has if/else (Where else is optional). VB has if/elif/elif/else. These are really different forms, and trying to shoehorn into ISyntaxFacts was not the right way to do things.

{
condition = statement.Condition;
whenTrueStatements = ExtractEmbeddedStatements(statement.Statement);
whenFalseStatements = statement.Else != null ? ExtractEmbeddedStatements(statement.Else.Statement) : null;
}

private static IEnumerable<StatementSyntax> ExtractEmbeddedStatements(StatementSyntax embeddedStatement)
=> embeddedStatement is BlockSyntax block ? block.Statements : SpecializedCollections.SingletonEnumerable(embeddedStatement);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Analyzers.UseCollectionExpression;
using Microsoft.CodeAnalysis.CSharp.Extensions;
Expand All @@ -11,11 +12,12 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.LanguageService;
using Microsoft.CodeAnalysis.UseCollectionInitializer;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionInitializer;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal class CSharpUseCollectionInitializerDiagnosticAnalyzer :
internal sealed class CSharpUseCollectionInitializerDiagnosticAnalyzer :
AbstractUseCollectionInitializerDiagnosticAnalyzer<
SyntaxKind,
ExpressionSyntax,
Expand All @@ -25,8 +27,13 @@ internal class CSharpUseCollectionInitializerDiagnosticAnalyzer :
InvocationExpressionSyntax,
ExpressionStatementSyntax,
ForEachStatementSyntax,
VariableDeclaratorSyntax>
IfStatementSyntax,
VariableDeclaratorSyntax,
CSharpUseCollectionInitializerAnalyzer>
{
protected override CSharpUseCollectionInitializerAnalyzer GetAnalyzer()
=> CSharpUseCollectionInitializerAnalyzer.Allocate();

protected override bool AreCollectionInitializersSupported(Compilation compilation)
=> compilation.LanguageVersion() >= LanguageVersion.CSharp3;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Diagnostics.CodeAnalysis;
Expand Down Expand Up @@ -33,14 +35,19 @@ internal class CSharpUseCollectionInitializerCodeFixProvider :
InvocationExpressionSyntax,
ExpressionStatementSyntax,
ForEachStatementSyntax,
VariableDeclaratorSyntax>
IfStatementSyntax,
VariableDeclaratorSyntax,
CSharpUseCollectionInitializerAnalyzer>
{
[ImportingConstructor]
[SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
public CSharpUseCollectionInitializerCodeFixProvider()
{
}

protected override CSharpUseCollectionInitializerAnalyzer GetAnalyzer()
=> CSharpUseCollectionInitializerAnalyzer.Allocate();

protected override StatementSyntax GetNewStatement(
SourceText sourceText,
StatementSyntax statement,
Expand All @@ -62,7 +69,7 @@ private static ExpressionSyntax GetNewObjectCreation(
ImmutableArray<Match<StatementSyntax>> matches)
{
return useCollectionExpression
? CreateCollectionExpression(objectCreation, matches, MakeMultiLine(sourceText, objectCreation, matches, wrappingLength))
? CreateCollectionExpression(sourceText, objectCreation, wrappingLength, matches)
: CreateObjectInitializerExpression(objectCreation, matches);
}

Expand All @@ -76,15 +83,16 @@ private static BaseObjectCreationExpressionSyntax CreateObjectInitializerExpress
}

private static CollectionExpressionSyntax CreateCollectionExpression(
SourceText sourceText,
BaseObjectCreationExpressionSyntax objectCreation,
ImmutableArray<Match<StatementSyntax>> matches,
bool makeMultiLine)
int wrappingLength,
ImmutableArray<Match<StatementSyntax>> matches)
{
var elements = CreateElements<CollectionElementSyntax>(
objectCreation, matches,
static (match, expression) => match?.UseSpread is true ? SpreadElement(expression) : ExpressionElement(expression));

if (makeMultiLine)
if (MakeMultiLine(sourceText, objectCreation, matches, wrappingLength))
elements = AddLineBreaks(elements, includeFinalLineBreak: false);

return CollectionExpression(elements).WithTriviaFrom(objectCreation);
Expand Down Expand Up @@ -140,42 +148,48 @@ private static bool MakeMultiLine(
if (!sourceText.AreOnSameLine(objectCreation.GetFirstToken(), objectCreation.GetLastToken()))
return true;

foreach (var match in matches)
{
var expression = GetExpression(match);

// If we have anything like: `new Dictionary<X,Y> { { A, B }, { C, D } }` then always make multiline.
// Similarly, if we have `new Dictionary<X,Y> { [A] = B }`.
if (expression is InitializerExpressionSyntax or AssignmentExpressionSyntax)
return true;

// if any of the expressions we're adding are multiline, then make things multiline.
if (!sourceText.AreOnSameLine(expression.GetFirstToken(), expression.GetLastToken()))
return true;
}

var totalLength = "{}".Length;
foreach (var match in matches)
Copy link
Member Author

Choose a reason for hiding this comment

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

note: while this is getting cleaned up here, it is also getting refactored heavily in the next PR where i redo how we do formatting here entirely. so don't have to examien this too closely :)

{
var expression = GetExpression(match);
totalLength += expression.Span.Length;
totalLength += ", ".Length;
foreach (var component in GetElementComponents(match.Statement))
{
// if any of the expressions we're adding are multiline, then make things multiline.
if (!sourceText.AreOnSameLine(component.GetFirstToken(), component.GetLastToken()))
return true;

if (totalLength > wrappingLength)
return true;
totalLength += component.Span.Length;
totalLength += ", ".Length;

if (totalLength > wrappingLength)
return true;
}
}

return false;

static ExpressionSyntax GetExpression(Match<StatementSyntax> match)
=> match.Statement switch
static IEnumerable<SyntaxNode> GetElementComponents(StatementSyntax statement)
{
if (statement is ExpressionStatementSyntax expressionStatement)
{
ExpressionStatementSyntax expressionStatement => expressionStatement.Expression,
ForEachStatementSyntax foreachStatement => foreachStatement.Expression,
_ => throw ExceptionUtilities.Unreachable(),
};
yield return expressionStatement.Expression;
}
else if (statement is ForEachStatementSyntax foreachStatement)
{
yield return foreachStatement.Expression;
}
else if (statement is IfStatementSyntax ifStatement)
{
yield return ifStatement.Condition;
yield return UnwrapEmbeddedStatement(ifStatement.Statement);
if (ifStatement.Else != null)
yield return UnwrapEmbeddedStatement(ifStatement.Else.Statement);
}
}
}

private static StatementSyntax UnwrapEmbeddedStatement(StatementSyntax statement)
=> statement is BlockSyntax { Statements: [var innerStatement] } ? innerStatement : statement;

public static SeparatedSyntaxList<TNode> AddLineBreaks<TNode>(
SeparatedSyntaxList<TNode> nodes, bool includeFinalLineBreak)
where TNode : SyntaxNode
Expand Down Expand Up @@ -207,7 +221,8 @@ private static SeparatedSyntaxList<TElement> CreateElements<TElement>(
{
using var _ = ArrayBuilder<SyntaxNodeOrToken>.GetInstance(out var nodesAndTokens);

UseInitializerHelpers.AddExistingItems(objectCreation, nodesAndTokens, createElement);
UseInitializerHelpers.AddExistingItems(
objectCreation, nodesAndTokens, addTrailingComma: matches.Length > 0, createElement);

for (var i = 0; i < matches.Length; i++)
{
Expand Down Expand Up @@ -241,6 +256,30 @@ private static SeparatedSyntaxList<TElement> CreateElements<TElement>(
if (i < matches.Length - 1)
nodesAndTokens.Add(Token(SyntaxKind.CommaToken));
}
else if (statement is IfStatementSyntax ifStatement)
{
var trueStatement = (ExpressionStatementSyntax)UnwrapEmbeddedStatement(ifStatement.Statement);

if (ifStatement.Else is null)
{
// Create: x ? [y] : []
var expression = ConditionalExpression(
ifStatement.Condition.Parenthesize(),
CollectionExpression(SingletonSeparatedList<CollectionElementSyntax>(ExpressionElement(ConvertExpression(trueStatement.Expression)))),
CollectionExpression());
nodesAndTokens.Add(createElement(match, expression));
}
else
{
// Create: x ? y : z
var falseStatement = (ExpressionStatementSyntax)UnwrapEmbeddedStatement(ifStatement.Else.Statement);
var expression = ConditionalExpression(ifStatement.Condition.Parenthesize(), ConvertExpression(trueStatement.Expression), ConvertExpression(falseStatement.Expression));
nodesAndTokens.Add(createElement(match, expression));
}

if (i < matches.Length - 1)
nodesAndTokens.Add(Token(SyntaxKind.CommaToken));
}
else
{
throw ExceptionUtilities.Unreachable();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ private static SeparatedSyntaxList<ExpressionSyntax> CreateExpressions(
using var _ = ArrayBuilder<SyntaxNodeOrToken>.GetInstance(out var nodesAndTokens);

UseInitializerHelpers.AddExistingItems<ObjectInitializerMatch, ExpressionSyntax>(
objectCreation, nodesAndTokens, static (_, e) => e);
objectCreation, nodesAndTokens, addTrailingComma: true, static (_, e) => e);

for (var i = 0; i < matches.Length; i++)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public static BaseObjectCreationExpressionSyntax GetNewObjectCreation(
public static void AddExistingItems<TMatch, TElementSyntax>(
BaseObjectCreationExpressionSyntax objectCreation,
ArrayBuilder<SyntaxNodeOrToken> nodesAndTokens,
bool addTrailingComma,
Func<TMatch?, ExpressionSyntax, TElementSyntax> createElement)
where TMatch : struct
where TElementSyntax : SyntaxNode
Expand All @@ -51,7 +52,7 @@ public static void AddExistingItems<TMatch, TElementSyntax>(

// If we have an odd number of elements already, add a comma at the end so that we can add the rest of the
// items afterwards without a syntax issue.
if (nodesAndTokens.Count % 2 == 1)
if (addTrailingComma && nodesAndTokens.Count % 2 == 1)
{
var last = nodesAndTokens.Last();
nodesAndTokens.RemoveLast();
Expand Down
Loading