Unintended consequences of disallowing CS and BC analyzer ids?

[hvanbakel]

VS version: 15.6.4, but I assume this applies to 15.6.*

The fixes applied for #23776, while logical, result in code changes IF you happen to have had analyzers that generated some of these violations.

In our case, we've had rules written for consumption in FxCop, then later these rules were ported to roslyn analyzers. These rules have used a rule ID starting with CS and have been in place for years leading to code with some suppressions as well as some newer pragmas to disable them.

With the latest VS2017 versions, I've been presented with the errors that I am no longer allowed to generate a violation like that because those are reserved for the compiler, which I totally understand. However, because of the way this is set up, there is no way for me to work around this because the analyzers now just throw an exception. This means that my violations are no longer showing up and I cannot suppress the rule disallowing this.

The implications of the above are however, that I need to change the rule ids on my analyzers. That's not really a problem, but it becomes a problem because it renders every single suppression useless. In our case that's thousands and probably tens of thousands of suppressions that have accumulated over the years.

What I would like to propose is to generate a separate violation that says you should not be using these but still let the error be reported. That way, I can opt out of that violation by disabling it from a ruleset. And yes, I that might add confusion that needs explaining but that's something that we needed to do anyway (internally).

[jcouv]

Tagging @jinujoseph for advice.
I don't have enough context on how strongly the guideline for analyzer IDs had been communicated (analyzer IDs must be unique) before my 15.6 PR tightened the enforcement.
If the guideline was pretty clear, then I would recommend that customers should fix their analyzer IDs and use search & replace to update their suppressions.
If the guideline was loose, then the suggestion above (produce a new diagnostic when a violation is found, rather than throwing an ArgumentException) makes sense.

[hvanbakel]

As some background, while roslyn analyzers might be relatively new, you were able to do this for years using FxCop. And changing rule ids would invalidate all suppressions.

[mavasani]

@jcouv The recommendation has always been to avoid producing CS or BC prefixed diagnostics for analyzers, but as @hvanbakel points out in #25748 (comment), there was no such restriction on old FXCop rule implementations, and legacy code bases that execute such rules are likely to have source suppressions for these IDs, which become useless if the ported analyzer doesn't choose the same ID. Given that legacy FXCop compat is one of our primary goals to make it easier for customers to move to analyzers, we cannot (and should not) prevent analyzer authors porting their custom legacy FXCop rules to new analyzers and carry forward the rule ID. I think we need to loosen our requirement to generate a separate diagnostic instead of AD0001 through an ArgumentException. I will take up this bug and make this change.

[sharwell]

#23776 was a back compat breaking change. I would be in favor of reverting it, which would also fix this problem.

[hvanbakel]

Given that 15.8 is in preview I'm assuming that this one is not going to make it? I just looked at how it was implemented but the fact that it is throwing an exception inside of 'IsSupportedDiagnostic' feels like a shortcut making this hard to fix.

I would suggest putting this either in the CompilationWithAnalyzers or the AnalysisScope class where a new diagnostic can be created if any of the analyzers report a diagnostic that is reserved.

Thoughts?

[mavasani]

I think we should fix this in 15.8 due to compat break.

[mavasani]

I am going to submit a PR that completely removes this new diagnostic from being reported. This diagnostic was intended to be a guidance for the analyzer authors, not the end users using the analyzer as it is not actionable for them. As such it would be much better to implement this as an analyzer that goes into https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeAnalysis.Analyzers/Microsoft.CodeAnalysis.Analyzers.md that fires on the analyzer project. I will file a separate issue in roslyn-analyzers repo to implement such a meta-analyzer.

[mavasani]

Filed dotnet/roslyn-analyzers#1727 and opened #28182 to fix this issue.

[hvanbakel]

Thanks for reverting this, analyzers (and associated fixes) are working again in VS 15.8 👍