search

dotnet / roslyn-analyzers

MIT License
1.59k stars 466 forks source link

Enrich analyzers with additional context #5332

Open msschl opened 3 years ago

msschl commented 3 years ago

As I understood from the ConfigureAwait FAQ written by @stephentoub it is unnecessary to call .ConfigureAwait(false) in ASP Net Core App code.

For people building projects with all analyzers enabled like we do as follow:

<PropertyGroup>
  <EnableNETAnalyzers>true</EnableNETAnalyzers>
  <AnalysisMode>AllEnabledByDefault</AnalysisMode>
  <AnalysisLevel>latest</AnalysisLevel>
  <CodeAnalysisTreatWarningsAsErrors>true</CodeAnalysisTreatWarningsAsErrors>
  <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

Overview of .NET source code analysis

This will result in some analyzers firing for some types of applications or scenarios where they probably shouldn’t have. For example, for ConfigureAwait CA2007 'Consider calling ConfigureAwait on the awaited task' as mentioned above in ASP Net Core application code.

Further, as I think @bartonjs stated in the latest GitHub Quick Reviews on .net analyzers: “[…] a lot of the analyzers we have are also only applicable to like certain project types and certain scenarios.”. This started me thinking whether analyzers could be enriched with additional context like the sdk id from the csproj (i.e., Micorosoft.NET.Sdk.Web) or other additional context to additionally improve the false positives.

Just an idea I had and wanted to throw out here for discussion. Also I don't know how analyzers work under the hood and if this is even possible.

bartonjs commented 3 years ago

Happy to be quoted, but that wasn't me :smile:. That is the voice of @GrabYourPitchforks.

msschl commented 3 years ago

I‘m sorry 😂 Yes, you’re right

sharwell commented 3 years ago

AllEnabledByDefault

This literally means All Enabled By Default. It includes all analyzers, regardless of whether they are applicable to the current project type or are recommended for general use (e.g. some analyzers are candidates for removal, but still exist and are enabled by this flag).

Use of this option requires the project maintainer manually review all rules for applicability in the context of their current project, and selectively disable rules they do not wish to use.

msschl commented 3 years ago

From dotnet/runtime#44194:

There are certain legacy analyzers shipped with NetAnalyzers, but not included in AllEnabledByDefault (CA1021, CA1045)

So when AllEnabledByDefault already doesn’t include all analyzers why not disable certain analyzers that are not applicable to certain project types or scenarios. Or maybe introducing a new AnalysisMode could do the trick?

sharwell commented 3 years ago

💭 Interesting. I wasn't aware of those exclusions.

Youssef1313 commented 3 years ago

The web SDK sets UsingMicrosoftNETSdkWeb MSBuild property to true, so this information is already available to analyzers:

https://github.com/dotnet/roslyn-analyzers/blob/87c20b98866d61c1bbc574ec95da07babc1eb15a/src/Utilities/Compiler/Extensions/CompilationExtensions.cs#L27-L49

The only analyzer that currently accounts for web projects is CA1822, other analyzers could account for that when appropriate.

msschl commented 3 years ago

@Youssef1313 that sounds good. 👍🏽 So it looks like analyzers are able to retrieve MSBuild properties.

However, I would like to point out that I am not only referring to enriching analyzers in context of web projects, but also in context of other types of applications such as desktop applications (WinForms, WPF), blazor applications, or libraries. The analyzer mentioned above CA2007 was only used as an example and to start a discussion. 😄

Youssef1313 commented 3 years ago

I think the idea is the same. WPF sets UseWPF property to true and WinForms sets UseWindowsForms to true.

It'd be helpful if you're aware of other analyzers that shouldn't execute in WinForms, WPF, or ASP.NET Core.

msschl commented 3 years ago

I think the idea is the same. WPF sets UseWPF property to true and WinForms sets UseWindowsForms to true.

Yes, I think so.

It'd be helpful if you're aware of other analyzers that shouldn't execute in WinForms, WPF, or ASP.NET Core.

Well off the top of my head, for ASP.NET Core probably CA1056, CA2234, as for example in Mvc the url parameters and properties are all string based. See for example ControllerBase.cs#L356 in Mvc