Skip to content

12.6.6.1: Defensive copy of input parameter #894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
KalleOlaviNiemitalo opened this issue Aug 16, 2023 · 17 comments · Fixed by #927
Closed

12.6.6.1: Defensive copy of input parameter #894

KalleOlaviNiemitalo opened this issue Aug 16, 2023 · 17 comments · Fixed by #927
Assignees
Milestone

Comments

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Aug 16, 2023

Describe the bug

In the C# 7.x draft, 12.6.6.1 (Function member invocation / General) describes how a copy of a value-type instance "not classified as a variable" is made and used instead of the original. This defensive copying should also apply to an input parameter unless its type is a readonly struct.

Example

using System;
struct S {
    public int I;
    public void M() { I++; }
}
readonly struct R {
    public void M() {}
}
class C {
    static void Main() {
        N(new S(), new S(), new R());
    }
    static void N(S s1, in S s2, in R r) {
        s1.M(); // no copying
        Console.WriteLine(s1.I); // 1
        
        s2.M(); // copy required
        Console.WriteLine(s2.I); // 0

        r.M(); // no copying
    }
}

Expected behavior

In 12.6.6.1, declare:

If E is not classified as a variable, or if E is an input parameter (15.6.2.3) and V is not a readonly struct type (16.2.2), then a temporary local variable of E’s type is created and the value of E is assigned to that variable.

Additional context

None.


Associated WorkItem - 157261

@KalleOlaviNiemitalo
Copy link
Contributor Author

Need to verify the behaviour for enum types and for type parameters.

@KalleOlaviNiemitalo
Copy link
Contributor Author

KalleOlaviNiemitalo commented Aug 16, 2023

interface I {
    void M();
}
class C {
    void N<T>(T t1, in T t2)
    where T : struct, I {
        t1.M(); // no copying
        t2.M(); // defensive copy
    }
}

IL generated by Roslyn makes a copy even if where T : class, I; but in that case, it's a copy of the reference rather than of the instance itself, so has no observable effect in C#.

@jskeet jskeet added this to the C# 7.x milestone Aug 16, 2023
@KalleOlaviNiemitalo
Copy link
Contributor Author

For a readonly struct, the lack of copying is observable by aliasing; although individual fields are readonly, the whole instance can be assigned.

using System;
readonly struct R {
    public readonly int I;
    public R(int i) {
        I = i;
    }
    public int M() {
        C.StaticR = new R(2);
        return I;
    }
}
class C {
    public static R StaticR = new R(1);
    static void Main() {
        N(in StaticR);
    }
    static void N(in R r) {
        Console.WriteLine(r.I); // 1
        int i = r.M(); // no copying (would copy if not readonly struct)
        Console.WriteLine(i); // 2 (would be 1 if not readonly struct)
        Console.WriteLine(r.I); // 2
    }
}

@KalleOlaviNiemitalo
Copy link
Contributor Author

C# programs cannot define methods in enum types, and the methods of System.Enum do not mutate the instance, so defensive copying does not seem to be observable for enums.

Enum.ToString() could call a custom IFormatProvider, which could replace the enum instance to which this is aliased. However, that would probably happen after Enum.ToString() has already read the underlying value, and thus would not be able to prove that no defensive copy was made. Also, the System.IFormatProvider and System.Globalization.CultureInfo types are not required by Annex C and thus cannot be used by a strictly conforming program.

@KalleOlaviNiemitalo
Copy link
Contributor Author

Need to check whether the special case for readonly struct should also apply when "E is not classified as a variable".

@Nigel-Ecma
Copy link
Contributor

@KalleOlaviNiemitalo – Is the “defensive” copy you refer to the one specified in §12.6.2.3, bullet 1, sub-bullet 2?

@KalleOlaviNiemitalo
Copy link
Contributor Author

@Nigel-Ecma, it is not the same.

@KalleOlaviNiemitalo
Copy link
Contributor Author

KalleOlaviNiemitalo commented Aug 17, 2023

