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.

Sign up for GitHub

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

Offer use-collection-expr when targeting interfaces #71373

Merged
merged 19 commits into from
Dec 21, 2023
Merged

Offer use-collection-expr when targeting interfaces #71373

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2b86991
Offer use-collection-expr when targeting interfaces
CyrusNajmabadi Dec 20, 2023
b001b75
Let the user know that semantics may change
CyrusNajmabadi Dec 20, 2023
975799d
Let the user know that semantics may change
CyrusNajmabadi Dec 20, 2023
0acf45e
Fix
CyrusNajmabadi Dec 20, 2023
c03b3db
Move
CyrusNajmabadi Dec 20, 2023
fc72fc8
Update src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseColl…
CyrusNajmabadi Dec 20, 2023
fa10f04
REvert
CyrusNajmabadi Dec 20, 2023
db369a4
Fix tests
CyrusNajmabadi Dec 20, 2023
f4188ad
Merge branch 'collectionExprInterfaces' of https://github.com/CyrusNa…
CyrusNajmabadi Dec 20, 2023
4846c44
Fix tests
CyrusNajmabadi Dec 20, 2023
51ac8c9
Update src/Analyzers/Core/CodeFixes/UseCollectionInitializer/Abstract…
CyrusNajmabadi Dec 20, 2023
9ee6db2
Update src/Analyzers/Core/CodeFixes/UseObjectInitializer/AbstractUseO…
CyrusNajmabadi Dec 20, 2023
e5e0e81
lint
CyrusNajmabadi Dec 21, 2023
0c22f77
Use new option system
CyrusNajmabadi Dec 21, 2023
dd5491a
remove
CyrusNajmabadi Dec 21, 2023
e7fd9fa
Merge branch 'main' into collectionExprInterfaces
CyrusNajmabadi Dec 21, 2023
44e6290
Fix
CyrusNajmabadi Dec 21, 2023
d156c37
update test
CyrusNajmabadi Dec 21, 2023
28ac4d1
Fix tests
CyrusNajmabadi Dec 21, 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
4 changes: 4 additions & 0 deletions ...lyzers/UseCollectionExpression/AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.UseCollectionInitializer;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;

Expand All @@ -18,6 +19,9 @@ namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;
internal abstract class AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer
: AbstractBuiltInCodeStyleDiagnosticAnalyzer
{
public static readonly ImmutableDictionary<string, string?> ChangesSemantics =
ImmutableDictionary<string, string?>.Empty.Add(UseCollectionInitializerHelpers.ChangesSemanticsName, "");

protected new readonly DiagnosticDescriptor Descriptor;
protected readonly DiagnosticDescriptor UnnecessaryCodeDescriptor;

Expand Down
43 changes: 28 additions & 15 deletions ...lyzers/UseCollectionExpression/CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,33 +46,38 @@ private void AnalyzeArrayCreationExpression(SyntaxNodeAnalysisContext context, I
return;

// Analyze the statements that follow to see if they can initialize this array.
var matches = TryGetMatches(semanticModel, arrayCreationExpression, expressionType, cancellationToken);
var allowInterfaceConversion = context.GetAnalyzerOptions().PreferCollectionExpressionForInterfaces.Value;
var matches = TryGetMatches(semanticModel, arrayCreationExpression, expressionType, allowInterfaceConversion, cancellationToken, out var changesSemantics);
Copy link
Member Author

Choose a reason for hiding this comment

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

most of the change is getting hte info about if sematnics changed, passing it into the diagnostic, and presenting it in the fix.

if (matches.IsDefault)
return;

ReportArrayCreationDiagnostics(context, syntaxTree, option, arrayCreationExpression);
ReportArrayCreationDiagnostics(context, syntaxTree, option, arrayCreationExpression, changesSemantics);
}

public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetMatches(
SemanticModel semanticModel,
ArrayCreationExpressionSyntax expression,
INamedTypeSymbol? expressionType,
CancellationToken cancellationToken)
bool allowInterfaceConversion,
CancellationToken cancellationToken,
out bool changesSemantics)
{
// we have `new T[...] ...;` defer to analyzer to find the items that follow that may need to
// be added to the collection expression.
var matches = UseCollectionExpressionHelpers.TryGetMatches(
semanticModel,
expression,
expressionType,
allowInterfaceConversion,
static e => e.Type,
static e => e.Initializer,
cancellationToken);
cancellationToken,
out changesSemantics);
if (matches.IsDefault)
return default;

