Skip to content

Commit

Permalink
Suggest ImmutableArray<T> as a base type for sequences
Browse files Browse the repository at this point in the history
  • Loading branch information
@DoctorKrolic
DoctorKrolic committed Mar 17, 2024
1 parent 6f04677 commit 10ed225
Show file tree
Hide file tree
Showing 5 changed files with 187 additions and 31 deletions.
65 changes: 65 additions & 0 deletions src/ArgumentParsing.CodeFixes/ChangePropertyTypeToImmutableArrayCodeFixProvider.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
using System.Collections.Immutable;
using System.Composition;
using System.Diagnostics;
using ArgumentParsing.Generators.Diagnostics;
using ArgumentParsing.Generators.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;

namespace ArgumentParsing.CodeFixes;

[ExportCodeFixProvider(LanguageNames.CSharp), Shared]
public sealed class ChangePropertyTypeToImmutableArrayCodeFixProvider : CodeFixProvider
{
public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(DiagnosticDescriptors.PreferImmutableArrayAsSequenceType.Id);

public override FixAllProvider? GetFixAllProvider()
=> WellKnownFixAllProviders.BatchFixer;

public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var document = context.Document;
var root = await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);

if (root?.FindNode(context.Span) is TypeSyntax propertyTypeSyntax)
{
context.RegisterCodeFix(
CodeAction.Create(
"Change property type to ImmutableArray<T>",
ct => ChangePropertyTypeToImmutableArray(document, root, propertyTypeSyntax, ct),
nameof(ChangePropertyTypeToImmutableArrayCodeFixProvider)),
context.Diagnostics[0]);
}
}

private static async Task<Document> ChangePropertyTypeToImmutableArray(Document document, SyntaxNode root, TypeSyntax propertyTypeSyntax, CancellationToken cancellationToken)
{
var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
if (semanticModel is null)
{
Debug.Fail("Shouldn't really happen");
return document;
}

var immutableArrayOfTType = semanticModel.Compilation.ImmutableArrayOfTType();
Debug.Assert(immutableArrayOfTType is not null);

var propertyType = semanticModel.GetTypeInfo(propertyTypeSyntax).Type!;
Debug.Assert(propertyType.OriginalDefinition.SpecialType is SpecialType.System_Collections_Generic_IEnumerable_T
or SpecialType.System_Collections_Generic_IReadOnlyCollection_T
or SpecialType.System_Collections_Generic_IReadOnlyList_T);

var sequenceUnderlyingType = ((INamedTypeSymbol)propertyType).TypeArguments[0];
var immutableArrayOfSequenceType = immutableArrayOfTType!.Construct(sequenceUnderlyingType);

var generator = SyntaxGenerator.GetGenerator(document);
var finalTypeSyntax = generator.TypeExpression(immutableArrayOfSequenceType, addImport: true);

var changedRoot = root.ReplaceNode(propertyTypeSyntax, finalTypeSyntax);
return document.WithSyntaxRoot(changedRoot);
}
}
1 change: 1 addition & 0 deletions src/ArgumentParsing.Generators/AnalyzerReleases.Unshipped.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
ARGP0011 | ArgumentParsing | Info |
ARGP0021 | ArgumentParsing | Info |
ARGP0034 | ArgumentParsing | Error |
ARGP0035 | ArgumentParsing | Warning |
ARGP0036 | ArgumentParsing | Error |
58 changes: 38 additions & 20 deletions src/ArgumentParsing.Generators/Diagnostics/Analyzers/OptionsTypeAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public sealed class OptionsTypeAnalyzer : DiagnosticAnalyzer
// ARGP0018
DiagnosticDescriptors.RequiredBoolOption,
DiagnosticDescriptors.RequiredNullableOption,
// ARGP0021
DiagnosticDescriptors.PreferImmutableArrayAsSequenceType,
DiagnosticDescriptors.NegativeParameterIndex,
DiagnosticDescriptors.DuplicateParameterIndex,
DiagnosticDescriptors.InvalidParameterPropertyType,
Expand All @@ -38,6 +38,7 @@ public sealed class OptionsTypeAnalyzer : DiagnosticAnalyzer
DiagnosticDescriptors.InvalidParameterName,
DiagnosticDescriptors.DuplicateRemainingParameters,
DiagnosticDescriptors.InvalidRemainingParametersPropertyType,
// ARGP0031
DiagnosticDescriptors.TooLowAccessibilityOfOptionsType,
DiagnosticDescriptors.NoOptionNames,
DiagnosticDescriptors.PropertyCannotHaveMultipleParserRoles);
Expand Down Expand Up @@ -340,18 +341,18 @@ property.SetMethod is not null &&
}
}

