Roslyn: In Roslyn have TreatWarningsAsErrors enabled for compilation but not for analysis

[wpanja]

Version Used:

VS 2015 Update 3

Steps to Reproduce:

N/A Currently, Can be provided if required.

Expected Behavior:

In Roslyn have TreatWarningsAsErrors enabled for compilation but not for analysis

Actual Behavior:

Roslyn Analyzer honor TreatWarningsAsErrors. As a result customer cannot move from FxCop to Roslyn Analyzers, since they get quite a lot of errors from analyzer. And they don’t want to switch TreatWarningsAsErrors to false since they want to have “warning-free” compilation. So question is how to have TreatWarningsAsErrors enabled for compilation but not for analysis?

@mavasani

[mavasani]

@gafter @srivatsn Do you think this is feasible for 15.1 timeframe? The idea is to probably add a new compiler switch to control treat warn as error separately for analyzer diagnostics versus compiler diagnostics. Customer is saying this issue is blocking them from moving from FxCop to FxCop based Roslyn analyzers.

[tom-dudley]

I would also be keen on having this behaviour. We would like to be able to utilise the Warning severity level for analyzer rules but to have non-analyzer warnings (csc and msbuild warnings) treated as errors.

• Turning on TreatWarningsAsErrors means we can't try out new rules by giving them the Warning action since they will fail the build.
• Turning off TreatWarningsAsErrors means csc and msbuild warnings can work their way into the code.

[plainionist]

Is there any update available on that topic? Any idea when a fix could be available?
thx

[mavasani]

Tagging @jinujoseph

[SDavydkin]

Dear team,

I've reported this problem via premium support on January 2017 for VS 2015.
For some period of time support promised including fix into the upcoming VS update.
Now is October 2017 and VS is 15.8.x but issue still is open.
Do you have any realistic ETA on it now?

Thanks,
Sergey Davydkin
Epicor Software Corporation

[mavasani]

Tagging @jcouv as we discussed the possible design for this today morning:

1. Enhance /warnaserror switch to take arguments which are diagnostic ID prefixes, such as say /warnaserror:CS* or /warnaserror:BC* or some such format which indicates that all warning diagnostics whose ID starts with that prefix need to be escalated to errors. This will also allow customized configuration per analyzer package, as normally each analyzer package chooses a single prefix for all its analyzers.

2. Add another radio button, say "All compiler warnings", to the "Treat warnings as errors" section in the Build project property page, which passes down /warnaserror:CS* or /warnaserror:BC* to the compiler based on the project language:
   

[SDavydkin]

1. Why just don't reuse already existing CodeAnalysisTreatWarningsAsErrors property from "MSBuild\Microsoft\VisualStudio\v15.0\CodeAnalysis\Microsoft.CodeAnalysis.Targets" file? AFAIR FxCopCmd.exe-related task uses it. It is not accessible via UI, but this approach is compatible with already existing behaviors.

2. VS UI exposes only part of 'treat-warnings-as-error' related settings. There is yet another one - WarningsNotAsErrors, which allows excluding some warnings from the main Treat Warning As Error rule.
   It would be great to have this option in UI too.

3. Also, it would be great to have similar (opt-out) option for the code analysis too.

[SDavydkin]

In general, The 'good enough' solution is to add 'All compiler warnings' radio-button. In combination with WarningsNotAsErrors (even without UI support of it) it covers most of the needs. And it looks like the less invasive change.

[jaredpar]

Enhance /warnaserror switch to take arguments which are diagnostic ID prefixes, such as say /warnaserror:CS* or /warnaserror:BC*

This is essentially adding glob matching to our warning specification. Probably both in the compiler, editorconfig and the like. Is that really what we want here? If the ask is just for the compiler supported warnings then maybe just use the word "compiler" or something that is likewise definitive?

[jaredpar]

cc @agocke

[mavasani]

If the ask is just for the compiler supported warnings then maybe just use the word "compiler" or something that is likewise definitive?

Yes, that is the ask, but having a prefix based support allows users to even configure specific analyzer package diagnostics. For example, if someone had both StyleCop analyzers and FxCop analyzers installed, it is likely they want to keep style based diagnostics as a suggestion and code quality based FxCop diagnostics as errors, and a command line such as /warnsaserror:CS* /warnaserror:CA* can provide that support (without having to configure every single diagnostic ID and updating it with new rules being added to each package). However, if that is more difficult to implement, I think the customers would be happy even with just the "compiler" approach you mentioned.