if (!UseCollectionExpressionHelpers.CanReplaceWithCollectionExpression(
semanticModel, expression, expressionType, skipVerificationForReplacedNode: true, cancellationToken))
semanticModel, expression, expressionType, allowInterfaceConversion, skipVerificationForReplacedNode: true, cancellationToken, out changesSemantics))
{
return default;
}
Expand All @@ -84,12 +89,14 @@ public static ImmutableArray<CollectionExpressionMatch<StatementSyntax>> TryGetM
SemanticModel semanticModel,
ImplicitArrayCreationExpressionSyntax expression,
INamedTypeSymbol? expressionType,
CancellationToken cancellationToken)
bool allowInterfaceConversion,
CancellationToken cancellationToken,
out bool changesSemantics)
{
// if we have `new[] { ... }` we have no subsequent matches to add to the collection. All values come
// from within the initializer.
if (!UseCollectionExpressionHelpers.CanReplaceWithCollectionExpression(
semanticModel, expression, expressionType, skipVerificationForReplacedNode: true, cancellationToken))
semanticModel, expression, expressionType, allowInterfaceConversion, skipVerificationForReplacedNode: true, cancellationToken, out changesSemantics))
{
return default;
}
Expand Down Expand Up @@ -124,9 +131,11 @@ private void AnalyzeArrayInitializerExpression(SyntaxNodeAnalysisContext context
var replacementCollectionExpression = CollectionExpression(
SeparatedList<CollectionElementSyntax>(initializer.Expressions.Select(ExpressionElement)));

var allowInterfaceConversion = context.GetAnalyzerOptions().PreferCollectionExpressionForInterfaces.Value;
if (!UseCollectionExpressionHelpers.CanReplaceWithCollectionExpression(
semanticModel, arrayCreationExpression, replacementCollectionExpression,
expressionType, skipVerificationForReplacedNode: true, cancellationToken))
expressionType, allowInterfaceConversion, skipVerificationForReplacedNode: true, cancellationToken,
out var changesSemantics))
{
return;
}
Expand All @@ -135,19 +144,20 @@ private void AnalyzeArrayInitializerExpression(SyntaxNodeAnalysisContext context
{
var matches = initializer.Parent switch
{
ArrayCreationExpressionSyntax arrayCreation => TryGetMatches(semanticModel, arrayCreation, expressionType, cancellationToken),
ImplicitArrayCreationExpressionSyntax arrayCreation => TryGetMatches(semanticModel, arrayCreation, expressionType, cancellationToken),
ArrayCreationExpressionSyntax arrayCreation => TryGetMatches(semanticModel, arrayCreation, expressionType, allowInterfaceConversion, cancellationToken, out _),
ImplicitArrayCreationExpressionSyntax arrayCreation => TryGetMatches(semanticModel, arrayCreation, expressionType, allowInterfaceConversion, cancellationToken, out _),
_ => throw ExceptionUtilities.Unreachable(),
};

if (matches.IsDefault)
return;

ReportArrayCreationDiagnostics(context, syntaxTree, option, arrayCreationExpression);
ReportArrayCreationDiagnostics(context, syntaxTree, option, arrayCreationExpression, changesSemantics);
}
else
{
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 All @@ -156,19 +166,21 @@ private void AnalyzeArrayInitializerExpression(SyntaxNodeAnalysisContext context
initializer.OpenBraceToken.GetLocation(),
option.Notification,
additionalLocations: ImmutableArray.Create(initializer.GetLocation()),
properties: null));
properties: changesSemantics ? ChangesSemantics : null));
}
}

