Warning not provided for obsolete method ReadOnlySpan.Equals

[sweemer]

Version Used:

$ dotnet --info
.NET SDK:
 Version:           9.0.101
 Commit:            eedb237549
 Workload version:  9.0.100-manifests.4a280210
 MSBuild version:   17.12.12+1cce77968

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19045
 OS Platform: Windows
 RID:         win-x64
 Base Path:   C:\Program Files\dotnet\sdk\9.0.101\

Steps to Reproduce:

1. git clone https://github.com/sweemer/ReadOnlySpanEquals.git
2. cd ReadOnlySpanEquals && dotnet run
3. Unhandled exception. System.NotSupportedException: Equals() on Span and ReadOnlySpan is not supported. Use operator== instead.

Diagnostic Id: CS0618

Expected Behavior: Warning shown when building project.

Actual Behavior: No warning shown, only runtime exception thrown.

Note that IntelliSense in Visual Studio Code correctly flags this method as deprecated, but no warning is shown.

[AlekseyTs]

It looks like the behavior is "By Design", obsolete state of a method is determined based on the least overridden declaration (Object.Equals in this case). There is a comment in Binder.ReportDiagnosticsIfObsolete:

            // There are two reasons to walk up to the least-overridden member:
            //   1) That's the method to which we will actually emit a call.
            //   2) We don't know what virtual dispatch will do at runtime so an
            //      overriding member is basically a shot in the dark.  Better to
            //      just be consistent and always use the least-overridden member.

Compiler also reports a warning when an Obsolete attribute is placed on an override:

warning CS0809: Obsolete member 'S1.Equals(object)' overrides non-obsolete member 'object.Equals(object?)'

Also, apparently the ReadOnlySpan.Equals implementation violates the following rule:

error CA1065: Equals creates an exception of type NotSupportedException. Exceptions should not be raised in this type of method. If this exception instance might be raised, change this method's logic so it no longer raises an exception. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1065)

I am not sure why framework decided to ignore all the "red" flags and applied the Obsolete attribute despite the fact that its presence does not make a difference. Since a span cannot be be boxed, it feels like always returning false would be correct implementation.

[sweemer]

@AlekseyTs Thanks for the thorough explanation. Can this issue be forwarded to the team that maintains the framework to see if there is any room for improvement?

[AlekseyTs]

Transferring to runtime

[AlekseyTs]

CC @stephentoub

[stephentoub]

despite the fact that its presence does not make a difference

It does make a difference:

• It shows up in the docs:

• It shows up in IntelliSense:

• It shows up in formatting:

[AlekseyTs]

It does make a difference:

In my opinion, it doesn't make a difference that people usually expect from an Obsolete attribute. And it doesn't prevent a user code from running into a completely unnecessary (again, in my opinion) exception at runtime.

[colejohnson66]

The idea is that usage of the method is illogical. As ref structs can’t be boxed, the user either knows the type (of at least that it could be a ref struct in a generic), and, in both those situations, T.Equals(object?) is never expected to work. Therefore, throwing at runtime catches this invalid usage instead of silently failing.

I do agree with @AlekseyTs though that such a decision goes against the grain of other BCL decisions. An analyzer alone (or the obsolete marker) should’ve been enough.

[AlekseyTs]

Therefore, throwing at runtime catches this invalid usage instead of silently failing.

Perhaps I am missing something, but returning false for an input that is not equal to the instance is the expected behavior of the Equals method. In my opinion, doing so is not a silent failure, but a success in the task that the method is supposed to perform.

[sweemer]

It shows up in formatting:

Apparently not in Visual Studio Code - see the screenshot in my original post. I will submit another issue about this to the right repo.

But even if it shows up in formatting, it's not as helpful as a compiler warning, which is the usual expectation for obsolete methods. It's not possible to fail a CI build because of formatting, but it is possible to fail a CI build because of a compiler warning.

The idea is that usage of the method is illogical.

Programmers cannot be trusted to always make logical decisions - that is what compilers are for.

An analyzer alone (or the obsolete marker) should’ve been enough.

If CS0618 cannot cover this case then a new analyzer would definitely be welcome.

returning false for an input that is not equal to the instance is the expected behavior of the Equals method.

That would be consistent with ReadOnlySpan's operator ==, which checks whether the left and right point at the same memory, which can never be true for an object right hand side.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.