In a method invocation r.M(a)

  • 12.6.2.3 can require copying the value of a to a new place to which an input parameter of M will refer. I don't think that needs changing.
  • 12.6.6.1 can require copying the value of r to a new place to which the this parameter of M will refer. This issue is about making that happen when r is an input parameter of the caller.

@Nigel-Ecma
Copy link
Contributor

@KalleOlaviNiemitalo – OK, I see what you’re talking about now. Thanks.

@KalleOlaviNiemitalo
Copy link
Contributor Author

The description of the example in 12.6.2.3 looks wrong though -- for the M1(i) call, the comment correctly says that "i is passed as an input argument" without a temporary, but then the text below claims that "In each method call, an unnamed int variable is created".

@KalleOlaviNiemitalo
Copy link
Contributor Author

KalleOlaviNiemitalo commented Aug 17, 2023

Ugh, the issue isn't even limited to input parameters. The wording needs to cover readonly fields, ref readonly returns, and ref readonly locals as well.

using System;
struct S {
    public void M() {}
}
class C {
    readonly S f = new S();
    S[] a = new S[1];
    
    void N() {
        f.M(); // copy
        
        ref readonly S r = ref a[0];
        r.M(); // copy
        
        ReadOnlySpan<S> ros = a;
        ros[0].M(); // copy
    }
}

IIUC, each of these is "classified as a variable".

@KalleOlaviNiemitalo
Copy link
Contributor Author

I hoped this could use the same distinction between readonly and writable variable_reference as in assignment expressions and in ref and out arguments. However, the prohibition of readonly fields as out arguments is not in 15.6.2.5 (Method parameters / Output parameters) but rather in 15.5.3.1 (Readonly fields / General).

For assignment, 12.21.2 (Simple assignment) has:

Otherwise, if x is classified as a variable, the variable is not readonly, x has a type T, and y has an implicit conversion to T, then the assignment has the type T.

Now, is there something that says a variable_reference to an input parameter is classified as a variable that is readonly? In the current wording, the readonly seems to be an aspect of the variable itself, not of the variable_reference. Which was okay before the introduction of input parameters and ref readonly returns and locals.

@KalleOlaviNiemitalo
Copy link
Contributor Author

KalleOlaviNiemitalo commented Sep 2, 2023

As another example, I think draft-v7 does not currently forbid this, even though it should:

class C {
    void M(ref int i) {}
    void N(in int i) {
        M(ref i);
    }
}

§15.6.2.3 (Input parameters) says:

It is a compile-time error to modify the value of an input parameter.

but the example above does not violate that; although M could modify the value via the reference parameter, it doesn't do so.

§15.6.2.4 (Reference parameters) does not forbid the variable_reference naming an input parameter.

§9.2.8 (Input parameters) restricts capture and specifies definite assignment but does not mention readonly vs. writable.

§12.6.2.3 (Run-time evaluation of argument lists) requires that the variable for a reference argument be definitely assigned, but does not require writability.

§15.5.3.1 (Readonly fields / General) does say:

Attempting to assign to a readonly field or pass it as an out or ref parameter in any other context is a compile-time error.

but this applies only to readonly fields, not to input parameters. (It doesn't apply to ref readonly locals or returns either, but I didn't check now whether those are disallowed somewhere else.)

§9.7.2.3 (Parameter ref safe context) says:

If p is a ref, or in parameter, its ref-safe-context is the caller-context. If p is an in parameter, it can’t be returned as a writable ref but can be returned as ref readonly

but the second sentence does not apply to passing p as a reference argument.

I think the standard should define readonly variable_reference and writable variable_reference, and then use those definitions when specifying assignments, reference arguments, output arguments, and defensive copying.

@KalleOlaviNiemitalo
Copy link
Contributor Author

KalleOlaviNiemitalo commented Sep 2, 2023