private void ReportArrayCreationDiagnostics(SyntaxNodeAnalysisContext context, SyntaxTree syntaxTree, CodeStyleOption2<bool> option, ExpressionSyntax expression)
private void ReportArrayCreationDiagnostics(
SyntaxNodeAnalysisContext context, SyntaxTree syntaxTree, CodeStyleOption2<bool> option, ExpressionSyntax expression, bool changesSemantics)
{
var properties = changesSemantics ? ChangesSemantics : null;
var locations = ImmutableArray.Create(expression.GetLocation());
context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
expression.GetFirstToken().GetLocation(),
option.Notification,
additionalLocations: locations,
properties: null));
properties: properties));

var additionalUnnecessaryLocations = ImmutableArray.Create(
syntaxTree.GetLocation(TextSpan.FromBounds(
Expand All @@ -182,6 +194,7 @@ expression is ArrayCreationExpressionSyntax arrayCreationExpression
additionalUnnecessaryLocations[0],
NotificationOption2.ForSeverity(UnnecessaryCodeDescriptor.DefaultSeverity),
additionalLocations: locations,
additionalUnnecessaryLocations: additionalUnnecessaryLocations));
additionalUnnecessaryLocations: additionalUnnecessaryLocations,
properties: properties));
}
}
70 changes: 36 additions & 34 deletions ...zers/UseCollectionExpression/CSharpUseCollectionExpressionForBuilderDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,14 @@
namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal sealed partial class CSharpUseCollectionExpressionForBuilderDiagnosticAnalyzer
: AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer
internal sealed partial class CSharpUseCollectionExpressionForBuilderDiagnosticAnalyzer()
: AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer(
IDEDiagnosticIds.UseCollectionExpressionForBuilderDiagnosticId,
EnforceOnBuildValues.UseCollectionExpressionForBuilder)
{
private const string CreateBuilderName = nameof(ImmutableArray.CreateBuilder);
private const string GetInstanceName = nameof(ArrayBuilder<int>.GetInstance);

public CSharpUseCollectionExpressionForBuilderDiagnosticAnalyzer()
: base(IDEDiagnosticIds.UseCollectionExpressionForBuilderDiagnosticId,
EnforceOnBuildValues.UseCollectionExpressionForBuilder)
{
}

protected override void InitializeWorker(CodeBlockStartAnalysisContext<SyntaxKind> context, INamedTypeSymbol? expressionType)
=> context.RegisterSyntaxNodeAction(context => AnalyzeInvocationExpression(context, expressionType), SyntaxKind.InvocationExpression);

Expand All @@ -47,56 +43,61 @@ private void AnalyzeInvocationExpression(
if (!option.Value || ShouldSkipAnalysis(context, option.Notification))
return;

if (AnalyzeInvocation(semanticModel, invocationExpression, expressionType, cancellationToken) is not { } analysisResult)
var allowInterfaceConversion = context.GetAnalyzerOptions().PreferCollectionExpressionForInterfaces.Value;
if (AnalyzeInvocation(semanticModel, invocationExpression, expressionType, allowInterfaceConversion, cancellationToken) is not { } analysisResult)
return;

var locations = ImmutableArray.Create(invocationExpression.GetLocation());
var properties = analysisResult.ChangesSemantics ? ChangesSemantics : null;
context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
analysisResult.DiagnosticLocation,
option.Notification,
additionalLocations: locations,
properties: null));
properties: properties));

FadeOutCode(context, analysisResult, locations);
}

