Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
1373fbe
Add IHeaderDictionary.Add analyzer and code fix
david-acker Oct 11, 2022
aabfef9
Change wording from "Disallow-" to "RecommendAgainst-"
david-acker Oct 11, 2022
eb91cd8
Change diagnostic rule ID
david-acker Oct 11, 2022
16332ba
Merge branch 'main' into add-header-dictionary-analyzer
david-acker Oct 13, 2022
2bf902d
Remove analyzer and code fix from AspNetCore.Analyzers project
david-acker Oct 13, 2022
88625b2
Add analyzer to Framework/AspNetCoreAnalyzers
david-acker Oct 13, 2022
dd36127
Add code fix to Framework/AspNetCoreAnalyzers
david-acker Oct 13, 2022
e6190b7
Fix nullable reference type
david-acker Oct 13, 2022
049bde0
Fix diagnostics in the project caused by new analyzer
david-acker Oct 13, 2022
1577df7
Fix using directive insertion logic
david-acker Oct 14, 2022
a2331a8
Fix StringComparison warning
david-acker Oct 14, 2022
d9aff0a
Update src/Analyzers/Analyzers/test/Microsoft.AspNetCore.Analyzers.Te…
david-acker Oct 17, 2022
a7d5124
Update src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/HeaderDic…
david-acker Oct 17, 2022
ed6ddcf
Update src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Http/HeaderDic…
david-acker Oct 17, 2022
faee1d0
Update src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Http/HeaderDic…
david-acker Oct 17, 2022
81f6715
Update src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Http/HeaderDic…
david-acker Oct 17, 2022
c2d8519
Update code fix equivalence key referenced in tests
david-acker Oct 17, 2022
8a0bc4b
Remove redundant test
david-acker Oct 17, 2022
362d172
Merge analyzer and code fix tests into single test file
david-acker Oct 17, 2022
e3caef0
Use top-level statements
david-acker Oct 19, 2022
debb846
Add test cases for multiple diagnostics
david-acker Oct 19, 2022
40bd514
Add comment about IDictionary.Add to diagnostic message
david-acker Oct 19, 2022
e2f0a12
Update diagnostic message
david-acker Oct 19, 2022
a216f87
Move checks before code fix registration
david-acker Oct 21, 2022
1bad1a3
Pass true for getInnermostNodeForTie
david-acker Oct 25, 2022
289260a
Perform symbol comparison for IHeaderDictionary
david-acker Oct 25, 2022
bc3ef4d
Add using directive via syntax annotation
david-acker Oct 27, 2022
620275d
Add editorconfig
david-acker Nov 19, 2022
117ab9e
Revert "Add editorconfig"
david-acker Nov 24, 2022
a57af9b
Skip test on Linux, macOS
david-acker Nov 24, 2022
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
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
Expand All @@ -8,7 +8,7 @@

<ItemGroup>
<ProjectReference Include="$(RepoRoot)src\Analyzers\Analyzers\src\Microsoft.AspNetCore.Analyzers.csproj" />

<Reference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit" />
<Reference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" />
<Reference Include="Microsoft.Extensions.DependencyModel" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,4 +151,13 @@ internal static class DiagnosticDescriptors
DiagnosticSeverity.Info,
isEnabledByDefault: true,
helpLinkUri: "https://aka.ms/aspnet/analyzers");

internal static readonly DiagnosticDescriptor DoNotUseIHeaderDictionaryAdd = new(
"ASP0019",
new LocalizableResourceString(nameof(Resources.Analyzer_HeaderDictionaryAdd_Title), Resources.ResourceManager, typeof(Resources)),
new LocalizableResourceString(nameof(Resources.Analyzer_HeaderDictionaryAdd_Message), Resources.ResourceManager, typeof(Resources)),
"Usage",
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: "https://aka.ms/aspnet/analyzers");
Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue tracking documentation for analyzers?

I see ASP0015, ASP0016, ASP0017, and ASP0018 with the same help link are not yet documented.

Copy link
Member

Choose a reason for hiding this comment

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

@david-acker You can file a docs issue in the docs repo here https://github.com/dotnet/AspNetCore.Docs/issues.

If you're so inclined, you can also submit the doc for this by updating this page.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;

namespace Microsoft.AspNetCore.Analyzers.Http;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public partial class HeaderDictionaryAddAnalyzer : DiagnosticAnalyzer
{
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd);

public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.EnableConcurrentExecution();
context.RegisterOperationAction(context =>
{
var invocation = (IInvocationOperation)context.Operation;

if (invocation.Instance?.Type is INamedTypeSymbol type &&
IsIHeadersDictionaryType(type))
{
if (invocation.TargetMethod.Parameters.Length == 2 &&
IsAddMethod(invocation.TargetMethod))
{
AddDiagnosticWarning(context, invocation.Syntax.GetLocation());
}
}
}, OperationKind.Invocation);
}