var (parseStrategy, isNullable, _) = GetParseStrategy(propertyType, knownTypes);
var (parseStrategy, isNullable, sequenceType) = GetParseStrategy(propertyType, knownTypes);
var propertySyntax = (BasePropertyDeclarationSyntax?)property.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken);
var locationForTypeRelatedDiagnostics = propertySyntax?.Type.GetLocation() ?? propertyLocation;

if (parseStrategy == ParseStrategy.None)
{
if (propertyType.TypeKind != TypeKind.Error)
{
var propertySyntax = (BasePropertyDeclarationSyntax?)property.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken);

context.ReportDiagnostic(
Diagnostic.Create(
DiagnosticDescriptors.InvalidOptionPropertyType,
propertySyntax?.Type.GetLocation() ?? propertyLocation,
locationForTypeRelatedDiagnostics,
propertyType));
}

Expand All @@ -371,6 +372,13 @@ property.SetMethod is not null &&
Diagnostic.Create(
DiagnosticDescriptors.RequiredNullableOption, propertyLocation));
}

if (sequenceType == SequenceType.List && knownTypes.ImmutableArrayOfTType is not null)
{
context.ReportDiagnostic(
Diagnostic.Create(
DiagnosticDescriptors.PreferImmutableArrayAsSequenceType, locationForTypeRelatedDiagnostics));
}
}
else if (isParameter)
{
Expand Down Expand Up @@ -418,8 +426,8 @@ property.SetMethod is not null &&
DiagnosticDescriptors.InvalidParameterName, propertyLocation, parameterName));
}

var (parseStrategy, isNullable, isSequence) = GetParseStrategy(propertyType, knownTypes);
if ((parseStrategy == ParseStrategy.None || isSequence) && propertyType.TypeKind != TypeKind.Error)
var (parseStrategy, isNullable, sequenceType) = GetParseStrategy(propertyType, knownTypes);
if ((parseStrategy == ParseStrategy.None || sequenceType != SequenceType.None) && propertyType.TypeKind != TypeKind.Error)
{
var propertySyntax = (BasePropertyDeclarationSyntax?)property.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken);

Expand Down Expand Up @@ -462,15 +470,24 @@ property.SetMethod is not null &&

declaredRemainingParameters = true;

var (parseStrategy, isNullable, isSequence) = GetParseStrategy(propertyType, knownTypes);
if ((parseStrategy == ParseStrategy.None || isNullable || !isSequence) && propertyType.TypeKind != TypeKind.Error)
{
var propertySyntax = (BasePropertyDeclarationSyntax?)property.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken);
var diagnosticLocation = propertySyntax?.Type.GetLocation() ?? propertyLocation;
var (parseStrategy, isNullable, sequenceType) = GetParseStrategy(propertyType, knownTypes);
var propertySyntax = (BasePropertyDeclarationSyntax?)property.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken);
var locationForTypeRelatedDiagnostics = propertySyntax?.Type.GetLocation() ?? propertyLocation;