private void FadeOutCode(SyntaxNodeAnalysisContext context, AnalysisResult analysisResult, ImmutableArray<Location> locations)
{
var additionalUnnecessaryLocations = ImmutableArray.Create(
analysisResult.LocalDeclarationStatement.GetLocation());
return;

context.ReportDiagnostic(DiagnosticHelper.CreateWithLocationTags(
UnnecessaryCodeDescriptor,
additionalUnnecessaryLocations[0],
NotificationOption2.ForSeverity(UnnecessaryCodeDescriptor.DefaultSeverity),
additionalLocations: locations,
additionalUnnecessaryLocations: additionalUnnecessaryLocations,
properties: null));

foreach (var statementMatch in analysisResult.Matches)
void FadeOutCode(SyntaxNodeAnalysisContext context, AnalysisResult analysisResult, ImmutableArray<Location> locations)
{
additionalUnnecessaryLocations = UseCollectionInitializerHelpers.GetLocationsToFade(
CSharpSyntaxFacts.Instance, statementMatch);
if (additionalUnnecessaryLocations.IsDefaultOrEmpty)
continue;
var additionalUnnecessaryLocations = ImmutableArray.Create(
analysisResult.LocalDeclarationStatement.GetLocation());

// Report the diagnostic at the first unnecessary location. This is the location where the code fix
// will be offered.
context.ReportDiagnostic(DiagnosticHelper.CreateWithLocationTags(
UnnecessaryCodeDescriptor,
additionalUnnecessaryLocations[0],
NotificationOption2.ForSeverity(UnnecessaryCodeDescriptor.DefaultSeverity),
additionalLocations: locations,
additionalUnnecessaryLocations: additionalUnnecessaryLocations,
properties: null));
properties: properties));

foreach (var statementMatch in analysisResult.Matches)
{
additionalUnnecessaryLocations = UseCollectionInitializerHelpers.GetLocationsToFade(
CSharpSyntaxFacts.Instance, statementMatch);
if (additionalUnnecessaryLocations.IsDefaultOrEmpty)
continue;

// Report the diagnostic at the first unnecessary location. This is the location where the code fix
// will be offered.
context.ReportDiagnostic(DiagnosticHelper.CreateWithLocationTags(
UnnecessaryCodeDescriptor,
additionalUnnecessaryLocations[0],
NotificationOption2.ForSeverity(UnnecessaryCodeDescriptor.DefaultSeverity),
additionalLocations: locations,
additionalUnnecessaryLocations: additionalUnnecessaryLocations,
properties: properties));
}
}
}

public static AnalysisResult? AnalyzeInvocation(
SemanticModel semanticModel,
InvocationExpressionSyntax invocationExpression,
INamedTypeSymbol? expressionType,
bool allowInterfaceConversion,
CancellationToken cancellationToken)
{
// Looking for `XXX.CreateBuilder(...)`
Expand Down Expand Up @@ -191,13 +192,13 @@ private void FadeOutCode(SyntaxNodeAnalysisContext context, AnalysisResult analy

// Make sure we can actually use a collection expression in place of the created collection.
if (!UseCollectionExpressionHelpers.CanReplaceWithCollectionExpression(
semanticModel, creationExpression, expressionType, skipVerificationForReplacedNode: true, cancellationToken))
semanticModel, creationExpression, expressionType, allowInterfaceConversion, skipVerificationForReplacedNode: true, cancellationToken, out var changesSemantics))
{
return null;
}

// Looks good. We can convert this.
return new(memberAccessExpression.Name.Identifier.GetLocation(), localDeclarationStatement, creationExpression, matches.ToImmutable());
return new(memberAccessExpression.Name.Identifier.GetLocation(), localDeclarationStatement, creationExpression, matches.ToImmutable(), changesSemantics);
}

return null;
Expand Down Expand Up @@ -244,5 +245,6 @@ public readonly record struct AnalysisResult(
Location DiagnosticLocation,
LocalDeclarationStatementSyntax LocalDeclarationStatement,
InvocationExpressionSyntax CreationExpression,
ImmutableArray<Match<StatementSyntax>> Matches);
ImmutableArray<Match<StatementSyntax>> Matches,
bool ChangesSemantics);
}
20 changes: 10 additions & 10 deletions ...yzers/UseCollectionExpression/CSharpUseCollectionExpressionForCreateDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,23 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.UseCollectionInitializer;

namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;