private static bool IsIHeadersDictionaryType(INamedTypeSymbol type)
{
// Only IHeaderDictionary is valid. Types like HeaderDictionary, which implement IHeaderDictionary,
// can't access header properties unless cast as IHeaderDictionary.
return type is
{
Name: "IHeaderDictionary",
ContainingNamespace:
{
Name: "Http",
ContainingNamespace:
{
Name: "AspNetCore",
ContainingNamespace:
{
Name: "Microsoft",
ContainingNamespace:
{
IsGlobalNamespace: true
}
}
}
}
};
}

private static bool IsAddMethod(IMethodSymbol method)
{
return method is
{
Name: "Add",
ContainingType:
{
Name: "IDictionary",
ContainingNamespace:
{
Name: "Generic",
ContainingNamespace:
{
Name: "Collections",
ContainingNamespace:
{
Name: "System",
ContainingNamespace:
{
IsGlobalNamespace: true
}
}
}
}
}
};
}

private static void AddDiagnosticWarning(OperationAnalysisContext context, Location location)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd,
location));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,10 @@
<data name="Analyzer_UnusedParameter_Title" xml:space="preserve">
<value>Unused route parameter</value>
</data>
<data name="Analyzer_HeaderDictionaryAdd_Message" xml:space="preserve">
<value>Suggest using IHeaderDictionary.Append or the indexer instead of Add</value>
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a statement here about the consequences of using IDictionary.Add so the user is more informed?

Copy link
Member Author

@david-acker david-acker Oct 19, 2022

Choose a reason for hiding this comment

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

I've updated the diagnostic message to the following:

Use IHeaderDictionary.Append or the indexer to append or set headers. IDictionary.Add will throw an ArgumentException when attempting to add a duplicate key.

Let me know if there are any adjustments you'd like me to make to this.

</data>
<data name="Analyzer_HeaderDictionaryAdd_Title" xml:space="preserve">
<value>Suggest using IHeaderDictionary.Append or the indexer</value>
</data>
</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Immutable;
using System.Composition;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Simplification;


namespace Microsoft.AspNetCore.Analyzers.Http.Fixers;

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

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

public sealed override Task RegisterCodeFixesAsync(CodeFixContext context)
{
foreach (var diagnostic in context.Diagnostics)
{
var appendTitle = "Use IHeaderDictionary.Append";
context.RegisterCodeFix(
CodeAction.Create(appendTitle,
cancellationToken => ReplaceAddWithAppendAsync(diagnostic, context.Document, cancellationToken),
equivalenceKey: appendTitle),
diagnostic);

var indexerTitle = "Use the indexer";
context.RegisterCodeFix(
CodeAction.Create(indexerTitle,
cancellationToken => ReplaceAddWithIndexerAsync(diagnostic, context.Document, cancellationToken),
equivalenceKey: indexerTitle),
diagnostic);
}

return Task.CompletedTask;
}

private static async Task<Document> ReplaceAddWithAppendAsync(Diagnostic diagnostic, Document document, CancellationToken cancellationToken)
{
var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
if (root is not CompilationUnitSyntax compilationUnitSyntax)
{
return document;
}

var invocation = compilationUnitSyntax.FindNode(diagnostic.Location.SourceSpan);

if (invocation is InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax { Name.Identifier: { } identifierToken } })
{
compilationUnitSyntax = compilationUnitSyntax.ReplaceToken(identifierToken, SyntaxFactory.Identifier("Append"));
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but could you check if SyntaxFactory.Identifier("Append").WithAdditionalAnnotations(Simplifier.AddImportsAnnotation) will avoid the need to manually add the using?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I just tried this but it doesn't seem to add the using. Is there anything else that I would need to do here besides adding the call to WithAdditionalAnnotations(Simplifier.AddImportsAnnotation) on the identifier?

Copy link
Member

Choose a reason for hiding this comment

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

@david-acker Try getting the symbol for Microsoft.AspNetCore.Http.HeaderDictionaryExtensions and create an annotation like the following:

var annotation = new SyntaxAnnotation("SymbolId", DocumentationCommentId.CreateReferenceId(symbol));

Then use SyntaxFactory.Identifier("Append").WithAdditionalAnnotations(Simplifier.AddImportsAnnotation, annotation)


// IHeaderDictionary.Append is defined as an extension method on Microsoft.AspNetCore.Http.HeaderDictionaryExtensions.
// We'll need to add the required using directive, when not already present.
compilationUnitSyntax = AddRequiredUsingDirectiveForAppend(compilationUnitSyntax);

return document.WithSyntaxRoot(compilationUnitSyntax);
}

return document;
}