[mavasani]

Why just don't reuse already existing CodeAnalysisTreatWarningsAsErrors property

@SDavydkin Can you expand on your proposal? Note that the default behavior for TreatWarningsAsErrors by itself (without any arguments) should still stay the same, i.e. it should still escalate all compiler and analyzer warnings to errors.

Also, it would be great to have similar (opt-out) option for the code analysis too.

That seems like a separate request to provide a mechanism/switch to skip analyzer execution during compilation. You can workaround this by using the approach we chose in Roslyn to implement DisableAnalyzers for a specific build.

[SDavydkin]

@mavasani In general, current behavior with TreatWarningsAsErrors applied to analyzers was a breaking change since originally (VS 2015 and FxCopCmd) it was applied only to compiler warnings. Analyzers warnings were controlled by CodeAnalysisTreatWarningsAsErrors setting. But it is history already.

Regarding a possible approach. If CodeAnalysisTreatWarningsAsErrors property value is not specified explicitly, then the value of TreatWarningsAsErrors property should be used as a default. Otherwise, analyzers warnings should be handled based on explicitly specified value.

That seems like a separate request to provide a mechanism/switch to skip analyzer execution during compilation.

I wrote about a different thing. Now user can explicitly specify that some warnings should NOT be treated as errors when TreatWarningsAsErrors = true. WarningsNotAsErrors property allows that. So, it would be nice to support similar thing for the analyzers warnings if they will be handled differently.

[mavasani]

In general, current behavior with TreatWarningsAsErrors applied to analyzers was a breaking change since originally (VS 2015 and FxCopCmd) it was applied only to compiler warnings.

FxCop was a completely different tool with IL based analysis, hence needed distinct properties to control/configure analysis. The primary goal behind integrating Roslyn analyzers into compilation was to remove the distinction between static analysis and compilation, and it was one of the core design goals to avoid compiler distinguish between diagnostics coming from a particular source (analyzer or compiler).

If CodeAnalysisTreatWarningsAsErrors property value is not specified explicitly, then the value of TreatWarningsAsErrors property should be used as a default. Otherwise, analyzers warnings should be handled based on explicitly specified value.

That is a viable approach, which possibly means adding a new compiler switch. However, that also makes the behavior of TreatWarningsAsErrors contextual based on whether or not CodeAnalysisTreatWarningsAsErrors is configured anywhere in the command line. I will let the compiler team decide on the preferred approach here.

WarningsNotAsErrors property allows that

I believe WarningsNotAsErrors transforms into /warnaserror-:<list>, so I presume either approach mention here will automatically handle this case. You are correct that we should update the IDE for this setting as well, not just TreatWarningsAsErrors and we should certainly add compiler unit tests to confirm the behavior of /warnaserror- after this change.

[SDavydkin]

FxCop was a completely different tool with IL based analysis, hence needed distinct properties to control/configure analysis.

I clearly understand the difference between FxCopCmd and Roslyn-based analyzers. But what you've described is the only difference in implementation. Both tools have an identical goal - analyze 'code' and provide recommendations. Actually, Roslyn-based analyzers can be treated as a replacement of the FxCopCmd. So, I'm pretty sure that that missing support of an analog of CodeAnalysisTreatWarningsAsErrors in Roslyn-based compilation stack was just a hole in a design. It made migration from FxCopCmd/StyleCop.exe to the Roslyn-based analyzer extremely hard (or even impossible as in our case).

That is a viable approach, which possibly means adding a new compiler switch.

I don't want to push my original idea. As I wrote in my second post, I think that "All compiler warnings" approach is good enough. At least it should work for us. And, I hope, it will not require an additional 2 years to be implemented :(

I believe WarningsNotAsErrors transforms into /warnaserror-:

Only if it occurs somewhere inside csc MSBuild task. Currently, csc task has a dedicated property - WarningsNotAsErrors="$(WarningsNotAsErrors)"

Update: Yes, you are right. The exclusion list was converted into /warnaserror-:612,618

[mavasani]

Ping @jcouv @jaredpar Any updates on the design here?

[jaredpar]

