diff --git a/accepted/better-obsoletion/better-obsoletion.md b/accepted/better-obsoletion/better-obsoletion.md new file mode 100644 index 000000000..f7f68853d --- /dev/null +++ b/accepted/better-obsoletion/better-obsoletion.md @@ -0,0 +1,278 @@ +# Better Obsoletion + +**PM** [Immo Landwerth](https://github.com/terrajobst) + +No framework, no matter how carefully crafted, is immune to making mistakes. For +a framework to last the test of time it's vital that it can be evolved to enable +developers to build the kind of applications that matter to their business, +which is an ever changing landscape and often puts additional constraints on the +developer's code that didn't exist before. + +The approach to productivity in .NET is called "the pit of success", which is +the idea that developers will more less stumble their way via code completion +towards the general vicinity of a correct solution. However, as the platform +grows and ages, the amount of mistakes in the platform also grows. And more +often than not the API that looks the most promising might be the wrong one to +use. So it becomes important that we can point developers the right way. + +Since .NET Framework 1.0, we were able to mark APIs as obsoleted via a built-in +attribute. Any usage of such an API results in the compiler emitting a warning +telling the developer that the API is obsoleted, which can include a small piece +of text pointing to an alternative. + +This obsoletion approach has a few benefits over the alternatives: + +* **API Removal**. Removing APIs is a very disrupting event. Some argue that + SemVer solves this problem but it doesn't work well for platforms as it ties + unrelated decisions together (e.g. in order to get access to new APIs, an + unrelated area in the code needs to be rewritten to avoid using the removed + APIs). Other technologies, such as C++, can use the preprocessor to remove + features unless the customer explicitly opts-in to legacy behavior. This + doesn't work well for .NET as this ecosystem is built around the reuse of + binaries, not source code. + +* **Documentation**. Let's face it, developers only read documentation when + something doesn't work. Only few developers research guidance and + best-practice proactively. But even if they did: guidance changes over time + and some technologies that looked like good ideas turn out to be unviable. We + need a mechanism that informs developers after the fact. + +Obsoletion via the `[Obsolete]` attribute works pretty well when: only a few +APIs are affected, it's easy to remove their usage, and guidance can be provided +as a short one-line string. + +In practice these requirements are often not satisfiable: + +* **Technologies have large API surfaces**. Obsoleting an entire technology + means that we'll have to annotate all entry points to that technology, which + often means obsoleting a *set of APIs*, such as types and static members. + +* **Removing obsoleted APIs isn't always practical**. It would be nice if we + could always remove dependencies on obsoleted technologies but this isn't + always possible if they are used heavily in existing code bases. In those + cases, you typically want a way to ignore obsoletion warnings regarding + specific technologies but not others so that you don't accidentally start + depending on obsoleted technologies when you're evolving your code base. + +* **Guidance is complex**. Obsoleted APIs rarely have a drop-in replacement. + After all, we don't obsolete based on their naming or shape but usually due to + fundamental design issues. That means the replacements are typically a nuanced + choice and require at least a page of explanation, rather than a one-liner. + +This document proposes changes to how APIs can be obsoleted in a more structured +way. + +## Scenarios and User Experience + +### Targeted Suppression + +Abigail is a developer and maintains the business logic layer component for +Fabrikam Billing Services. It was written in 2004 and makes heavy use of +non-generic collections by inheriting from them. Refactoring the code base to +use generic collections would be possible but a ton of work of dubious value as +the code is well-tested. Last year, the code was ported to .NET Core 2.1. + +The component is designed for a traditional two-tier environment and includes +data access that handles retrieving and storing business objects from and to SQL +Server. Abigail is now tasked to refactor that code to use REST-based services +instead so that the application can eventually be converted to a cloud-based +offering. + +Abigail would like to use the brand new JSON APIs so she updates the project +from .NET Core 2.1 to .NET Core 3.1. However, after upgrading the notices that +we obsoleted non-generic collections and she gets a bunch of warnings against +her code: + +| Code | Description | Project | +|--------|--------------------------|--------------------------------| +| DE0006 | 'ArrayList' is obsolete | Fabrikam.Billing.BusinessLogic | +| DE0006 | 'Dictionary' is obsolete | Fabrikam.Billing.BusinessLogic | + +Due to the heavy use, there are dozens of warnings like this throughout her +code, so she doesn't want to use `#pragma warning disable` for each affected +call site. Instead, she decides to suppress all obsoletion warnings for +non-generic collections by adding the ID `DE0006` to the +`Fabrikam.Billing.BusinessLogic` project's `NoWarn` list. + +Now the build produces zero warnings and she starts to add the networking +component. After spending a few minutes online she finds a code sample from +Stack Overflow that looks promising. After copy & pasting that into the code +editor, she immediately gets another obsoletion warning: + +| Code | Description | Project | +|--------|-------------------------------------------------|--------------------------------| +| DE0003 | 'HttpWebRequest' is obsolete. Use 'HttpClient'. | Fabrikam.Billing.BusinessLogic | + +Glad to have learned that, she continues to build her networking layer with the +recommendation of `HttpClient`. + +## More Details + +Bill is an architect and maintains the authentication and authorization related +components in Fabrikam Billing. Based on guidance from 2005, the code uses +`SecureString`. + +Bill is now tasked with refactoring this code to support Azure Active Directory +based authentication. Since most of the code base is now being upgraded to .NET +Core 3.1, he starts by retargeting the project. After doing so, he gets a bunch +of obsoletion warnings: + +| Code | Description | Project | +|--------|-----------------------------|-----------------------| +| DE0001 | 'SecureString' is obsolete | Fabrikam.Billing.Auth | + +Trying to understand why, he clicks the warning ID `DE0001` in the error list +which takes him to a documentation page explaining the issues with +`SecureString`. There he learns that this type doesn't work well for +cross-platform scenarios due to the lack of automatic encryption facilities and +the recommendation is to stop using it altogether. Bill knows that moving to the +cloud also means they are considering moving to Linux, so he takes note that +they also need to plan for removing their dependency on `SecureString`. + +## Requirements + +### Goals + +* **Global suppressions are specific**. Developers must be able to suppress + obsoletion warnings for all obsoletion warning that relate to the same + technology, rather than having to turn all obsoletion warnings off or having + to resort to suppress every single call site individually. + +* **Documentation pages are easily accessible**. Developers must be able to go + from the warning/error in the error list straight to the documentation that + provides the details. + +### Non-Goals + +* **Detecting specific usage patterns**. Sometimes, an API isn't obsoleted, just + a particular combination of arguments or specific values. If we need to + identify these, we'll author a custom analyzer. + +* **Being able to obsolete APIs in already shipped versions of .NET**. The + attribute must be applied to the API surface that the compiler sees and thus + must be present in the reference assemblies. We can only obsolete in new + versions of the platform. In other words, this doesn't support a side car file + that can be applied to an existing version of .NET. + +## Design + +The design proposal piggy backs on the existing `ObsoleteAttribute` that is +already supported by all .NET compiler. The idea is to expand this a little to +support the desired scenarios. Alternatives are listed afterwards. + +### API Changes + +We'd add the following properties to `ObsoleteAttribute`: + +```C# +namespace System +{ + public sealed 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; } + public string Url => UrlFormat == null ? null : string.Format(UrlFormat, DiagnosticId); + } +} +``` + +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. + +### Compiler Changes + +The compilers would be enlightened about these new properties and use them as +follows: + +* If `DiagnosticId` is `null`, use the existing diagnostic ID (e.g. `CS618` in + C#). Otherwise, use the specified warning ID. +* If `Url` is not `null`, use it as the diagnostic link when rendering them in + the IDE. + +### Samples + +Here is how the attribute would be applied: + +```C# +namespace System.Collections +{ + [Obsolete(DiagnosticId = "DE0006", UrlFormat="http://aka.ms/obsolete/{0}")] + public class ArrayList + { + // ... + } + + [Obsolete(DiagnosticId = "DE0006", UrlFormat="http://aka.ms/obsolete/{0}")] + public class Hashtable + { + // ... + } +} + +namespace System.Security +{ + [Obsolete(DiagnosticId = "DE0001", UrlFormat="http://aka.ms/obsolete/{0}")] + public sealed class SecureString + { + // ... + } +} +``` + +### Possible Alternatives + +#### String Encoding + +Instead of introducing new APIs, we could invent a textual format that we put +in the attribute's message property, such as: + +```C# +namespace System.Collections +{ + [Obsolete("::DE0006|http://aka.ms/obsolete/{0}")] + public class ArrayList + { + // ... + } +} +``` + +The benefit is that it doesn't require new API and that that the call sites +are slightly more compact. + +The downside is more cryptic use and weak contract between the compiler and the +API surface, which might also result in accidental recognition. + +#### Separate Attribute + +We could also introduce a new attribute and move detection to an analyzer that +is referenced by default. The shape could look as follows: + +```C# +namespace System +{ + public sealed class DeprecatedAttribute : Attribute + { + public DeprecatedAttribute(string diagnosticId); + public DeprecatedAttribute(string diagnosticId, string urlFormat); + + public string DiagnosticId { get; set; } + public string UrlFormat { get; set; } + public string Url => UrlFormat == null ? null : string.Format(UrlFormat, DiagnosticId); + } +} +``` + +The irony is that this would effectively mean that we're obsoleting the +`ObsoleteAttribute` which now begs the question whether we should mark it +`[Obsolete]` or `[Deprecated]`. \ No newline at end of file