Completions should not show `Obsolete` items with `error` set to `true`

[DoctorKrolic]

Version Used: Latest main

Steps to Reproduce:

var c = new MyClass();
c.$$

class MyClass
{
    [Obsolete("Do not use this", error: true)]
    public void M()
    {

    }
}

Invoke completions at $$

Expected Behavior: M is not shown, because using it leads to compiler error

Actual Behavior: M is in the list and is even promoted by IntelliCode:

Additional context: When fixing this issue need to verify that such items are shown in nameof context, since code like this is perfectly valid:

var c = new MyClass();
var name = nameof(c.M); // usage of `M` here

class MyClass
{
    [Obsolete("Do not use this", error: true)]
    public void M()
    {

    }
}

[Youssef1313]

If this change is accepted, care should be taken to not regress other scenarios. For example:

using System;

var c = new MyClass();
c.M(); // perfectly fine here.

class MyClass
{
    [Obsolete(null, error: true)]
    public void M()
    {

    }
}

Yes, it is a bug that is kept for backward compatibility that when the message is null we always produce a warning not an error.

Are there any other cases? I don't know.

[CyrusNajmabadi]

I'm generally opposed to this. I think it's fine to offer. If someone doesn't want the item in completion, they can editorbrowsable it.

I would also be fine with updating the UI to show the item in a special way (like a strike through).

[DoctorKrolic]

I was thinking for a strikethrou for non-error obsolete items. But since completing these ones leads to compiler error, they become just unnecessary noise in the list

[CyrusNajmabadi]

You can use obsolete things from other obsolete things. We've definitely done this. Hiding means that becomes more painful.

they become just unnecessary noise in the list

I disagree. If someone doesn't want it in the list, that have that capability already with editorbrowsable. We should not conflate things here.

[DoctorKrolic]

I am not getting it. Of course I agree, that the list should show non-error cases, but commiting an error one means you will 100% have to clear it a bit later because of an error. How having it in the list can be helpful then?

[CyrusNajmabadi]

This is legal:

It's a core part of the design of obsolete that it can be used/suppressed. It's meant to let you know about issues directly calling it. But you can always get around it intentionally so you can still continue using it in ways you might need to.

We do this ourselves as we obsolete things and get most things off of it. But occasionally code needs to be kept around that still uses it.