Skip to content

Commit

Permalink
Merge pull request #69219 from CyrusNajmabadi/useCollectionExpression2
Browse files Browse the repository at this point in the history 
Update "use collection initializer" to use a collection-expression if the user prefers that form.
  • Loading branch information
@CyrusNajmabadi
CyrusNajmabadi authored Jul 28, 2023
2 parents c6c59b7 + d65ce70 commit 7a592d3
Show file tree
Hide file tree
Showing 41 changed files with 2,706 additions and 391 deletions.
1 change: 1 addition & 0 deletions src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
<Compile Include="$(MSBuildThisFileDirectory)UseCoalesceExpression\CSharpUseCoalesceExpressionForNullableTernaryConditionalCheckDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCoalesceExpression\UseCoalesceExpressionHelpers.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCollectionExpression\UseCollectionExpressionHelpers.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCompoundAssignment\CSharpUseCompoundAssignmentDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCompoundAssignment\CSharpUseCompoundCoalesceAssignmentDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseCompoundAssignment\Utilities.cs" />
Expand Down
168 changes: 15 additions & 153 deletions ...lyzers/UseCollectionExpression/CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,22 @@

using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Analyzers.UseCollectionExpression;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.CSharp.Utilities;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;

using static SyntaxFactory;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal sealed partial class CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer
: AbstractBuiltInCodeStyleDiagnosticAnalyzer
{
private static readonly CollectionExpressionSyntax s_emptyCollectionExpression = CollectionExpression();

public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
=> DiagnosticAnalyzerCategory.SemanticSpanAnalysis;

Expand Down Expand Up @@ -56,7 +49,7 @@ protected override void InitializeWorker(AnalysisContext context)

private void OnCompilationStart(CompilationStartAnalysisContext context)
{
if (!context.Compilation.LanguageVersion().IsCSharp12OrAbove())
if (!context.Compilation.LanguageVersion().SupportsCollectionExpressions())
return;

// We wrap the SyntaxNodeAction within a CodeBlockStartAction, which allows us to
Expand All @@ -72,101 +65,6 @@ private void OnCompilationStart(CompilationStartAnalysisContext context)
});
}

private static bool IsInTargetTypedLocation(SemanticModel semanticModel, ExpressionSyntax expression, CancellationToken cancellationToken)
{
var topExpression = expression.WalkUpParentheses();
var parent = topExpression.Parent;
return parent switch
{
EqualsValueClauseSyntax equalsValue => IsInTargetTypedEqualsValueClause(equalsValue),
CastExpressionSyntax castExpression => IsInTargetTypedCastExpression(castExpression),
// a ? [1, 2, 3] : ... is target typed if either the other side is *not* a collection,
// or the entire ternary is target typed itself.
ConditionalExpressionSyntax conditionalExpression => IsInTargetTypedConditionalExpression(conditionalExpression, topExpression),
// Similar rules for switches.
SwitchExpressionArmSyntax switchExpressionArm => IsInTargetTypedSwitchExpressionArm(switchExpressionArm),
InitializerExpressionSyntax initializerExpression => IsInTargetTypedInitializerExpression(initializerExpression, topExpression),
AssignmentExpressionSyntax assignmentExpression => IsInTargetTypedAssignmentExpression(assignmentExpression, topExpression),
BinaryExpressionSyntax binaryExpression => IsInTargetTypedBinaryExpression(binaryExpression, topExpression),
ArgumentSyntax or AttributeArgumentSyntax => true,
ReturnStatementSyntax => true,
_ => false,
};

bool HasType(ExpressionSyntax expression)
=> semanticModel.GetTypeInfo(expression, cancellationToken).Type != null;

static bool IsInTargetTypedEqualsValueClause(EqualsValueClauseSyntax equalsValue)
// If we're after an `x = ...` and it's not `var x`, this is target typed.
=> equalsValue.Parent is not VariableDeclaratorSyntax { Parent: VariableDeclarationSyntax { Type.IsVar: true } };

static bool IsInTargetTypedCastExpression(CastExpressionSyntax castExpression)
// (X[])[1, 2, 3] is target typed. `(X)[1, 2, 3]` is currently not (because it looks like indexing into an expr).
=> castExpression.Type is not IdentifierNameSyntax;

bool IsInTargetTypedConditionalExpression(ConditionalExpressionSyntax conditionalExpression, ExpressionSyntax expression)
{
if (conditionalExpression.WhenTrue == expression)
return HasType(conditionalExpression.WhenFalse) || IsInTargetTypedLocation(semanticModel, conditionalExpression, cancellationToken);
else if (conditionalExpression.WhenFalse == expression)
return HasType(conditionalExpression.WhenTrue) || IsInTargetTypedLocation(semanticModel, conditionalExpression, cancellationToken);
else
return false;
}

bool IsInTargetTypedSwitchExpressionArm(SwitchExpressionArmSyntax switchExpressionArm)
{
var switchExpression = (SwitchExpressionSyntax)switchExpressionArm.GetRequiredParent();

// check if any other arm has a type that this would be target typed against.
foreach (var arm in switchExpression.Arms)
{
if (arm != switchExpressionArm && HasType(arm.Expression))
return true;
}

// All arms do not have a type, this is target typed if the switch itself is target typed.
return IsInTargetTypedLocation(semanticModel, switchExpression, cancellationToken);
}

bool IsInTargetTypedInitializerExpression(InitializerExpressionSyntax initializerExpression, ExpressionSyntax expression)
{
// new X[] { [1, 2, 3] }. Elements are target typed by array type.
if (initializerExpression.Parent is ArrayCreationExpressionSyntax)
return true;

// new [] { [1, 2, 3], ... }. Elements are target typed if there's another element with real type.
if (initializerExpression.Parent is ImplicitArrayCreationExpressionSyntax)
{
foreach (var sibling in initializerExpression.Expressions)
{
if (sibling != expression && HasType(sibling))
return true;
}
}

// TODO: Handle these.
if (initializerExpression.Parent is StackAllocArrayCreationExpressionSyntax or ImplicitStackAllocArrayCreationExpressionSyntax)
return false;

// T[] x = [1, 2, 3];
if (initializerExpression.Parent is EqualsValueClauseSyntax)
return true;

return false;
}

bool IsInTargetTypedAssignmentExpression(AssignmentExpressionSyntax assignmentExpression, ExpressionSyntax expression)
{
return expression == assignmentExpression.Right && HasType(assignmentExpression.Left);
}

bool IsInTargetTypedBinaryExpression(BinaryExpressionSyntax binaryExpression, ExpressionSyntax expression)
{
return binaryExpression.Kind() == SyntaxKind.CoalesceExpression && binaryExpression.Right == expression && HasType(binaryExpression.Left);
}
}

