Consider reserving diagnostic prefixes

[terrajobst]

As part of the specification for better obsoletion, @jaredpar and @agocke underlined my concern around conflicting diagnostic IDs which I have discussed with @jinujoseph as well. The analyzer team is already maintaining a table of used ranges that 1st parties should use to avoid conflicts.

However, we don't really have any guidance how third parties should choose their prefix to avoid conflicts. It seems to me that a prefix should be owned by a single party that can then control the IDs.

A few things:

1. Should Roslyn enforce that certain prefixes are only used by 1st parties? This would certainly include CS and BC, but should probably also include CA and IDE.

2. Should we give guidance to use longer prefixes? My understanding is that the prefix can be of any length, not just 2-3 characters. One option to avoid conflicts would be to use a qualified name, such as the NuGet package name.

3. Should we give guidance to use longer IDs? My understanding is that there is no requirement to use numbers. We could just tell people to spell out string like CA_Do_Not_Use_Async_Suffixes. In this cases, prefixes are almost irrelevant as two parties are fairly unlikely to use define the same rule (and if they do, it probably doesn't matter because suppressing both simultaneously is probably desired anyways).

[sharwell]

Should Roslyn enforce that certain prefixes are only used by 1st parties?

I'm not sure this will matter in the end. We should avoid shipping conflicting numbers from within our own products, but if another team wants things to look similar for some reason I don't see a particular problem allowing them to do so.

Historically, a non-trivial number of cases where the analyzer driver team decided to "block people who might not be as experienced from doing bad things" ended up backfiring either in a way that prevented a third-party analyzer from doing something desirable or by introducing performance overhead in the driver that becomes a tax for all analyzers even though few to none needed it. If we do want to make a best practice, the preference should be making it part of the Roslyn meta analyzers and not actually enforced within the driver.

Should we give guidance to use longer prefixes? ... Should we give guidance to use longer IDs?

Long names don't show up well in the error list. The VSTHRDxxx IDs are about the longest that can be used without disrupting primary interactions.

[terrajobst]

I'm not sure this will matter in the end. We should avoid shipping conflicting numbers from within our own products, but if another team wants things to look similar for some reason I don't see a particular problem allowing them to do so.

Based on my own personal experience, this isn't how things are usually working out for us:

1. Someone files a bug complaining about a specific analyzer ID. We're investigating how our analyzer could have reported a diagnostic only to find out that it wasn't our analyzer at all.

2. We're adding a new diagnostic but some popular extension already used that ID. Customers complain we broke them.

I think we need to make it clear that certain prefixes are owned by us. Enforcement (for example via an analyzer) seems to be the easiest way to document these in a customer facing way.

Keep in mind that for .NET 5 we're planning to ship a set of analyzers in-box that are on by default. We need a sustainable way to add to these over time otherwise it becomes one-shot kinda deal.

Historically, a non-trivial number of cases where the analyzer driver team decided to "block people who might not be as experienced from doing bad things"

I'm not suggesting blocking features. Reserving prefixes is similar to what we did on nuget.org or blocking reflection invoke on private framework APIs. This isn't to prevent people from using complex features, it's about preventing hacks by enforcing policies that are already in-place.

Long names don't show up well in the error list. The VSTHRDxxx IDs are about the longest that can be used without disrupting primary interactions.

That's fair but I wonder how much of this is a static property of the UX vs. a point-in-time that we could address.

[mavasani]

I presume dotnet/roslyn-analyzers#1727 has more history around this. #25748 has good discussion on why our only option on helping customers in this space is by writing a meta-analyzer. In-source suppressions is one of the reasons why rule authors are hesitant to change their rule IDs once shipped - the end users are rendered with stale suppressions that they need to manually fix.

[terrajobst]

I'd argue that #25748 proves why reserving is a must have feature for a platform provided prefix. It might be too late for CS/BC but I'd be in favor in doing that for whatever the prefix is we're going to use for the framework provided ones.

[mavasani]

why reserving is a must have feature for a platform provided prefix

@terrajobst By reserving, you mean adding a meta-analyzer, correct? We can also consider making this an analyzer error if we want higher user impact. Given that analyzer errors can be suppressed, such a rule will not completely block analyzer authors who are choosing a conflicting ID for compat reasons.

[jaredpar]

I'm not sure this will matter in the end. We should avoid shipping conflicting numbers from within our own products, but if another team wants things to look similar for some reason I don't see a particular problem allowing them to do so.

Another team re-using our own prefixes can do nothing but add confusion to the end user. There are expectations that come with diagnostics we produce including: documentation on the error being available, support in forums for how to deal with an issue and the ability to discuss the diagnostic rationally on this repository.

Reserving the diagnostic prefix means that customers can depend on a specific level of support for the diagnostics they come to expect from Microsoft products.

By reserving, you mean adding a meta-analyzer, correct

In my mind we just add this to the compiler / analyzer driver. Essentially reject any diagnostic whole sale if it has a prefix that is Microsoft reserved.

[terrajobst]

@mavasani

@terrajobst By reserving, you mean adding a meta-analyzer, correct?

I don't have a strong opinion on how the enforcement is done, so long it's not just a policy document (i.e. it's tooled).

@jaredpar

In my mind we just add this to the compiler / analyzer driver. Essentially reject any diagnostic whole sale if it has a prefix that is Microsoft reserved.

I would agree with that. We can't hold the stack hostage just because we made a few mistakes early on. However, I'm totally happy with that just being an analyzer that people can suppress (so long the analyzer is on by default, of course). It just means I will come done hard on anyone who wants us to make changes to accommodate their decision to suppress those diagnostics.

[mavasani]

In my mind we just add this to the compiler / analyzer driver. Essentially reject any diagnostic whole sale if it has a prefix that is Microsoft reserved.

@jcouv implemented this in #23776 for CS and BC compiler IDs, but that had to be reverted due to #25748, it caused bunch of grief for both analyzer authors and users who needed to use these IDs for compat reasons. I feel having this as an analyzer error which is enabled by default in Microsoft.CodeAnalysis.Analyzers that should be installed for all analyzer projects might be a good compromise.

[jaredpar]

@mavasani

Reading through that thread I don't understand the compat burden they're adherring to. There is mention of FxCop but that is owned by us now. If they're producing their own FxCop errors then that seems like a problem.

[mavasani]

@jaredpar FxCop exposed a public API and extension point to allow third parties to plug in their own rule assemblies with custom authored rules, and the customer seemed to have chosen the CS/BC prefix for their custom rules.

Tagging @hvanbakel if he can give more clarity on how they ended up using CS/BC diagnostic IDs for their custom rules.

[jaredpar]

This seems very fragile. Are we now bound from creating specific error messages in the compiler if any third party extender of FxCop has already added that particular error? I definitely wouldn't lean towards reverting a compiler error based on that criteria.

[mavasani]

This seems very fragile.

I agree - Ideally, FxCop should have just allowed a specific range or prefixes for customer authored rules to avoid this state. However, given we already have third party rules authored with FxCop using specific IDs, which have/should be ported to analyzers now, any hard block from compiler/analyzer driver would likely affect back compat. IMO, an analyzer error is still good enough compromise, and would likely lead all analyzer authors to avoid using the reserved IDs. The ones that suppress it and still use the reserved IDs should have very good back compat justification.

[terrajobst]

Giving this more thought I'm not sure I understand the backwards compat argument. Eventually, the compiler will add a warning/error with the same ID as the customer's FxCop rule. Now what? It seems to me, that their rule is a ticking time bomb anyway.

[agocke]

One note: I think the Prefix+number pattern is fundamentally broken and I would argue against it for the compiler if we could do it again.

The actual diagnostics in the compiler have names like "WRN_NullAsNonNullable" that hint to what the warning actually means, instead of just some opaque number. That's the pattern, with or without some namespacing prefix, that I would pick for any new projects.

[nguerrera]

(+100 to what @agocke said. FxCop used PascalCasedRuleNames before it became part of VS. The introduction of the short prefix + numeric codes to fit the error list was a massive experience regression in my opinion, and I wish we’d pushed back harder all those years ago.)