I've given my feedback here. I'm not really in favor of adding globbing support to the compiler layer. The warning / diagnostic matching is already extremely complex. This would be adding yet another layer to that complexity (in both the compiler, editor config, project system, etc ...). The support here doesn't seem cheap and I'd prioritize a lot of other features on top of it.

[mavasani]

I feel globbing design is lot more natural, has lot more benefits for end users and should also be trivial to implement. I think the only change here needs to go into the core command line parsers at the place we parse and AddWarnings based on the arguments to warnaserror and nowarn, and all other pieces will fall into place (editor config, project system, etc.). I am going to do a quick prototype as per https://github.com/dotnet/roslyn/issues/16535#issuecomment-427515639 and either create a PR for personal review OR update this issue with any unseen blockers.

[agocke]

I agree with Jared. I don't like a complex sub-language in our command line compiler. What really kills this proposal for me, though, is that it will not work with EditorConfig support, assuming we eventually ship it.

Overall, I'm not convinced the compiler should do anything here. We have the capability to use rulesets to set diagnostics to Info instead of Warning, which avoids this problem. Once we ship EditorConfig we'll have that as well. At that point, I think we should hand this off to the IDE to let it autogenerate the rules as necessary.

[mavasani]

Tagging @jinujoseph

@jaredpar

The support here doesn't seem cheap and I'd prioritize a lot of other features on top of it.

Note that there have been multiple requests already for this feature, and this has been classified as one of the _primary_ blockers by some customers who use /warnaserror to:

1. Migrate from legacy FxCop to FxCop analyzers and
2. Upgrade to future versions of analyzer packages as more rules get implemented which keep breaking their build on package upgrade.

@agocke

We have the capability to use rulesets to set diagnostics to Info instead of Warning, which avoids this problem.

I feel this is not a reasonable solution to this problem. We cannot expect customers to keep updating their ruleset or .editorconfig files every time they upgrade any of their analyzer packages (which can potentially bring new rules), especially as there is no way to even know what new diagnostics came from an analyzer package unless a specific one fires.

I think we should hand this off to the IDE to let it autogenerate the rules as necessary.

I am not sure I understand what you mean here, as the primary request here is for batch compilation, IDE is not involved at all.

What really kills this proposal for me, though, is that it will not work with EditorConfig support, assuming we eventually ship it.

Can you please provide concrete details here? Requested support is not from any specific configuration file (ruleset, .editorconfig, etc.), but from existing command line compiler switch, which already overrides configurations from ruleset files and has to keep doing so even for any .editorconfig based configuration.

 // Process ruleset files first so that diagnostic severity settings specified on the command line via
 // /nowarn and /warnaserror can override diagnostic severity settings specified in the ruleset file.

[jaredpar]

@mavasani

Note that there have been multiple requests already for this feature, and this has been classified as one of the primary blockers by some customers who use /warnaserror to

This would be completely addressed by the earlier suggestion of using "compiler" as an option. Globbing is an unnecessary complexity add on here.

Upgrade to future versions of analyzer packages as more rules get implemented which keep breaking their build on package upgrade.

This is the choice the customer makes when they use /warnaserror. If they're upset about every warning being treated as an error they have several options including enabling only the specific warning they want to be an error.

[mavasani]

This would be completely addressed by the earlier suggestion of using "compiler" as an option. Globbing is an unnecessary complexity add on here.

I am fine with this approach, this meets the primary requests from customer. What is the process to push this through compiler design review before it can enter implementation phase?

This is the choice the customer makes when they use /warnaserror

I feel this is a choice that we forced on the customers when we designed /warnaserror to imply both compiler and analyzer diagnostics get affected it. As noted in this comment, in pre-analyzer world, customers had two separate MSBuild properties/flags to control warnaserror for compiler and FxCop diagnostics and is considered as a feature take back for customers migrating from legacy FxCop to FxCop analyzers:

_In general, current behavior with TreatWarningsAsErrors applied to analyzers was a breaking change since originally (VS 2015 and FxCopCmd) it was applied only to compiler warnings. Analyzers warnings were controlled by CodeAnalysisTreatWarningsAsErrors setting._

[agocke]

I think there's some design work needed. At the moment adding compiler as an option to /warnaserror would make compiler a reserved diagnostic identifier. I'm not opposed, but we should be clear about the consequences. AFAIK, this would be the first ever reserved identifier for diagnostics.

