[Analyzer proposal]: Replace literal enum member ToString with nameof

[drewnoakes]

sharplab

static class C
{
    enum E { A }
    static string M1() => nameof(E.A);
    static string M2() => E.A.ToString();
}

; Core CLR 8.0.724.31311 on x86

C.M1()
    L0000: mov eax, [0x1766db7c]
    L0005: ret

C.M2()
    L0000: mov ecx, 0x2f62c9d0
    L0005: call 0x06b6300c
    L000a: xor ecx, ecx
    L000c: mov [eax+4], ecx
    L000f: mov ecx, eax
    L0011: call dword ptr [0x7046b8c]
    L0017: ret

Potential complications around flags enums.

These are not exactly functionally equivalent as they return different object references.

ReferenceEquals(C.M1(), C.M2()) // false

[huoyaoyuan]

Related: #57748 #70577

These are not exactly functionally equivalent as they return different object references.

User should not depend on identity of strings. We may certainly change to return the same instance in the future.

The string content may change when two enum members are having same values. However, it's considered undefined for ToString in this case: #95617

[dotnet-policy-service]

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

[julealgon]

Strongly dislike this being an analyzer and forcing the optimization onto users. I'd rather it was handled by the compiler or the JIT to make the replacement behind the scenes when possible.

I don't think it is as intuitive to the general dev to start seeing nameof usages like that instead of the much more natural ToString call.

[am11]

Strongly dislike this being an analyzer and forcing the optimization onto users. I'd rather it was handled by the compiler or the JIT to make the replacement behind the scenes when possible.

Same can be said for existing analyzers? JIT is very likely not going to handle every case where a better alternative is available in the ecosystem (nameof keyword in this case).

[KalleOlaviNiemitalo]

Class members have the similar behavior, so analyzer may still be helpful (as long as ToString is not overridden):

static class C
{
    class E { public static int A; }
    static string M1() => nameof(E.A);
    static string M2() => E.A.ToString();
}

Would you expect the analyzer to output a diagnostic for that code? Those methods are not at all equivalent; C.M1() returns "A" but C.M2() returns something like "0".

[julealgon]

@am11

Same can be said for existing analyzers?

Maybe some of them. My gripe here is regarding semantics. To me, something like nameof is a "lower-level construct" when compared to the ToString() method. By forcing devs to make this switch, you are (IMHO) making the code a bit more complex than it needs to be for what appears to be a fairly trivial optimization that could be done elsewhere.

Additionally, I don't think this would cover other similarly trivial cases, such as when the enum is inside of a string interpolation. Would you then have this same analyzer suggest the opimization there as well?

-var text = $"MyEnumValue: {E.A}";
+var text = $"MyEnumValue: {nameof(E.A)}";

If you don't, AFAIK, the first option would end up calling ToString on E.A behind the scenes and suffer from the same penalty as the direct call in the example.

If ToString itself was optimized, it would handle all such situations seamlessly.

JIT is very likely not going to handle every case where a better alternative is available in the ecosystem (nameof keyword in this case).

You are of course correct. But to me, in this particular case, it would be better to keep the well-known and more natural ToString() as the code. And keep in mind we are talking about a specific optimization suggestion here. My take would likely change on a case-by-case basis.

[ericstj]

The behavior here can actually differ in cases of overlapping enum values.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEUCuA7NAExAGoAfAAQCYAGAWAChqBGRxi5gTgAoB5ANxhQANgEMADuICWeAOYBRPDgC2AOgCCw8QAtRASgDc7LtzyjlMCADM+gkROlzFKjVt17DxngKFjJMhSU1ACFPJhMzC2tbXwcA5xCYDH0wxhgggAIfe38nIMYAb0YMkoz1DIBeDOY0YtLNHVFK6tqGUozg5qpW9uCkpqqqRgBfIA===

The enum.ToString value is actually pretty lousy performance as was pointed out by @huoyaoyuan --#57748.