Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to disable analyzers via MSBuild property 'RunAnalyzers' #46471

Closed
wants to merge 2 commits into from

Conversation

mavasani
Copy link
Contributor

Fixes #40926

This support was added to Microsoft.CodeAnalysis.targets (ships with MSBuild in VS) in VS2019 16.5:
https://docs.microsoft.com/visualstudio/code-quality/disable-code-analysis?view=vs-2019.

However, the MSBuild property is not respected for 'dotnet' builds where this targets file is not imported (see #40926 (comment) for details). This change moves the disable analyzers logic down to Microsoft.Managed.Core.targets so it is respected for all builds.

Fixes dotnet#40926

This support was added to `Microsoft.CodeAnalysis.targets` (ships with VS) in VS2019 16.5:
https://docs.microsoft.com/en-us/visualstudio/code-quality/disable-code-analysis?view=vs-2019.

However, the MSBuild property is not respected from 'dotnet' builds where this targets file is not imported (see dotnet#40926 (comment) for details). This change moves the disable analyzers logic down to `Microsoft.Managed.Core.targets` so it is respected for all builds.
@mavasani
Copy link
Contributor Author

@dotnet/roslyn-compiler for an additional review.

1 similar comment
@mavasani
Copy link
Contributor Author

mavasani commented Aug 3, 2020

@dotnet/roslyn-compiler for an additional review.

@jaredpar
Copy link
Member

jaredpar commented Aug 3, 2020

@chsienki can u take a look at this since Rikki is OOF today?


<!-- Remove all analyzers for non-design time builds. -->
<ItemGroup Condition="'$(_RunAnalyzers)' == 'false' and '$(_IsAnyDesignTimeBuild)' != 'true'">
<Analyzer Remove="@(Analyzer)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

My Concern with this is the same as #46300

Because we pass generators in via analyzers, builds can be potentially broken by this now. I think before it was fine: an analyzer can only take a successful build -> failed, but generators are very much the other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm we already have shipping functionality in VS based on such properties: https://docs.microsoft.com/en-us/visualstudio/code-quality/disable-code-analysis?view=vs-2019. This is done by this targets file that ships with MSBuild in VS. This change is just ensuring the existing functionality works fine from dotnet builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I don't think its necessarily an analyzer problem.

I think there is a separate issue here though, that saying 'Don't run analyzers' is also going to prevent generators from running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, seems like we can take couple of paths forward here:

  1. Abandon this PR and instead have a separate command line compiler flag, say skipAnalyzers so the build skips analyzer execution, but not generators.
  2. Keep the approach in this PR, and break the Analyzer collection into two separate collections Analyzer and Generator, so this change will not affect generators. However, if user ships both analyzers and generators in the same assembly, then they will be affected.

@mavasani
Copy link
Contributor Author

Closing in favor of #46669

@mavasani mavasani closed this Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSBuild parameter -p:RunAnalyzers=false is ignored
4 participants