bbowyersmyth commented 8 years ago

The Problem

A good thing about the .NET Core 1.0 surface area is that it made it clear what classes/methods were considered obsolete because for the most part, they just didn't exist. With the larger surface area planned for v2 a lot of this missing functionality is being brought back purely for compatibility reasons. How does a developer get informed that a given API is no longer recommended for use?

ObsoleteAttribute is the heavy handed version of this which says "You should stop using this immediately, it will be removed in a future version". There needs to be an "Info" level version of this for things that are considered to be obsolete but it's not appropriate to mark it is such.

[EditorBrowsable(EditorBrowsableState.Never)] assists when writing new code but doesn't help someone identify usages in existing code.

If I am using something that is no longer recommended I want to know about, and I would like my IDE to be able to tell me. Marking up libraries with a LegacyAttribute would be the first step to having that metadata.

Solution

    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Enum |
        AttributeTargets.Interface | AttributeTargets.Constructor | AttributeTargets.Method |
        AttributeTargets.Property  | AttributeTargets.Field | AttributeTargets.Event | 
        AttributeTargets.Delegate, Inherited = false)]
    public sealed class LegacyAttribute : Attribute
    {
        public LegacyAttribute()
        public LegacyAttribute(string message)

        public string Message { get; }       
    }

Semantic Meanings "This exists for backwards compatibility. Nothing to see here. Move along" "This will work for as long as you need it but we recommend you use something else"

Examples

TimeZone can't be marked obsolete https://github.com/dotnet/corefx/issues/10074 Analyzers need metadata https://github.com/DotNetAnalyzers/Proposals/issues/37 Legacy collections can't be marked obsolete https://github.com/dotnet/corefx/issues/370 (It's common for Java developers to use ArrayList on .net, how do we educate that's not recommended anymore?) CharSet.Unicode inappropriate to mark obsolete https://github.com/dotnet/corefx/issues/7804 BitVector32 https://github.com/dotnet/corefx/issues/373

Summary

Compatibility has been one of the full .NET frameworks great strengths. There needs to be a way to deal with the downside of that which is education of APIs that still exist only for that compatibility story.

juliusfriedman commented 8 years ago

    /// <summary>
    /// This code is obsolete.
    /// </summary>

khellang commented 8 years ago

I'd say this attribute should be valid on assembly level as well, no?

juliusfriedman commented 8 years ago

I'd say, "What meaningful information is this going to relay and to whom?"

I will certainly not be putting this attribute on any of my code, especially when I obsolete or deprecate it I will be removing it and not annotating it.

mburbea commented 8 years ago

It just smells like another flavor of obsolete. It also has all the same problem. If your concern is that obsoleted a bunch of classes will create a bunch of noise warnings in existing code bases, then legacy would do the exact same thing. There are many cases where the obsolete attribute has been on a class for multiple versions of .net and those have not been removed and will probably never be removed. (see things like Enum.ToString(IFormatProvider))

Obsolete let's you put in a message, and you can happily say on ArrayList use List<T> instead, but it's not done because it would create a huge amount of warnings in existing code bases. Why is obsolete, which is never removed worse or scarier than legacy?

bbowyersmyth commented 8 years ago

@khellang Good suggestion. The list was taken from the ObsoleteAttribute, not sure why it couldn't be applied at an assembly level as well.

@mburbea Think of it as not another type but another level. The ObsoleteAttribute is not being applied to a lot of things it could be because it results in a build break if you follow the recommended practice of having "Warnings as errors" on. Which can be a runtime error in ASP.NET. Few things are obsolete enough that you would consider breaking someone's build until they stopped using it. How do you educate about the rest?

The answer to this problem can't be that the user has to read the docs for any function they might call on any external API... and again on any upgrade.

My concern is not regarding message noise. That would be up the IDE implementation. I would probably suggest it be off by default in something like VS. But even if I had to install an analyser that would be fine too. As long as there is an automated approach to finding API status. If that produced 100's of info level messages, great. That is exactly what I want to know.

Because they have different support requirements than most, the .net framework's version of removing obsolete items is not porting them to new platforms. Framework authors following semantic versioning would remove them from the next major version. There is a whole group of "Not recommended" APIs that this wouldn't be appropriate for.

AlexGhiondea commented 7 years ago

@terrajobst what are your thoughts on this?

terrajobst commented 7 years ago

Couple of thoughts:

Yes, [Obsolete] is heavy handed.
Before discussing whether we need a new type, we should focus on the desired developer experience:
- Should the compiler provide diagnostics?
- Should an analyzer provide diagnostics?
- Should the developer be able to suppress specific areas?
- Should we have a way to point to a web site with more information?
- Should we support automatic replacements via fixers?
- Do we want to be able to mark APIs without updating the assembly?
- ...

I do think obssoletion doesn't work very well but just adding another attribute will do what @bbowyersmyth said: it's just kicking the can down the road. I'd rather we design a process for obsoletiton that helps developers moving forward while avoiding being in intrusive and/or noisy.

bbowyersmyth commented 7 years ago

@terrajobst One example I'm thinking of is say you have an application that has been around for a long time and has seen a few variations on what is considered secure password hashing. On login you may have to recreate the hash using "obsolete" tech but afterwards update their record using the current standard. So everyone slowly migrates.

In this case: Yes all that old stuff needs to continue be available (.NET Standard 2.0 :white_check_mark:). Yes the developer should be warned or informed about using it in new code. Yes the developer will need to be able to disable these messages in code that is itself legacy.

Not sure about fixers. In some cases it will be a recommendation to use another overload, but often it might be to use another api all together.

The opportunity for a help url may be useful. But for most library authors they would only have a GitHub repo to point to. Honestly I have found obsolete warning messages to be good enough to understand what is going on.

It's hard to say that someone will fall into the pit of success if they wander into System.Security.Cryptography. It's an evolving field and it would be great to have security specialists marking up those classes with the metadata required to deduce an api's status. I'm sure they would also love the opportunity to educate people through their IDE.

Another ObsoleteAttribute problem: Breaks XML Serializer https://github.com/dotnet/corefx/pull/13745

am11 commented 7 years ago

to deduce an api's status

👍

If we are generalizing ObsoleteAttribute and LegacyAttribute as "API status indicators / levels" as opposed to "means of obsoletion", IMO, we should also consider borrowing ExperimentalAttribute from F# :)

Related to http://stackoverflow.com/a/16484780; the sealed modifier on ObsoleteAttribute class is one of few barriers to extend in this area. Perhaps, it would make sense to instead:

introduce a new kind of attribute hierarchy with for example ApiStatusAttribute at the base providing extensibility in terms of controlling:
- editors' severity level for message logging: info, warning, error and fatal etc.
- other impressions / gestures conveyable to various editors via compiler
derive Obsolete/Legacy/Experimental attribute types from ApiStatusAttribute base
this will give the devs ability to even derive their custom status attributes and override the editor behavior offered by the base

Joe4evr commented 4 years ago

This is handled now that the Better Obsoletion properties are merged in, it's just a matter of waiting for the compilers to honor the new properties.

bbowyersmyth commented 4 years ago

ObsoleteAttribute now has additional features https://github.com/dotnet/runtime/issues/33089

dotnet / runtime

Proposal: LegacyAttribute #17865

The Problem

Solution

Examples

Summary