private static CompilationUnitSyntax AddRequiredUsingDirectiveForAppend(CompilationUnitSyntax compilationUnitSyntax)
{
var usingDirectives = compilationUnitSyntax.Usings;

var includesRequiredUsingDirective = false;
var insertionIndex = 0;

for (var i = 0; i < usingDirectives.Count; i++)
{
var namespaceName = usingDirectives[i].Name.ToString();

// Always insert the new using directive after any 'System' using directives.
if (namespaceName.StartsWith("System", StringComparison.Ordinal))
{
insertionIndex = i + 1;
continue;
}

var result = string.Compare("Microsoft.AspNetCore.Http", namespaceName, StringComparison.Ordinal);

if (result == 0)
{
includesRequiredUsingDirective = true;
break;
}

if (result < 0)
{
insertionIndex = i;
break;
}
}

if (includesRequiredUsingDirective)
{
return compilationUnitSyntax;
}

var requiredUsingDirective =
SyntaxFactory.UsingDirective(
SyntaxFactory.QualifiedName(
SyntaxFactory.QualifiedName(
SyntaxFactory.IdentifierName("Microsoft"),
SyntaxFactory.IdentifierName("AspNetCore")),
SyntaxFactory.IdentifierName("Http")));

return compilationUnitSyntax.WithUsings(
usingDirectives.Insert(insertionIndex, requiredUsingDirective));
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static CompilationUnitSyntax AddRequiredUsingDirectiveForAppend(CompilationUnitSyntax compilationUnitSyntax)
{
var usingDirectives = compilationUnitSyntax.Usings;
var includesRequiredUsingDirective = false;
var insertionIndex = 0;
for (var i = 0; i < usingDirectives.Count; i++)
{
var namespaceName = usingDirectives[i].Name.ToString();
// Always insert the new using directive after any 'System' using directives.
if (namespaceName.StartsWith("System", StringComparison.Ordinal))
{
insertionIndex = i + 1;
continue;
}
var result = string.Compare("Microsoft.AspNetCore.Http", namespaceName, StringComparison.Ordinal);
if (result == 0)
{
includesRequiredUsingDirective = true;
break;
}
if (result < 0)
{
insertionIndex = i;
break;
}
}
if (includesRequiredUsingDirective)
{
return compilationUnitSyntax;
}
var requiredUsingDirective =
SyntaxFactory.UsingDirective(
SyntaxFactory.QualifiedName(
SyntaxFactory.QualifiedName(
SyntaxFactory.IdentifierName("Microsoft"),
SyntaxFactory.IdentifierName("AspNetCore")),
SyntaxFactory.IdentifierName("Http")));
return compilationUnitSyntax.WithUsings(
usingDirectives.Insert(insertionIndex, requiredUsingDirective));
}

private static async Task<Document> ReplaceAddWithIndexerAsync(Diagnostic diagnostic, Document document, CancellationToken cancellationToken)
{
var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
if (root == null)
{
return document;
}

var invocation = root.FindNode(diagnostic.Location.SourceSpan);

if (invocation is InvocationExpressionSyntax
{
Expression: MemberAccessExpressionSyntax memberAccessExpression,
ArgumentList.Arguments: { Count: 2 } arguments
})
{
var assignmentExpression =
SyntaxFactory.AssignmentExpression(
SyntaxKind.SimpleAssignmentExpression,
SyntaxFactory.ElementAccessExpression(
memberAccessExpression.Expression,
SyntaxFactory.BracketedArgumentList(
SyntaxFactory.SeparatedList(new[] { arguments[0] }))),
arguments[1].Expression);

return document.WithSyntaxRoot(root.ReplaceNode(invocation, assignmentExpression));
}

return document;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.CodeAnalysis.Testing;
using VerifyCS = Microsoft.AspNetCore.Analyzers.Verifiers.CSharpAnalyzerVerifier<
Microsoft.AspNetCore.Analyzers.Http.HeaderDictionaryAddAnalyzer>;

namespace Microsoft.AspNetCore.Analyzers.Http;

public class HeaderDictionaryAddAnalyzerTests
{
[Fact]
public async Task IHeaderDictionary_WithAdd_ReportsDiagnostics()
{
// Arrange & Act & Assert
await VerifyCS.VerifyAnalyzerAsync(@"
using Microsoft.AspNetCore.Http;
namespace HeaderDictionaryAddAnalyzerTests;
public class Program
{
public static void Main()
{
var context = new DefaultHttpContext();
{|#0:context.Request.Headers.Add(""Accept"", ""text/html"")|};
}
}",
new DiagnosticResult(DiagnosticDescriptors.DoNotUseIHeaderDictionaryAdd)
.WithLocation(0)
.WithMessage(Resources.Analyzer_HeaderDictionaryAdd_Message));
}

[Fact]
public async Task IHeaderDictionary_WithAppend_NoDiagnostics()
{
// Arrange & Act & Assert
await VerifyCS.VerifyAnalyzerAsync(@"
using Microsoft.AspNetCore.Http;
namespace HeaderDictionaryAddAnalyzerTests;
public class Program
{
public static void Main()
{
var context = new DefaultHttpContext();
context.Request.Headers.Append(""Accept"", ""text/html"");
}
}");
}

[Fact]
public async Task IHeaderDictionary_WithIndexer_NoDiagnostics()
{
// Arrange & Act & Assert
await VerifyCS.VerifyAnalyzerAsync(@"
using Microsoft.AspNetCore.Http;
namespace HeaderDictionaryAddAnalyzerTests;
public class Program
{
public static void Main()
{
var context = new DefaultHttpContext();
context.Request.Headers[""Accept""] = ""text/html"";
}
}");
}
}
Loading