-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Unintended consequences of disallowing CS and BC analyzer ids? #25748
Comments
Tagging @jinujoseph for advice. |
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. |
@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 |
#23776 was a back compat breaking change. I would be in favor of reverting it, which would also fix this problem. |
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 Thoughts? |
I think we should fix this in 15.8 due to compat break. |
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. |
This PR reverts that change and a similar diagnostic will be implemented as an analyzer with dotnet/roslyn-analyzers#1727. Fixes dotnet#25748
Filed dotnet/roslyn-analyzers#1727 and opened #28182 to fix this issue. |
Thanks for reverting this, analyzers (and associated fixes) are working again in VS 15.8 👍 |
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).
The text was updated successfully, but these errors were encountered: