dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.69k stars 3.98k forks source link

Unnecessary defensive copies on enums passed by `in` #72016

Open hamarb123 opened 4 months ago

hamarb123 commented 4 months ago

The compiler emits unnecessary defensive copies on enums passed by in.

Version Used:

Sharplab.io Also reproduces with the compiler included in the .NET 8.0.100 SDK

Steps to Reproduce:

using System;
public class C
{
    public void M<T1, T2>(in Enum1 value1, ref Enum1 value2, Enum1 value3, in T1 value4, in T2 value5) where T1 : Enum where T2 : struct, Enum
    {
        value1.ToString(); //defensive copy (unnecessary)
        value2.ToString(); //no defensive copy
        value3.ToString(); //no defensive copy
        value4.ToString(); //defensive copy (unnecessary)
        value5.ToString(); //defensive copy (unnecessary)
    }
}
public enum Enum1
{
}

Link

Expected Behavior:

None of the calls to .ToString() emit a defensive copy, since none is required for any of them.

Actual Behavior:

value1.ToString() emits a defensive copy, and similar for value4 and value5.

ghost commented 4 months ago

Hey, I could take this, but just to ask a clarification: is there any circumstances in which the defensive copy is of use here?

I would think no since I can't seem to find anything that incurs side effects on System.Enum, but I'm just asking to be sure.

hamarb123 commented 4 months ago

None of the default implementations of the methods on System.Enum, System.ValueType or System.Object mutate, furthermore the ECMA specification says that you cannot provide additional methods or overrides on enums (see II.22.37 - 33. If Extends = System.Enum (i.e., type is a user-defined Enum) then). Therefore, any member access on any value of type Enum is provably non-mutating (note that this is not the case for ValueType or Object in general, since you can override ToString and similar in structs).

jaredpar commented 4 months ago

None of the default implementations of the methods on System.Enum, System.ValueType or System.Object mutate, ...

It's true that they do not mutate today. This PR and issue is essentially saying that they can never mutate again in the future because it's baking in that assumption into codegen. Further there are more .NET runtimes than the ones you listed here: SPOT for example. Before going forward with this PR we'll need to square with the runtime team if they're comfortable with this change being made and essentially saying that all compliant runtimes cannot have mutating methods on enum.

@davidwrighton, @jjonescz