private static void AnalyzeArrayInitializer(SyntaxNodeAnalysisContext context)
{
var semanticModel = context.SemanticModel;
Expand All @@ -179,74 +77,38 @@ private static void AnalyzeArrayInitializer(SyntaxNodeAnalysisContext context)
if (!option.Value)
return;

if (initializer.GetDiagnostics().Any(d => d.Severity == DiagnosticSeverity.Error))
return;

var parent = initializer.GetRequiredParent();
var topmostExpression = parent is ExpressionSyntax parentExpression
? parentExpression.WalkUpParentheses()
: initializer.WalkUpParentheses();
var isConcreteOrImplicitArrayCreation = initializer.Parent is ArrayCreationExpressionSyntax or ImplicitArrayCreationExpressionSyntax;

if (!IsInTargetTypedLocation(semanticModel, topmostExpression, cancellationToken))
// a naked `{ ... }` can only be converted to a collection expression when in the exact form `x = { ... }`
if (!isConcreteOrImplicitArrayCreation && initializer.Parent is not EqualsValueClauseSyntax)
return;

var isConcreteOrImplicitArrayCreation = parent is ArrayCreationExpressionSyntax or ImplicitArrayCreationExpressionSyntax;
if (isConcreteOrImplicitArrayCreation)
{
// X[] = new Y[] { 1, 2, 3 }
//
// First, we don't change things if X and Y are different. That could lead to something observable at
// runtime in the case of something like: object[] x = new string[] ...

var typeInfo = semanticModel.GetTypeInfo(parent, cancellationToken);
if (typeInfo.Type is null or IErrorTypeSymbol ||
typeInfo.ConvertedType is null or IErrorTypeSymbol)
{
return;
}
var arrayCreationExpression = isConcreteOrImplicitArrayCreation
? (ExpressionSyntax)initializer.GetRequiredParent()
: initializer;

if (!typeInfo.Type.Equals(typeInfo.ConvertedType))
return;
}
else if (parent is not EqualsValueClauseSyntax)
if (!UseCollectionExpressionHelpers.CanReplaceWithCollectionExpression(
semanticModel, arrayCreationExpression, cancellationToken))
{
return;
}

// Looks good as something to replace. Now check the semantics of making the replacement to see if there would
// any issues. To keep things simple, all we do is replace the existing expression with the `[]` literal. This
// will tell us if we have problems assigning a collection expression to teh target type.
//
// Note: this does mean certain unambiguous cases with overloads (like `Goo(int[] values)` vs `Goo(string[]
// values)`) will not get simplification. We can revisit this in the future to see if that warrants a more
// expensive check that involves checking the consitutuent elements of the literal.
var speculationAnalyzer = new SpeculationAnalyzer(
topmostExpression,
s_emptyCollectionExpression,
semanticModel,
cancellationToken,
skipVerificationForReplacedNode: false,
failOnOverloadResolutionFailuresInOriginalCode: true);

if (speculationAnalyzer.ReplacementChangesSemantics())
return;

if (isConcreteOrImplicitArrayCreation)
{
var locations = ImmutableArray.Create(initializer.GetLocation());
context.ReportDiagnostic(DiagnosticHelper.Create(
s_descriptor,
parent.GetFirstToken().GetLocation(),
arrayCreationExpression.GetFirstToken().GetLocation(),
option.Notification.Severity,
additionalLocations: locations,
properties: null));

var additionalUnnecessaryLocations = ImmutableArray.Create(
syntaxTree.GetLocation(TextSpan.FromBounds(
parent.SpanStart,
parent is ArrayCreationExpressionSyntax arrayCreation
arrayCreationExpression.SpanStart,
arrayCreationExpression is ArrayCreationExpressionSyntax arrayCreation
? arrayCreation.Type.Span.End
: ((ImplicitArrayCreationExpressionSyntax)parent).CloseBracketToken.Span.End)));
: ((ImplicitArrayCreationExpressionSyntax)arrayCreationExpression).CloseBracketToken.Span.End)));

context.ReportDiagnostic(DiagnosticHelper.CreateWithLocationTags(
s_unnecessaryCodeDescriptor,
Expand All @@ -257,7 +119,7 @@ parent is ArrayCreationExpressionSyntax arrayCreation
}
else
{
Debug.Assert(parent is EqualsValueClauseSyntax);
Debug.Assert(initializer.Parent is EqualsValueClauseSyntax);
// int[] = { 1, 2, 3 };
//
// In this case, we always have a target type, so it should always be valid to convert this to a collection expression.
Expand Down
Loading

0 comments on commit 7a592d3

Please sign in to comment.