search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.89k stars 4.01k forks source link

Reference Microsoft.NetFramework.Analyzers and fix violations #33197

Open davidwengier opened 5 years ago

davidwengier commented 5 years ago

Some of the SDL and security related analyzers are currently not enabled for Roslyn. For full security coverage we should reference Microsoft.NetFramework.Analyzers and fix any existing violations that come up.

Two of the diagnostics are covered by https://devdiv.visualstudio.com/DevDiv/_workitems/edit/788330 and https://devdiv.visualstudio.com/DevDiv/_workitems/edit/788328

From @jaredpar:

I don't think we can take a 7% build regression for this. That would impact the inner productivity loop of the team too much. I think we should either take the fixes and push on performance before we merge this in. Or alternatively if we must run this now then we should limit the running of this analyzer to the build correctness leg. That leg is already used for such types of extra build validation and given it compiles 100% of our code it would meet whatever SDL requirement we're trying to hit here.

Performance improvements to the analyzer are tracked by https://github.com/dotnet/roslyn-analyzers/issues/2103 or alternatively running these analyzers on the build correctness leg in CI is fine.

davidwengier commented 5 years ago

As mentioned in the PR, I was planning on opening this issue to begin discussion, but the work needed turned out to be so small I just did it. Feel free to have said discussion on the PR if preferred.

sharwell commented 5 years ago

The last time I measured them, the security analyzers were horrifically slow, to the point that it would be detrimental to productivity to attempt to include them by default in a project the size of Roslyn. I'm going to mark this as blocked while we review the developer impact of this move.

davidwengier commented 5 years ago

Making this only run on CI builds is fine, though could be annoying to a developer if they have a successful local build which then fails in CI. Removing the analyzers altogether would presumably require some alternative static analysis solution, or a tenet exception and director approval. The exception process is outlined here: https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki?wikiVersion=GBwikiMaster&pagePath=%2FCompliance%20%26%20Tenets%2FTenet%20Exception%20Process&pageId=1779

sharwell commented 5 years ago

:memo: I moved the performance discussion to the implementation PR