using static UseCollectionExpressionHelpers;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal sealed partial class CSharpUseCollectionExpressionForCreateDiagnosticAnalyzer
: AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer
internal sealed partial class CSharpUseCollectionExpressionForCreateDiagnosticAnalyzer()
: AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer(
IDEDiagnosticIds.UseCollectionExpressionForCreateDiagnosticId,
EnforceOnBuildValues.UseCollectionExpressionForCreate)
{
public const string UnwrapArgument = nameof(UnwrapArgument);

private static readonly ImmutableDictionary<string, string?> s_unwrapArgumentProperties =
ImmutableDictionary<string, string?>.Empty.Add(UnwrapArgument, UnwrapArgument);

public CSharpUseCollectionExpressionForCreateDiagnosticAnalyzer()
: base(IDEDiagnosticIds.UseCollectionExpressionForCreateDiagnosticId,
EnforceOnBuildValues.UseCollectionExpressionForCreate)
{
}

protected override void InitializeWorker(CodeBlockStartAnalysisContext<SyntaxKind> context, INamedTypeSymbol? expressionType)
=> context.RegisterSyntaxNodeAction(context => AnalyzeInvocationExpression(context, expressionType), SyntaxKind.InvocationExpression);

Expand All @@ -46,11 +43,14 @@ private void AnalyzeInvocationExpression(SyntaxNodeAnalysisContext context, INam
return;

// Make sure we can actually use a collection expression in place of the full invocation.
if (!CanReplaceWithCollectionExpression(semanticModel, invocationExpression, expressionType, skipVerificationForReplacedNode: true, cancellationToken))
var allowInterfaceConversion = context.GetAnalyzerOptions().PreferCollectionExpressionForInterfaces.Value;
if (!CanReplaceWithCollectionExpression(semanticModel, invocationExpression, expressionType, allowInterfaceConversion, skipVerificationForReplacedNode: true, cancellationToken, out var changesSemantics))
return;

var locations = ImmutableArray.Create(invocationExpression.GetLocation());
var properties = unwrapArgument ? s_unwrapArgumentProperties : null;
var properties = unwrapArgument ? s_unwrapArgumentProperties : ImmutableDictionary<string, string?>.Empty;
if (changesSemantics)
properties = properties.Add(UseCollectionInitializerHelpers.ChangesSemanticsName, "");

context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
Expand Down
19 changes: 7 additions & 12 deletions ...lyzers/UseCollectionExpression/CSharpUseCollectionExpressionForEmptyDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,11 @@ namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;
/// replace with <c>[]</c> if legal to do so.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal sealed partial class CSharpUseCollectionExpressionForEmptyDiagnosticAnalyzer
: AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer
internal sealed partial class CSharpUseCollectionExpressionForEmptyDiagnosticAnalyzer()
: AbstractCSharpUseCollectionExpressionDiagnosticAnalyzer(
IDEDiagnosticIds.UseCollectionExpressionForEmptyDiagnosticId,
EnforceOnBuildValues.UseCollectionExpressionForEmpty)
{
public CSharpUseCollectionExpressionForEmptyDiagnosticAnalyzer()
: base(IDEDiagnosticIds.UseCollectionExpressionForEmptyDiagnosticId,
EnforceOnBuildValues.UseCollectionExpressionForEmpty)
{
}

protected override void InitializeWorker(CodeBlockStartAnalysisContext<SyntaxKind> context, INamedTypeSymbol? expressionType)
=> context.RegisterSyntaxNodeAction(context => AnalyzeMemberAccess(context, expressionType), SyntaxKind.SimpleMemberAccessExpression);

Expand All @@ -48,16 +44,15 @@ private void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, INamedTypeSy
if (nodeToReplace is null)
return;

if (!CanReplaceWithCollectionExpression(semanticModel, nodeToReplace, expressionType, skipVerificationForReplacedNode: true, cancellationToken))
var allowInterfaceConversion = context.GetAnalyzerOptions().PreferCollectionExpressionForInterfaces.Value;
if (!CanReplaceWithCollectionExpression(semanticModel, nodeToReplace, expressionType, allowInterfaceConversion, skipVerificationForReplacedNode: true, cancellationToken, out var changesSemantics))
return;

context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
memberAccess.Name.Identifier.GetLocation(),
option.Notification,
additionalLocations: ImmutableArray.Create(nodeToReplace.GetLocation()),
properties: null));

return;
properties: changesSemantics ? ChangesSemantics : null));
}
}
Loading
Loading