if (parseStrategy == ParseStrategy.None || isNullable || sequenceType == SequenceType.None)
{
if (propertyType.TypeKind != TypeKind.Error)
{
context.ReportDiagnostic(
Diagnostic.Create(
DiagnosticDescriptors.InvalidRemainingParametersPropertyType, locationForTypeRelatedDiagnostics));
}
}
else if (sequenceType != SequenceType.ImmutableArray)
{
context.ReportDiagnostic(
Diagnostic.Create(
DiagnosticDescriptors.InvalidRemainingParametersPropertyType, diagnosticLocation));
DiagnosticDescriptors.PreferImmutableArrayAsSequenceType, locationForTypeRelatedDiagnostics));
}
}
}
Expand Down Expand Up @@ -529,15 +546,15 @@ property.SetMethod is not null &&
}
}

static (ParseStrategy, bool IsNullable, bool IsSequence) GetParseStrategy(ITypeSymbol type, KnownTypes knownTypes)
static (ParseStrategy, bool IsNullable, SequenceType) GetParseStrategy(ITypeSymbol type, KnownTypes knownTypes)
{
if (type is not INamedTypeSymbol namedType)
{
return default;
}

var isNullable = false;
var isSequence = false;
var sequenceType = SequenceType.None;

if (namedType is { ConstructedFrom.SpecialType: SpecialType.System_Nullable_T, TypeArguments: [var nullableUnderlyingType] })
{
Expand All @@ -555,11 +572,12 @@ property.SetMethod is not null &&
else
{
var constructedFrom = namedType.ConstructedFrom;
var isImmutableArray = constructedFrom.Equals(knownTypes.ImmutableArrayOfTType, SymbolEqualityComparer.Default);

if (constructedFrom.Equals(knownTypes.IEnumerableOfTType, SymbolEqualityComparer.Default) ||
if (isImmutableArray ||
constructedFrom.Equals(knownTypes.IEnumerableOfTType, SymbolEqualityComparer.Default) ||
constructedFrom.Equals(knownTypes.IReadOnlyCollectionOfTType, SymbolEqualityComparer.Default) ||
constructedFrom.Equals(knownTypes.IReadOnlyListOfTType, SymbolEqualityComparer.Default) ||
constructedFrom.Equals(knownTypes.ImmutableArrayOfTType, SymbolEqualityComparer.Default))
constructedFrom.Equals(knownTypes.IReadOnlyListOfTType, SymbolEqualityComparer.Default))
{
if (namedType.TypeArguments is [INamedTypeSymbol namedSequenceUnderlyingType])
{
Expand All @@ -570,11 +588,11 @@ property.SetMethod is not null &&
return default;
}

isSequence = true;
sequenceType = isImmutableArray ? SequenceType.ImmutableArray : SequenceType.List;
}
}

return (namedType.GetPrimaryParseStrategy(), isNullable, isSequence);
return (namedType.GetPrimaryParseStrategy(), isNullable, sequenceType);
}
}

Expand Down
8 changes: 7 additions & 1 deletion src/ArgumentParsing.Generators/Diagnostics/DiagnosticDescriptors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,13 @@ public static class DiagnosticDescriptors
defaultSeverity: DiagnosticSeverity.Warning,
isEnabledByDefault: true);

// ARGP0021 - suggest switching sequence option to immutable array if possible
public static readonly DiagnosticDescriptor PreferImmutableArrayAsSequenceType = new(
id: "ARGP0021",
title: "Prefer ImmutableArray<T> as a sequence type",
messageFormat: "Prefer ImmutableArray<T> type for sequence options and remaining parameters",
category: ArgumentParsingCategoryName,
defaultSeverity: DiagnosticSeverity.Info,
isEnabledByDefault: true);

public static readonly DiagnosticDescriptor NegativeParameterIndex = new(
id: "ARGP0022",
Expand Down
86 changes: 76 additions & 10 deletions tests/ArgumentParsing.Tests.Unit/OptionsTypeAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,39 @@ class MyOptions
await VerifyAnalyzerAsync(source);
}

