Hide ReferenceEquals and Equals for custom types in IntelliSence

[YegorStepanov]

Introduction

It's annoying to see ReferenceEquals and Equals anywhere.

Static classes

Both methods are meaningless:

Math.Equals(...);
Math.ReferenceEquals(...);

Assert, Serilog Log and small user's helper classes suffer from this at code completion.

Struct

The static Equals is wrong for any struct.

int.Equals(3, 3);

This bug easily appears when changing a class to a struct in a user type.

ReferenceEquals

ReferenceEquals is used via object or via direct call. I've never seen the code like:

string.ReferenceEquals(...);

Also this is a low-level method, but you see it at every code completion for a type.

Proposal

Hide static Equals for structs and static classes.
Hide ReferenceEquals for all types, except object.

This should be done by hiding them from IntelliSense. The new analyzer will complain about the use of these hidden methods.

Questions

Should the direct Equals call be hidden? It will help to better detect the "changing a class to a struct" bug.

struct S
{
    void M() { Equals(this, this); } // use object.Equals(...) instead
}

[CyrusNajmabadi]

@sharwell any concerns doing this?

[sharwell]

Yes, this is certain to break the experience for users who intentionally go through a derived type.

[sharwell]

This is the only documented request we've ever had to hide ReferenceEquals from the completion list. The Equals case has come up in specific contexts before and is covered by #4434. Due to risk of breaking valid scenarios and lack of requests, we will almost certainly not be implementing this request.

[CyrusNajmabadi]

wfm.

[YegorStepanov]

@sharwell

The Equals case has come up in specific contexts before and is covered by #4434.

The part about static-class can't be implemented by EditorBrowsableAttribute. It should be handled directly in IntelliSense logic.

Yes, this is certain to break the experience for users who intentionally go through a derived type.

Did you mean about MyType.ReferenceEquals ?
Isn't it good that the user should use Object for low-level infrastructure methods?
My screenshots show that we are just used to not noticing infrastructural methods, but this is not good.

You've not explicitly rejected the following cases:

Static classes

There no one reason for existing ReferenceEquals and object.Equals methods except you've made the great .NET by one year.

The static classes are implemented System.Object. But it is the details of implementation from user side.

It's good to always warning the user that the current code is unclear/not modern and safe.

Struct Equals

Are you agree this is incorrect method always? For all structs?

int.Equals(3, 5);

We understand this case only because we remembered what wrong with it. This will allow us to forget one special case in the infrastructure, it reduces the language barrier.

This bug quite easily introduced by changing class to struct. The new analyzer will display some warning.
Hmm, it will almost free to change readonly class to readonly struct in easy-medium scenarios (except perf issues).

The end

It was strange not to find anything on the Internet about this. It was my duty to report this.

If the new small rules will be more modern, why not gently point users to this.

FYI @stephentoub

[YegorStepanov]

#59420 (reply in thread)
@CyrusNajmabadi

I'm not sure what you're asking for. However, you could certainly write your own analyzer to find this and force you to stop. You could also suggest such an analyzer at github.com/dotnet/roslyn-analyzers.

Currently, the static classes are common classes with private constructor.
The proposal suggests treating them as a set of functions, that don't implement System.Object.

Is it not a language changes?

Since this changes the internals of C#, I suggest simply hiding Equals/ReferenceEquals as it will look the same to the user.

Similar situation with structs.

[CyrusNajmabadi]

Is it not a language changes?

I don't see anything in your proposal that actually changes the language. Can you specifically call that out? It looked like this was all in the tooling space.

[YegorStepanov]

Warning and no code completion:

Math.Equals(...)
Math.ReferenceEquals(...)
int.Equals(...)
int.ReferenceEquals(...)
string.ReferenceEquals(...)

Can be implemented like:

class object
{
  - public static bool ReferenceEquals(object a, object b);
  + protected static bool ReferenceEquals(object a, object b);
}

class ImaginaryBaseClassForAnyStaticClass: object
{
    protected static new bool Equals(object a, object b);
}

class ValueType: object
{
    protected static new bool Equals(object a, object b);
}

Hide direct Equals call for structs?

class ValueType: object
{
    private static new bool Equals(object a, object b);
}

But since we are not allowed to break existing code, this will be implemented in the most soft way.

But for a user who writes new code, the code above will be truthful (if we forget about reflection).

[CyrusNajmabadi]

I don't get what language change is being asked for here.

class object
{
  - public static bool ReferenceEquals(object a, object b);
  + protected static bool ReferenceEquals(object a, object b);
}

This would break every single piece of code out there taht does things like object.ReferenceEquals(a, b); As an example, there were almost 300 usages of that in roslyn itself.

class ValueType: object
{
    private static new bool Equals(object a, object b);
}

This would have no effect as it's a private symbol. Someone calling int.Equals(a, b) would jsut not see that method and woudl find the one on object.

[YegorStepanov]

The first note:
I forgot this case. The object.ReferenceEquals(...) and a direct ReferenceEquals(...) both are allowed.

The second note:
I thought the method would be hidden.

I thought that hiding members from static classes changes the language.

Okay, you've convinced me it's not language related. I've changed the proposal a little.

[YegorStepanov]

Wow, The Czech rider hides ReferenceEquals and Equals from all types, except object and string (only string.ReferenceEquals is hidden).

Amazing attention to details. I don't know if R# does it or not.

[RaphaelTetreault]

For what it's worth, I'm in agreement that it's unnecessary clutter. I'm done writing a simple 2D Game framework for Game Design college students, using a lot of simple static classes to manage key variables and function calls. However, Equals and ReferenceEquals littered in every static class is a nuisance and distracts from usage and learning.

If this isn't something to edit on the implementation side, can't there just be one more checkbox in the sea of checkboxes in Options to hide it (or any other editor change), as Yegor mentioned Rider does something similar?

[CyrusNajmabadi]

Each option adds complexity and maintenance burden. We generally only add them when there is very high value.