CodeAnalysisRuleSet property should be added to an unconditional PropertyGroup in the msbuild project file

[AArnott]

Roslyn will report compile warnings even though I have suppressed them in the IDE using the References -> Analyzers UI in Solution Explorer.

git clone https://github.com/AArnott/PCLCrypto.git
cd pclcrypto\src
git checkout fbd8c226de0
nuget restore
msbuild PCLCrypto.iOS-Unified\PCLCrypto.iOS-Unified.csproj

Expected
No SA1124 warnings. The PCLCrypto.iOS-Unified.ruleset file has this analyzer set to None.

Actual
Several SA1124 warnings:

C:\Users\andarno\git\PCLCrypto\src\PCLCrypto.Shared.Common\CryptoStream.cs(186,9): warning SA1124: Do not use regions [C:\Users\andarno\git\PCLCrypto\src\PCLCrypto.iOS-Unified\PCLCrypto.iOS-Unified.csproj]

[AArnott]

The project file conditionally specifies the ruleset file, and does not specify it for many of the platforms supported by that project. In fact, the default platform (iPhoneSimulator) does not specify the ruleset file. This explains why the analyzer warning shows up in compilation.

  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
    <CodeAnalysisRuleSet>PCLCrypto.iOS-Unified.ruleset</CodeAnalysisRuleSet>
  </PropertyGroup>

The References folder in Solution Explorer doesn't capture the nuances of configurations-specific rule set files. And the UI presented to me seems misleading as I set the analyzer to None and yet it still shows up in the build. I suggest the CodeAnalysisRuleSet property be defined in an unconditional PropertyGroup to match the UI.

[mavasani]

We should fix this.

[mavasani]

VSTF bug 1082466 also raised this issue and @tmeschter added the following comment prior to Won't fixing it:

We're limited by the design of the extensibility system, which considers CodeAnalysisRuleSet to be a per-configuration setting, not a project-wide setting. Since we only update this automatically in the case that you're replacing a built-in ruleset with a modified copy, it is very unlikely that this will cause issues for users.

Tom, we probably need to fix it now. Do you think it is going to be difficult to fix?

[tmeschter]

@AArnott My understanding is that the command line build is using a different configuration/platform than the IDE. Is that correct?

[tmeschter]

@mavasani It isn't clear that this is actually a bug, much less what the fix would be.

If the user has different rule set files for different configuration/platform combinations, then we need to honor that. That means that updating a rule's severity only updates the the rule set for the current configuration/platform (what we call the "active" rule set). Otherwise we make it difficult, or even impossible, to have different sets of rules active for different configurations.

That being said, I can think of two things we might be able to do to improve this:

1. When setting the CodeAnalysisRuleSet property in a project, if none of the project configuration/platform combinations already specify the property, then put it in an unconditional PropertyGroup. Otherwise only update the current configuration/platform. This will require changes in the project system (possible very extensive), and risks breaking other code that depends on getting or setting this property.
2. If the active rule set file has been added as a project item, update its icon in Solution Explorer to make it clear that it is the active rule set. This should be pretty simple, but I'm not convinced it will really help.

[tmeschter]

@AArnott @mavasani And I should say that Solution Explorer doesn't capture the nuances of configuration-specific rule set files because Solution Explorer doesn't know anything about configurations. It only knows about what is currently considered to be part of the project; information on other configurations generally doesn't make it from MSBuild to the language services.

[mavasani]

Tagging @srivatsn

Thanks Tom. From your description, this seems like more of a feature request, then a bug (at least it is not a trivial fix).

[AArnott]

@tmeschter said:

My understanding is that the command line build is using a different configuration/platform than the IDE. Is that correct?

In one of my solutions, the active solution config varies by the minute as I switch between iPhone, Android, and various Windows target platforms. The command line of course has just one default (Mixed Platforms, I believe). I always want all my configurations to be in sync for rules.

@tmeschter said:

If the user has different rule set files for different configuration/platform combinations, then we need to honor that. That means that updating a rule's severity only updates the the rule set for the current configuration/platform (what we call the "active" rule set). Otherwise we make it difficult, or even impossible, to have different sets of rules active for different configurations.

My intuition tells me most people don't have different rule sets for different configurations, as that would make it much more difficult to manage and reason about. Should we sacrifice the common setting case to such a degree for these folks? Do we know how many are on either side of it? It is a hard call though. In C++ we've had the same dilemmas.