[Theory]
[InlineData("IEnumerable", "string")]
[InlineData("IReadOnlyCollection", "int")]
[InlineData("IReadOnlyList", "char")]
public async Task SuggestImmutableArrayForSequenceOptionType(string sequenceBaseType, string sequenceUnderlyingType)
{
var source = $$"""
using System.Collections.Generic;
using System.Collections.Immutable;
[OptionsType]
class MyOptions
{
[Option]
public required {|ARGP0021:{{sequenceBaseType}}<{{sequenceUnderlyingType}}>|} Option { get; init; }
}
""";

var fixedSource = $$"""
using System.Collections.Generic;
using System.Collections.Immutable;
[OptionsType]
class MyOptions
{
[Option]
public required ImmutableArray<{{sequenceUnderlyingType}}> Option { get; init; }
}
""";

await VerifyAnalyzerWithCodeFixAsync<ChangePropertyTypeToImmutableArrayCodeFixProvider>(source, fixedSource);
}

[Fact]
public async Task NegativeParameterIndex()
{
Expand Down Expand Up @@ -1027,16 +1060,16 @@ class MyOptions
public async Task DuplicateRemainingParameters()
{
var source = """
using System.Collections.Generic;
using System.Collections.Immutable;
[OptionsType]
class MyOptions
{
[RemainingParameters]
public IEnumerable<string> {|ARGP0029:A|} { get; init; }
public ImmutableArray<string> {|ARGP0029:A|} { get; init; }
[RemainingParameters]
public IEnumerable<string> {|ARGP0029:B|} { get; init; }
public ImmutableArray<string> {|ARGP0029:B|} { get; init; }
}
""";

Expand All @@ -1047,19 +1080,19 @@ class MyOptions
public async Task DuplicateRemainingParameters_DuplicatesInDifferentPartialDeclarations()
{
var source = """
using System.Collections.Generic;
using System.Collections.Immutable;
[OptionsType]
partial class MyOptions
{
[RemainingParameters]
public IEnumerable<string> {|ARGP0029:A|} { get; init; }
public ImmutableArray<string> {|ARGP0029:A|} { get; init; }
}
partial class MyOptions
{
[RemainingParameters]
public IEnumerable<string> {|ARGP0029:B|} { get; init; }
public ImmutableArray<string> {|ARGP0029:B|} { get; init; }
}
""";

Expand All @@ -1070,19 +1103,19 @@ partial class MyOptions
public async Task DuplicateRemainingParameters_ThreeDuplicates()
{
var source = """
using System.Collections.Generic;
using System.Collections.Immutable;
[OptionsType]
class MyOptions
{
[RemainingParameters]
public IEnumerable<string> {|ARGP0029:A|} { get; init; }
public ImmutableArray<string> {|ARGP0029:A|} { get; init; }
[RemainingParameters]
public IEnumerable<string> {|ARGP0029:B|} { get; init; }
public ImmutableArray<string> {|ARGP0029:B|} { get; init; }
[RemainingParameters]
public IEnumerable<string> {|ARGP0029:C|} { get; init; }
public ImmutableArray<string> {|ARGP0029:C|} { get; init; }
}
""";

Expand Down Expand Up @@ -1126,6 +1159,39 @@ class MyOptions
await VerifyAnalyzerAsync(source);
}

[Theory]
[InlineData("IEnumerable", "string")]
[InlineData("IReadOnlyCollection", "int")]
[InlineData("IReadOnlyList", "char")]
public async Task SuggestImmutableArrayForRemainingParametersType(string sequenceBaseType, string sequenceUnderlyingType)
{
var source = $$"""
using System.Collections.Generic;
using System.Collections.Immutable;
[OptionsType]
class MyOptions
{
[RemainingParameters]
public required {|ARGP0021:{{sequenceBaseType}}<{{sequenceUnderlyingType}}>|} Option { get; init; }
}
""";

var fixedSource = $$"""
using System.Collections.Generic;
using System.Collections.Immutable;
[OptionsType]
class MyOptions
{
[RemainingParameters]
public required ImmutableArray<{{sequenceUnderlyingType}}> Option { get; init; }
}
""";

await VerifyAnalyzerWithCodeFixAsync<ChangePropertyTypeToImmutableArrayCodeFixProvider>(source, fixedSource);
}

[Theory]
[InlineData("")]
[InlineData("private")]
Expand Down

0 comments on commit 10ed225

Please sign in to comment.