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

There are some unnecessary defensive copies for deconstructors, implicit index & range, and foreach with explicit implementation #72008

Open hamarb123 opened 4 months ago

hamarb123 commented 4 months ago

When a member is marked as readonly, a defensive copy need not be made, since the member will not mutate the struct. These are some instances of where we could avoid more defensive copies.

Version Used:

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

Steps to Reproduce:

using System;
using System.Collections;
using System.Collections.Generic;

public class C
{
    public void M1(ref S1 value1, ref S2 value2)
    {
        int a, b;

        (a, b) = value1; //defensive copy (unnecessary, since value1 is mutable?)
        a = value1[^3]; //no defensive copy
        _ = value1[3..4]; //no defensive copy
        foreach (var x in value1) { } //no defensive copy

        (a, b) = value2; //defensive copy (unnecessary)
        a = value2[^3]; //no defensive copy
        _ = value2[3..4]; //no defensive copy
        foreach (var x in value2) { } //no defensive copy
    }

    public void M2(in S1 value1, in S2 value2)
    {
        int a, b;

        (a, b) = value1; //defensive copy
        a = value1[^3]; //defensive copy
        _ = value1[3..4]; //defensive copy
        foreach (var x in value1) { } //defensive copy

        (a, b) = value2; //defensive copy (unnecessary)
        a = value2[^3]; //defensive copy (unnecessary)
        _ = value2[3..4]; //defensive copy (unnecessary)
        foreach (var x in value2) { } //defensive copy (unnecessary)
    }
}

public struct S1 : IEnumerable<int>
{
    public void Deconstruct(out int a, out int b) => (a, b) = (1, 2);
    public int Length => 0;
    public int this[int a] => 0;
    public S1 Slice(int start, int length) => this;
    IEnumerator IEnumerable.GetEnumerator() => null!;
    IEnumerator<int> IEnumerable<int>.GetEnumerator() => null!;
}

public struct S2 : IEnumerable<int>
{
    public readonly void Deconstruct(out int a, out int b) => (a, b) = (1, 2);
    public readonly int Length => 0;
    public readonly int this[int a] => 0;
    public readonly S2 Slice(int start, int length) => this;
    readonly IEnumerator IEnumerable.GetEnumerator() => null!;
    readonly IEnumerator<int> IEnumerable<int>.GetEnumerator() => null!;
}

Link

Expected Behavior:

Members marked as "defensive copy (unnecessary)" do not emit a defensive copy.

Actual Behavior:

Members marked as "defensive copy (unnecessary)" do emit a defensive copy. The rest are correct.

jaredpar commented 4 months ago

Looking through the samples I would say that essentially there are three issues:

  1. Deconstruct seems to be always emitting a defensive copy pretty much always. This seems like an omission in the code. The copies are happening because we pass the receiver through EvaluateSideEffectingArgumentToTemp. Given this is a ref it can change between reads hence a copy is generated. In these specific cases though it's not needed. @jcouv, @RikkiGibson any thoughts?
  2. Seems like index / range isn't accounting for readonly members correctly.
  3. Seems like foreach doesn't account for readonly members correctly.

Even so ... probably rate this on the lower end of optimizations. Will likely end up as "help-wanted".