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

Update "use collection initializer" to use a collection-expression if the user prefers that form. #69219

Merged
merged 44 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
873f289
Extract helper code
CyrusNajmabadi Jul 24, 2023
1903cf4
Add tests
CyrusNajmabadi Jul 24, 2023
6882edd
crlf
CyrusNajmabadi Jul 24, 2023
c1bb50f
Compiling
CyrusNajmabadi Jul 24, 2023
e278ebf
in progress
CyrusNajmabadi Jul 24, 2023
60bf4c1
In progrss
CyrusNajmabadi Jul 24, 2023
e96f926
Building
CyrusNajmabadi Jul 24, 2023
424722a
Rename
CyrusNajmabadi Jul 24, 2023
95f1180
Rename
CyrusNajmabadi Jul 24, 2023
c9ad1ad
Rename
CyrusNajmabadi Jul 24, 2023
8da263d
Fixes
CyrusNajmabadi Jul 24, 2023
04899dc
Test fixes
CyrusNajmabadi Jul 25, 2023
0681c8d
In progrss
CyrusNajmabadi Jul 25, 2023
594a61c
Merge branch 'useCollectionExpression' into useCollectionExpression2
CyrusNajmabadi Jul 25, 2023
503a97f
Merge branch 'useCollectionExpression' into useCollectionExpression2
CyrusNajmabadi Jul 25, 2023
f738836
Fixes
CyrusNajmabadi Jul 25, 2023
ca91efd
Add suppression
CyrusNajmabadi Jul 25, 2023
57cdd67
updates
CyrusNajmabadi Jul 25, 2023
419565e
Revert
CyrusNajmabadi Jul 25, 2023
1e97397
Fixes
CyrusNajmabadi Jul 25, 2023
9120941
tweak
CyrusNajmabadi Jul 25, 2023
ea38ef0
In progress
CyrusNajmabadi Jul 25, 2023
361487a
In progress
CyrusNajmabadi Jul 25, 2023
3b43bb2
In progress
CyrusNajmabadi Jul 25, 2023
f4fd927
in progress
CyrusNajmabadi Jul 25, 2023
a13673f
Mostly working
CyrusNajmabadi Jul 26, 2023
7072918
Improvements
CyrusNajmabadi Jul 26, 2023
59db01a
Improvements
CyrusNajmabadi Jul 26, 2023
a452a0c
Merge remote-tracking branch 'upstream/main' into useCollectionExpres…
CyrusNajmabadi Jul 26, 2023
9932551
Add test
CyrusNajmabadi Jul 26, 2023
bb6702c
Add test
CyrusNajmabadi Jul 26, 2023
b93d46c
Share codE
CyrusNajmabadi Jul 26, 2023
8dcaadf
Update src/Analyzers/CSharp/CodeFixes/UseCollectionInitializer/CSharp…
CyrusNajmabadi Jul 26, 2023
b0c9195
Self documenting
CyrusNajmabadi Jul 26, 2023
3a9c5bb
Simplify
CyrusNajmabadi Jul 26, 2023
9f8bcf2
Merge branch 'useCollectionExpression2' of https://github.com/CyrusNa…
CyrusNajmabadi Jul 26, 2023
560a123
REstore
CyrusNajmabadi Jul 26, 2023
3d629fb
REstore
CyrusNajmabadi Jul 26, 2023
acd85e5
Add comments
CyrusNajmabadi Jul 26, 2023
8f4bfd7
simplify
CyrusNajmabadi Jul 26, 2023
640dd0f
simplify
CyrusNajmabadi Jul 26, 2023
cbf06f5
simplify
CyrusNajmabadi Jul 26, 2023
1d75e4d
Apply suggestions from code review
CyrusNajmabadi Jul 26, 2023
d65ce70
Merge remote-tracking branch 'upstream/main' into useCollectionExpres…
CyrusNajmabadi Jul 28, 2023
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: 1 addition & 0 deletions src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems
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
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 LiteralExpressionSyntax s_nullLiteralExpression = LiteralExpression(SyntaxKind.NullLiteralExpression);

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)
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jul 26, 2023

Choose a reason for hiding this comment

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

this code moved to helper type since it will be used by a lot more analyzers. basically, it's the code that says "could i use a collection literal here".

{
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,72 +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 `null` literal.
// This is a similarly 'untyped' literal (like a collection-expression is), so it tells us if the new code will
// have any issues moving to something untyped. This will also tell us if we have any ambiguities (because
// there are multiple destination types that could accept the collection expression).
var speculationAnalyzer = new SpeculationAnalyzer(
topmostExpression,
s_nullLiteralExpression,
semanticModel,
cancellationToken,
skipVerificationForReplacedNode: true,
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 @@ -255,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
Loading