We could alternatively add a new flag/option, but I want to know how it would interact with the other options.

[sharwell]

I lean towards leaving the existing behavior as the primary means to support this feature. Diagnostics reported by analyzers (including but not limited to the FXCop Analyzers) can be added to WarningsNotAsErrors if users don't want those warnings to be treated as errors during a build.

[mavasani]

Diagnostics reported by analyzers (including but not limited to the FXCop Analyzers) can be added to WarningsNotAsErrors if users don't want those warnings to be treated as errors during a build.

The request is to provide a single switch to control the behavior without explicitly listing specific diagnostic IDs in either bucket (WarningAsErrors or WarningNotAsErrors) OR in a ruleset/.editorconfig. It should also work with analyzer package upgrades, which can bring in bug fixes to existing rules + new CA rules, which were potentially not even part of legacy FxCop and hence can't be hardcoded as WarningNotAsErrors.

[sharwell]

I discussed this with @mavasani offline and we seemed to reach the following conclusion:

Ideally, it will be easy to update the compiler to have a command line flag addressing this situation. I have no preference about the form or manner of implementation, so I leave that to @mavasani and the rest of the compiler team. Should problems with that approach be encountered, a viable workaround for the specific case of FXCop Analyzers is to generate a .targets file containing content like the following, and include the file in the FXCop Analyzer packages:

<PropertyGroup Condition="'$(CodeAnalysisTreatWarningsAsErrors)' == 'false'">
  <WarningsNotAsErrors>$(WarningsNotAsErrors),CAxxxx,...</WarningsNotAsErrors>
</PropertyGroup>

[SDavydkin]

Dear team,
What range can be used to explicitly enable warnings as error mode for all or most of compiler warnings without setting TreatWarningsAsError switch?
It can unblock the migration to Roslyn-based analizers.

[mavasani]

@SDavydkin You can find all the C# compiler error code ID ranges here. Currently there is no way to specify a range argument to /warnaserror switch though.

[sharwell]

@SDavydkin If I was maintaining an external project, I would actually go the other way around:

1. Disable Treat Warnings as Errors so the build will run to completion
2. Build the solution and write the compiler output to a file
3. Define a <WarningsNotAsErrors> property like I mentioned in my previous comment, and add all the warnings from the build except the ones starting with CS (and/or BC if you are using Visual Basic) to the list
4. Enable Treat Warnings as Errors

If a new code analysis diagnostic appeared for the first time in a future build, the team can decide to either fix the warning immediately or add a new item to the list of exclusions from step (3). For improved long-term code quality, I find most people prefer a "keep clean code clean", which means updates to the list of exclusions should be exceedingly rare and the approach as a whole doesn't take long to configure.

[SDavydkin]

Thanks for the quick reply.

@mavasani

a range argument to /warnaserror switch though

