Incorrect optimization: a faulty load could be eliminated when compiling C# to IL

[GeeLaw]

Version Used: 5.0.0-2.25523.111 (b0f34d51), language version 14.0.

Steps to Reproduce:

public static class Test
{
  public readonly ref struct S
  {
    public readonly ref int Reference;
  }
  public static ref int GetReference(S s)
  {
    return ref s.Reference;
  }
  // Cases below.
  public static void DiscardParam(ref int r)
  {
    _ = r;
  }
  public static void DiscardMethod()
  {
    _ = GetReference(default);
  }
  public static void DiscardDirect()
  {
    _ = default(S).Reference;
  }
  public static void UnusedLocalParam(ref int r)
  {
    int unused = r;
  }
  public static void UnusedLocalMethod()
  {
    int unused = GetReference(default);
  }
  public static void UnusedLocalDirect()
  {
    int unused = default(S).Reference;
  }
}

Expected Behavior: The IL of all methods below Cases below. tries to load a value from the reference.

Actual Behavior:

• When optimization is on, methods ...Param and ...Method do not have ldind.i4.
• When optimization is off, methods DiscardParam and DiscardMethod does not have ldind.i4.
• Regardless of optimization, methods ...Direct always has ldind.i4 (desired).

Explanation: In certain of my libraries, I decide to trigger NullReferenceException (note 1) prior to doing anything to the reference. Consider the following code:

static void CopyBackwards(ref int src, ref int dst, int count)
{
  if (count == 0)
    return;
  _ = src;
  _ = dst;
  for (int i = count; i != 0;)
  {
    --i;
    Unsafe.Add(ref dst, i) = Unsafe.Add(ref src, i);
  }
}

It's important to trigger access on src, dst before trying anything, so that we get NullReferenceException appropriately. Otherwise (suppose we don't check for it), if count is very large, then the following bad things can happen:

• The first iteration succeeds, corrupting memory.
• The first iteration fails, but the exception thrown is AccessViolationException.

Note 1: I know the recommended way is to check for null and throw ArgumentNullException. This bug report has nothing to do with best practices. Also, the context is actually in .NET Framework, which I understand might not be tracked in this repo. But the bug reproduces in .NET 10.0, so it should be relevant.

Analysis:

Prior to the introduction of Span<T> and more generally ref fields, the only possible ref that can be created by a purely C# program is always valid non-null. Therefore, eliminating the load was valid as far as C# is concerned.

However, the introduction of ref fields and the default initialization behavior means ref could point to null, so reading from it then discarding the result is not always a no-op. The side effect should be preserved.

[jjonescz]

Expected Behavior: The IL of all methods below Cases below. tries to load a value from the reference.

I don't see this guaranteed by the C# language standard. Specifically, https://github.com/dotnet/csharpstandard/blob/draft-v8/standard/variables.md#97-reference-variables-and-returns says:

When a reference variable is used where a value is required its referent’s value is returned

When you use a ref variable in a discard, its value is not required, so there doesn't seem to be any need to dereference it and so the compiler is free to optimize it away. At least that is my interpretation. For more details, a discussion or issue in the dotnet/csharpstandard or dotnet/csharplang repo would be better. (Specifically, the standard needs to be clear about the behavior before we can fix this in the compiler.) Thanks.

It's important to trigger access on src, dst before trying anything, so that we get NullReferenceException appropriately

Consider using Unsafe.IsNullRef<T> for that.

[GeeLaw]

So I cloned csharpstandard, checked out draft-v8, and searched for discard (case-insensitive, non-whole-word). What I found:

• discards are local variables,
• every time a discard is mentioned, it's a new local variable,
• there is no special provision for _ = EXPR other than the meaning of above, plus that of a declaration with variable initializer.

Those agree with my mental model of discards. Therefore, the following two excerpts of C# code shall have the same meaning:

// excerpt 1
_ = EXPR;

// excerpt 2
var unused1 = EXPR;
// unused1 is a valid identifier not appearing anywhere else.

Roslyn doesn't compile both to the same effect in debug mode.

My understanding is that

static void Foo(ref int arg)
{
  int unused = arg;
}

requires the value of arg. Therefore, so is

static void Foo(ref int arg)
{
  _ = arg;
}

The version that doesn't use the value of arg would be

static void Foo(ref int arg)
{
  _ = ref arg;
}

But I agree with you that the standard is lacking regarding ref things pointing to null, because that's fairly new in C#. I'll open a discussion and link it here.

[GeeLaw]

Oh wow... The last example _ = ref arg is actually incorrect, because 12.19 doesn't allow ref-assignment for discards, but Roslyn accepts it.