Roslyn: Honor new properties on ObsoleteAttribute

[terrajobst]

We'd like to make obsoletion more viable. This requires extending the ObsoleteAttribute:

```C#
namespace System
{
public partial class ObsoleteAttribute : Attribute
{
// Existing:
//
// public ObsoleteAttribute();
// public ObsoleteAttribute(string message);
// public ObsoleteAttribute(string message, bool error);
// public bool IsError { get; }
// public string Message { get; }

    // New:
    public string DiagnosticId { get; set; }
    public string UrlFormat { get; set; }
}

}
```

• DiagnosticId. Represents the ID the compiler will use when reporting a use of the API. This relies on diagnostic IDs being unique, just like the rest of the Roslyn analyzer facility.
• UrlFormat. The URL that should be used by an IDE for navigating to corresponding documentation. Instead of taking the URL directly, the API takes a format string. This allows having a generic URL that includes the diagnostic ID. This avoids having to repeat the ID twice and thus making a copy & paste mistake.

We'd like the compiler to honor these new properties as follows (more details in the proposal):

• If DiagnosticId is null, use the existing diagnostic ID (e.g. CS0618 in C#). Otherwise, use DiagnosticId.
• The compiler should honor suppressions for the specified diagnostic id.
• When the user suppresses the existing generic diagnostic ID for obsoletion (e.g. CS0618 in C#), the compiler should not suppress any obsoletions with an explicitly set diagnostic ID (unless that explicit ID happens to match the generic diagnostic, but that would be bad practice from the API author). Instead, the developer has to add a suppression for the specified diagnostic ID. As such, adding a diagnostic is the same as changing a diagnostic ID and thus a source breaking change.
• The compiler doesn't report diagnostics for use of obsoleted APIs when the use site itself is marked obsolete. On the one hand, it seems logical to amend this behavior to only skip the reporting if the diagnostic ID of the use site and the declaration site are the same. On the other hand, that's probably overkill because a developer who marked a type or method as obsoleted makes it clear that the code is only provided for backwards compatibility. Having to keep adding new suppressions for other obsoleted APIs seems unhelpful.
• If UrlFormat is not null, use it as the diagnostic link when rendering them in the IDE.
• The compiler should assume that UrlFormat and DiagnosticId are independent features, in other words both can be used, either, or neither.

@jaredpar @agocke

[terrajobst]

One item that was left as an open issue in the spec is this:

Should the compiler suppress all diagnostics from ObsoleteAttribute if the existing generic diagnostic ID is suppressed (i.e. CS0618)? If not, the developer has no way to turn this feature off entirely.

My proposal would be no. If an ObsoleteAttribute has an explicit diagnostic ID, it cannot be turned off by suppressing CS0618. Instead, the developer has to add a suppression for the specified diagnostic ID. As such, adding a diagnostic is the same as changing a diagnostic ID and thus a source breaking change.

[tmat]

Does this need to be a compiler feature? Seems like an analyzer would do (other than change to the compiler to not report the obsolete diagnostic when DiagnosticId is present). Also, should Obsolete reporting be removed from the compiler entirely and moved to an analyzer?

@mavasani

[terrajobst]

I discussed this with @agocke and @jaredpar. We can't make this an analyzer feature without removing this from the compiler (otherwise we end up reporting the obsoletions twice, once under the old diagnostic and once under the new one). But we don't have a vehicle for shipping built-in analyzers except for .NET 5 + SDK-style projects, so you'd effectively remove this feature everywhere else.

To be honest, I don't think moving this to an analyzer is worth it. But if we were to build the feature today, we'd most certainly make this an analyzer, rather than a compiler feature.

[sharwell]

@terrajobst The compiler has a suppression mechanism (see e.g. https://github.com/dotnet/machinelearning/pull/4803). It would be trivial to suppress the original diagnostic ID at the same time as reporting the new one. However, analyzers require diagnostic IDs be declared in advance so it would not be possible to arbitrarily specify diagnostic IDs.

[terrajobst]

I'm not sure what is gained by doing this. If we can't remove the code in the compiler that deals with handling [Obsolete] and we have to re-do this work for the analyzer (and we have to ship a suppressor for the built-in one), then we

1. Run the [Obsolete] analysis twice (once in the compiler, once in the analyzer)
2. Have the potential of introducing bugs by not handling the cases the same as the compiler does today

It seems way easier to simply extend the code where the compiler emits a diagnostic than redoing the analysis. In my opinion, creating an analyzer for this is trying way too hard to use a tool just because it's there...

[sharwell]

@terrajobst I'm not making a statement regarding which approach to use; I'm simply clarifying what the current tooling is capable of.

[sharwell]

I'm not sure how changing the diagnostic IDs would be expected to interact with #28549. Also, if a method marked obsolete with diagnostic ID A uses a member marked obsolete with diagnostic ID B, should the usage be reported or suppressed (and why)?

[terrajobst]

@sharwell

Also, if a method marked obsolete with diagnostic ID A uses a member marked obsolete with diagnostic ID B, should the usage be reported or suppressed (and why)?

That's an interesting question. I'm inclined to say that the usage should only be suppressed if the diagnostic IDs match. The reason being that the intention of this feature is to allow the developer to create sets of obsoleted APIs that can be suppressed individually, rather than making this a global on/off switch.

[RikkiGibson]

The link in the issue description is broken @terrajobst

I went ahead and updated it.

[terrajobst]

Thanks Rikki!

[terrajobst]

@sharwell

Also, if a method marked obsolete with diagnostic ID A uses a member marked obsolete with diagnostic ID B, should the usage be reported or suppressed (and why)?

That's an interesting question. I'm inclined to say that the usage should only be suppressed if the diagnostic IDs match. The reason being that the intention of this feature is to allow the developer to create sets of obsoleted APIs that can be suppressed individually, rather than making this a global on/off switch.

Actually, I changed my mind. I think we should leave the current behavior, i.e. not reporting any obsolete diagnostics when the use site is obsolete. I've updated the description above and the spec (https://github.com/dotnet/designs/commit/bf390d20108d7247e2ba60b0c801a947383637d1).

One item that was left as an open issue in the spec is this:

Should the compiler suppress all diagnostics from ObsoleteAttribute if the existing generic diagnostic ID is suppressed (i.e. CS0618)? If not, the developer has no way to turn this feature off entirely.

My proposal would be no. If an ObsoleteAttribute has an explicit diagnostic ID, it cannot be turned off by suppressing CS0618. Instead, the developer has to add a suppression for the specified diagnostic ID. As such, adding a diagnostic is the same as changing a diagnostic ID and thus a source breaking change.

Since there was no pushback on this, I've called this behavior out in the issue and my spec (https://github.com/dotnet/designs/commit/fe0d541f074405eda77241de660d9411ee587174).

[RikkiGibson]

A key bit of the spec, and how we are landing on it in terms of behavior:

The compiler should assume that UrlFormat and DiagnosticId are independent features, in other words both can be used, either, or neither.

The expected behaviors for "neither", "both", or "only DiagnosticId provided" are fairly straightforward. But I wanted to be clear about the case where UrlFormat is provided but DiagnosticId is not provided: we will use the DiagnosticId that the compiler chooses by default as the format argument to UrlFormat. This could be CS0618, CS0619, CS1062, or CS1063. The internal error codes don't consider leading zeroes, but the padding is important when we actually do the format substitution.

When I write it out it seems pretty obvious :smile: but still good to record the expectation.

/cc @terrajobst

[terrajobst]

When I write it out it seems pretty obvious 😄 but still good to record the expectation.

Good point. I've submitted https://github.com/dotnet/designs/pull/105 to clarify the expected behavior.

[terrajobst]

Awesome! Thanks @RikkiGibson!