Sad. It means my approach is not acceptable :(

@sharwell
It is least possible that this approach will work for us. We have a few thousands of projects per product.
Even my team solution contains more than 170 projects. It is near to impossible to manage warnings/errors suggested way.

[sharwell]

@SDavydkin You can define your property in a separate .targets file which is included in all of your projects, so you only have to manage the definition in one location (or at most a small number of locations depending on your source control configuration). If your projects are all within the same top-level folder, you can place a Directory.Build.targets file in the top-level folder and it will be automatically included in all the projects underneath that folder.

[SDavydkin]

@sharwell
Good point! Thank you for the idea. I'll try to play with this approach.

[SDavydkin]

@sharwell

To be clear... Does WarningsNotAsErrors support wildcards? Or should I specify every CA/SA warning explicitly?

[mavasani]

Does WarningsNotAsErrors support wildcards?

I don't believe any of the command line compiler switches that deal with warning IDs support wildcards.

[SDavydkin]

@mavasani

Pity but thanks

[mavasani]

@SDavydkin @wpanja Can you please let us know if the workarounds suggested in this issue are acceptable to you to unblock you from migrating to FxCop analyzers? We are trying to analyze the priority of this request, given that the proposed solution(s) here involve adding/updating command line compiler switches, which have a pretty high compat bar.

Tagging @jinujoseph @jaredpar

[mavasani]

I have filed https://github.com/dotnet/roslyn-analyzers/issues/1887 to implement the workaround suggested by @sharwell in https://github.com/dotnet/roslyn/issues/16535#issuecomment-433947630. Once we have an analyzer package with that fix, I will close this issue as Resolution-External since it unblocks the FxCop migration scenario for customers with /warnaserror + CodeAnalysisTreatWarningsAsErrors. If anyone believes this is a useful feature from a non-FxCop migration context, please feel free to file a separate bug to track that feature request.

[SDavydkin]

@mavasani Sorry for the delay, I didn't notice the notification message.
Technically workaround works. I've used something like following

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <TreatWarningsAsErrorsWorkaround Condition="'$(CodeAnalysisTreatWarningsAsErrors)' != 'true'">true</TreatWarningsAsErrorsWorkaround>
  </PropertyGroup>

  <Choose>
    <When Condition=" '$(WarningsNotAsErrors)' != ''">
      <PropertyGroup>
        <WarningsNotAsErrors>$(WarningsNotAsErrors),612,618</WarningsNotAsErrors>
      </PropertyGroup>
    </When>
    <Otherwise>
      <PropertyGroup>
        <WarningsNotAsErrors>612,618</WarningsNotAsErrors>
      </PropertyGroup>
    </Otherwise>
  </Choose>

  <ItemGroup Condition="'$(TreatWarningsAsErrorsWorkaround)' != ''">
    <!--Style cop rules to exclude -->
    <StyleCopRulesToExclude Include="SA0001" />
    <StyleCopRulesToExclude Include="SA0002" />
    <StyleCopRulesToExclude Include="SA1025" />
    <StyleCopRulesToExclude Include="SA1101" />
    <StyleCopRulesToExclude Include="SA1107" />
    <StyleCopRulesToExclude Include="SA1135" />
    <StyleCopRulesToExclude Include="SA1136" />
    <StyleCopRulesToExclude Include="SA1137" />
    <StyleCopRulesToExclude Include="SA1139" />
    <StyleCopRulesToExclude Include="SA1208" />
    <StyleCopRulesToExclude Include="SA1209" />
    <StyleCopRulesToExclude Include="SA1210" />
    <StyleCopRulesToExclude Include="SA1211" />
    <StyleCopRulesToExclude Include="SA1213" />
    <StyleCopRulesToExclude Include="SA1314" />
    <StyleCopRulesToExclude Include="SA1404" />
    <StyleCopRulesToExclude Include="SA1413" />
    <StyleCopRulesToExclude Include="SA1515" />
    <StyleCopRulesToExclude Include="SA1520" />

    <!-- FxCop rules to exclude -->
  </ItemGroup>

  <PropertyGroup Condition="'$(TreatWarningsAsErrorsWorkaround)' != ''">
    <WarningsNotAsErrors>$(WarningsNotAsErrors),@(StyleCopRulesToExclude, ',')</WarningsNotAsErrors>
  </PropertyGroup>
</Project>

Unfortunately, the workaround is least useful in production.

1. Usage of Directory.Build.targets file conflicts with experimental/future releases builds which creates such files dynamically.
2. Different teams and different projects have different baselines and a different set of rules with different severities. As a result, such an exclusion file will include all (or at least most of) StyleCop and FxCop rules.
3. So, I've transformed target file into property file and updated team's projects to import it.
4. But even in such case, it looks ugly inefficient. Each call to csc sends a quite huge /wanaserror-: parameter.

[mavasani]

Thanks @SDavydkin. Note that I merged in a workaround into FxCop analyzers NuGet package, such that if you explicitly set CodeAnalysisTreatWarningsAsErrors to false before the FxCop analyzer props imports, then we will automatically add a /wanaserror-: for each rule ID implemented in the package. You can try this out with this myget package, which will soon be released as a pre-release/release package on nuget.org as well.

Regarding all the drawbacks mentioned by you under the workaround is least useful in production., I will let @jaredpar @jinujoseph make a call on it.
I agree with you on Each call to csc sends a quite huge /wanaserror-: parameter, as with the workaround added to FxCop analyzers package, each csc command line is effectively doubled with the extremely large /wanaserror- list.

[SDavydkin]

@mavasani Thanks for the update!
In general, now I'm trying to migrate projects to StyleCop.Analyzers, since version which we use does not allow us to use C# 7.x. So, FxCop goes to scope only after that.