dotnet / csharpstandard

Working space for ECMA-TC49-TG2, the C# standard committee.
Creative Commons Attribution 4.0 International
709 stars 84 forks source link

Obsolete member accessing another obsolete member #701

Open KalleOlaviNiemitalo opened 1 year ago

KalleOlaviNiemitalo commented 1 year ago

If an obsolete member accesses another obsolete member, Microsoft's C# compilers do not issue the warning required by C# 6.0 §21.5.4. Should this suppression be part of the C# standard? If not, does the standard allow it as an extension (which would then have to be defined in the conformance document)?

For example:

static class Demo
{
    [System.Obsolete]
    static void M() {}

    [System.Obsolete]
    static void N()
    {
        M(); // no warning about accessing obsolete M, because N is also obsolete
    }

    static void O()
    {
        N(); // warning issued
    }
}

At least the following compiler versions behave this way:

jskeet commented 1 year ago

Well spotted, thanks for this. I think we should probably approach 21.5.4 with fresh eyes and consider the most appropriate course of action. (It's relatively rare for the standard to mandate warnings - Obsolete is a bit different because it can cause an error, not just a warning.)

But interestingly, the compiler doesn't issue an error when the code is called from another obsolete member either. Not sure whether that's alarming or just consistent. We'll discuss it at the next meeting.

KalleOlaviNiemitalo commented 1 year ago

See also https://github.com/dotnet/roslyn/issues/23015.

KalleOlaviNiemitalo commented 1 year ago

Those compilers also issue a warning rather than an error, when a member decorated with [Obsolete(null, true)] is accessed. That was apparently a bug in older compilers and now a deliberate compatibility hack in Roslyn.

https://github.com/dotnet/roslyn/blob/15b43b33901c88f68ef43f8314b5a2457716780d/src/Compilers/CSharp/Portable/Symbols/ObsoleteAttributeHelpers.cs#L159-L165 https://github.com/dotnet/roslyn/blob/15b43b33901c88f68ef43f8314b5a2457716780d/src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs#L5745-L5746

jskeet commented 1 year ago

Right - that last part sounds like something we definitely wouldn't want in the standard, and should instead be in a "known incompatibilities in Roslyn" document (which I have an action item to start...)

jskeet commented 1 year ago

Aim for today's meeting: decide whether or not we want to do this, and assign it accordingly. We don't need to spend time guessing on wording.

jskeet commented 1 year ago

Right - we would like to spec this, as specific optional behavior. We note at the same time that "the error parameter" is a bit unclear unless you go looking in Annex C. (Even a cross-reference would help.) Low priority though - this has been broken forever.