Obsolete error should be reported for container class of a collection expression builder class

[Spolutrean]

Version Used: visual studio17.8.0 Preview 1.0, .net80

Steps to Reproduce:

[CollectionBuilder(typeof(Container.MyCollectionBuilder), "Create")]
public struct MyCollection<T> : IEnumerable<T>
{
    IEnumerator<T> IEnumerable<T>.GetEnumerator() => default;
    IEnumerator IEnumerable.GetEnumerator() => default;
}

[Obsolete("Message", error: true)]
public class Container
{
    public class MyCollectionBuilder
    {
        public static MyCollection<T> Create<T>(ReadOnlySpan<T> items) => default;
    }
}

class Program
{
    static void Main()
    {
        MyCollection<string> x = [];
        MyCollection<int> y = [1, 2, 3];
    }
}

Sharplab

Diagnostic Id: CS0619

Expected Behavior: [] and [1, 2, 3] are highlighted with obsolete error

Actual Behavior: there are no obsolete error highlightings on a [] and [1, 2, 3]

[Rekkonnect]

I think that the obsoletion errors should only be reported on the direct usage of the obsoleted symbol. As in the SharpLab example you provided, there already is a correctly reported obsoletion error on the usage of the Container class, which is enough to break the compilation. If you only had it as a warning, you would then still be able to use the MyCollectionBuilder class that is not actually marked as obsolete itself, it is only contained within an obsolete class and does not actually interact with it.

Note that if you mark MyCollectionBuilder itself as obsolete, you will actually get the obsoletion errors. And this is sufficient, because you are speficially denoting that class as obsolete, and not inferring its obsoletion via its container class. You might be more interested into an Obsolete attribute that also marks all nested members as obsolete.

[Spolutrean]

@Rekkonnect

I think that the obsoletion errors should only be reported on the direct usage of the obsoleted symbol.

If the MyCollectionBuilder is marked there will be an error/warning, but there is no "direct" usage of it. We are using the Create method inside, as well as we are using the inner class MyCollectionBuilder with a Container case. It is not clear what is considered "direct use" in this case. And why, when using a method inside a class, it is considered "direct use" of a class, and when using a class inside a parent class, it is not considered as "direct using" of a parent class.

[Rekkonnect]

And why, when using a method inside a class, it is considered "direct use" of a class

It is not. Example

If the MyCollectionBuilder is marked there will be an error/warning, but there is no "direct" usage of it.

It is. The syntax simply hides it, but the underlying collection type is directly used.

Now this sounds contradictory if we consider the new() feature in this example. We only get errors on the type itself, but not on the new() expression. But there is a good reason for this behavior (as a user), and that is because we are target-typing to an obsolete type, so we do not need extra errors indicating that we are directly using that type.

As a user, I can live and would rather live without those errors. The parent class is not related to the nested class unless explicitly specified (through inheritance, or by having a constructor accepting a parameter of the parent class type, etc.). The nested class contained in the parent class could very well be for structural purposes. Should the parent class be obsoleted, the nested class is not necessarily obsoleted. If we wanted to keep the nested class but not the parent class, we would move that nested class elsewhere, either nesting it in a different class, or having it directly contained in a namespace.

[Spolutrean]

I think "direct" usage should work for all nested classes. Let's look at an example. Here is the container class is marked obsolete, and therefore we don't show highlighting when we create an instance of the obsolete class inside M.

[Rekkonnect]

Good counterpoint, it seems like a bug to me. Or an edge case that was not considered to align with behavior outside the scope of allowing obsolete elements. Might need to reconsider how obsoletion is evaluated as a whole. My point however remains that I would prefer no more errors to appear, and in your last example the warning should appear.

[Spolutrean]

I think it is not possible to reconsider now such a basic think in an obsolete annotation, because of the backward compatibility. For example this obvious bug won't be fixed for the same reason. So I guess it will be more consistent if we accept that all parent classes have a "direct" usage on the called code, and start handle it as I suggested in a ticket.

[Rekkonnect]

It would be very funny if the bug got intentionally fixed with the reasoning "time passed, and you still hold onto obsolete members". But yes, I see where this comes from, it's a largely breaking change. Moving forward, I would like to see those get corrected though. Obsolete members are meant to eventually be completely removed or un-obsoleted after all.

[jaredpar]

This is not covered in the spec but looking at similar behaviors I believe the behavior is correct. Essentially where are other times that the compiler translates syntax into method calls on values and does it consider Obsolete on types / members.

Consider foreach as an example:

D d = new();
foreach (var e in d)
{
}

[Obsolete]
class D 
{
    public class E
    {
        public bool MoveNext() => false;
        public int Current => default;
    }

    public E GetEnumerator() => default!;
}

This gives a diagnostic on the declaration but not on the foreach. If you add an [Obsolete] to say GetEnumerator then a diagnostic is issued in foreach. That seems to match the behavior for collection expressions here (presuming we do give a diagonstic when the member has the attribute.

@cston, @RikkiGibson for feedback

[Spolutrean]

If we consider your example with foreach, it turns out that there is no need to highlight the error if the BuilderType is Obsolete, only if the function itself is Obsolete. But now we do highlight such case :c

[cston]

@Spolutrean, thanks for reporting this issue.

It looks like the compiler may not be consistent with when and where obsolete diagnostics are reported. (See also C# spec.) We’ve discussed this within the Roslyn team and decided that for new features we’ll use the following rule for reporting obsolete diagnostics:

If the source text explicitly mentions a type or member decorated with an ObsoleteAttribute, a diagnostic is reported.

This rule will be used for collection expressions and for new features going forward, but not otherwise applied to existing features.

For collection expressions, this means diagnostics for an obsolete collection builder method (or explicitly referenced containing type) will be reported at the attribute where the method is referenced, but not at each collection expression that implicitly uses the obsolete builder method.

[jaredpar]

Closing as the current implementation matches our defined behavior going forward