Each of these should be a readonly variable_reference:

  • A readonly field F or this.F, except in constructors of the struct or class in which the field is declared.
  • Member access P or this.P where P is an auto-implemented get-only property whose access is converted to a backing field access that matches the above.
  • Member access e.F where F is a field and e is either a readonly variable_reference, or not a variable_reference at all (e.g. a method call that returns by value).
  • An input argument, including this in readonly struct types. (In the future, also this in readonly methods.)
  • A method or delegate call that returns ref readonly. Includes raising an auto-implemented event that returns such.
  • A ref readonly property or indexer.
  • A ref readonly local, including a var local whose type is deduced = ref a readonly variable_reference.
  • A conditional expression e1 ? ref e2 : ref e3 where at least one of e2 or e3 is a readonly variable_reference.

I hope that's all.

@KalleOlaviNiemitalo
Copy link
Contributor Author

  • Member access e.F where F is a field and e is either a readonly variable_reference, or not a variable_reference at all (e.g. a method call that returns by value).

Huh, apparently e still needs to be a variable_reference:

partial struct S {
    public int I;
}
static class Ex {
    public static ref readonly S GetThat(this in S that) => ref that; // OK
}
partial class C {
    S M1() => new S { I = 5 };
    void M2(in int i) {}
    int M3() => M1().I;
    void M4() {
        //M2(in M1().I); // error CS1612
    }
    void M5() {
        M2(in M1().GetThat().I); // OK, copy M1() to local
    }
}

#if false // C# 11 Low Level Struct Improvements
partial struct S {
    [System.Diagnostics.CodeAnalysis.UnscopedRef]
    public ref readonly S GetThis() => ref this;
}
partial class C {
    void M6() {
        M2(in M1().GetThis().I); // OK, copy M1() to local
    }
}
#endif

@jskeet
Copy link
Contributor

jskeet commented Sep 6, 2023

Meeting 2023-09-06: Assigning to Bill, who will find the relevant places and also discuss with the compiler team.

BillWagner added a commit to BillWagner/csharpstandard that referenced this issue Sep 12, 2023
First commit:

Fix the issue raised in the description of dotnet#894: If a method takes an `in` parameter of a struct type that is not a `readonly struct`, then the compiler must create a temporary to invoke a non-readonly member of that type.
@KalleOlaviNiemitalo
Copy link
Contributor Author

KalleOlaviNiemitalo commented Sep 13, 2023

I'm not sure that #927 addresses ref conditional expressions. It might but I haven't verified.

I mean this kind of code

struct S {
    public void M() {}
}
class C {
    void N(bool b, ref S r, in S i) {
        (b ? ref r : ref i).M();
    }
}

The compiler behaviour seems to be the same as

S s = (b ? r : i);
s.M();

i.e., if b is true, then the S instance referenced by r is copied, even though r.M() would have used the reference without copying.

And if I change the parameter list to (bool b, ref S r, ref S i), then the behaviour becomes the same as

ref S s = ref (b ? ref r : ref i);
s.M();

i.e., no instance of S is copied.

This behaviour seems to mean that the ref conditional expression as a whole is considered a writable ref or a readonly ref, depending on whether the second and third operands are writable refs, and the defensive copying then depends on this decision.

@BillWagner BillWagner moved this from 🔖 Ready to 🏗 In progress in dotnet/docs September 2023 sprint Sep 13, 2023
@BillWagner BillWagner moved this from 🏗 In progress to 👀 In review in dotnet/docs September 2023 sprint Sep 13, 2023
BillWagner added a commit to BillWagner/csharpstandard that referenced this issue Sep 13, 2023
First commit:

Fix the issue raised in the description of dotnet#894: If a method takes an `in` parameter of a struct type that is not a `readonly struct`, then the compiler must create a temporary to invoke a non-readonly member of that type.
BillWagner added a commit to BillWagner/csharpstandard that referenced this issue Sep 13, 2023
jskeet pushed a commit that referenced this issue Sep 20, 2023
First commit:

Fix the issue raised in the description of #894: If a method takes an `in` parameter of a struct type that is not a `readonly struct`, then the compiler must create a temporary to invoke a non-readonly member of that type.
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in dotnet/docs September 2023 sprint Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants