Add analyzer and code fix to recommend against IHeaderDictionary.Add

[david-acker]

Description

• Adds an analyzer which identifies invocations of IHeaderDictionary.Add.
• Adds two code fixes, one for replacing Add with Append and another for replacing Add with the indexer.

Questions

• Is there a preferred architecture or way of organizing new analyzers and code fixes? The analyzers and codes fixes in AspNetCore.Analyzers, Mvc.Analyzers, Mvc.Api.Analyzers, and Framework/AspNetCoreAnalyzers seemed to all have slightly different styles and ways for handling the analyzers, code fixes, tests, etc. I pulled from these when creating this draft, but didn't follow the style of any one of them too rigidly. [Resolved: Moved to Framework/AspNetCoreAnalyzers and used the analyzer and code fix pattern used there.]
• What's the process for picking a rule ID for the new diagnostic? I used ASP0015 as a placeholder for now as this appeared to be the next one available, based on this list of diagnostics: Code analysis in ASP.NET Core apps. [Updated to use ASP0019]

Fixes #41362

[ghost]

Thanks for your PR, @david-acker. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[adityamandaleeka]

@david-acker Can you please put it in src/Framework/AspNetCoreAnalyzers and follow the pattern used there?

As for the ID, for now we're just incrementing the number (see https://github.com/dotnet/aspnetcore/blob/main/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs).

[david-acker]

@captainsafia @Youssef1313 Sounds good! I'll address the remaining comments within the next few days.

I'll also file an issue in the docs repo and submit a PR to update the diagnostics doc page that's linked above.

[adityamandaleeka]

@david-acker Looks like there's some test failures.

[david-acker]

@adityamandaleeka Hmm, that's odd. I'm not getting those test failures locally. I'll dig into this a bit.

[captainsafia]

@adityamandaleeka Hmm, that's odd. I'm not getting those test failures locally. I'll dig into this a bit.

Seems to be a pesky line-ending issue. I wonder if there is a way to get the verifier to ignore these when comparing texts...

[Youssef1313]

@adityamandaleeka Hmm, that's odd. I'm not getting those test failures locally. I'll dig into this a bit.

Seems to be a pesky line-ending issue. I wonder if there is a way to get the verifier to ignore these when comparing texts...

I'm not sure if adding an editorconfig to the tests with end_of_line = crlf can fix the failure (or if it's the correct action).

cc @sharwell @CyrusNajmabadi for suggestions.

[Youssef1313]

@captainsafia Could the failing test be skipped on Linux and get the PR merged?

[captainsafia]

@captainsafia Could the failing test be skipped on Linux and get the PR merged?

I'd like to see if trying

I'm not sure if adding an editorconfig to the tests with end_of_line = crlf can fix the failure (or if it's the correct action).

might help here.

We've had issues before where analyzer tests were not running and resulted in some unsavory unhandled exceptions. Let me see if I can tweak this PR to flow it along.

[BrennanConroy]

Could the failing test be skipped on Linux and get the PR merged?

@david-acker Could you try that out? Apply the OSSkipCondition to the affected tests: https://github.com/dotnet/aspnetcore/blob/de3019b7dab5b5995942db1fbec6aa466c4af34c/src/Middleware/Diagnostics.EntityFrameworkCore/test/FunctionalTests/DatabaseErrorPageMiddlewareTest.cs#L97

[david-acker]

I updated the failing test to be skipped on Linux and macOS. I also reverted the last commit which added the .editorconfig file.

[captainsafia]

@david-acker Thanks for submitting this PR! Documentation has already been handled in https://github.com/dotnet/aspnetcore/issues/45025#issuecomment-1326100598. Feel free to submit any updates to the docs if you'd like.