Skip to content
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

ref readonly is not allowed in local_variable_initializer #831

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

pkulikov
Copy link
Contributor

@pkulikov pkulikov commented Jul 1, 2023

Fixes #828

@jskeet jskeet added this to the C# 7.x milestone Jul 4, 2023
@jskeet
Copy link
Contributor

jskeet commented Jul 4, 2023

@Nigel-Ecma and @BillWagner : I think this sounds right - any concerns?

@Nigel-Ecma
Copy link
Contributor

@jskeet & @BillWagner – This isn’t a right/wrong issue, both the current and proposed text are valid designs. Requiring readonly looks like intentional design to me – which makes Roslyn non-compliant; I was hoping someone who knew the designer's intention would weigh in. If the intention is to not allow/require readonly then this PR is good.

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Jul 4, 2023

Generally, C# allows ref readonly only in front of a type, not in front of an expression. For example, the return statement and the ? : operator allow ref but not ref readonly. If ref readonly were allowed (or even required) in local_variable_initializer, that would be particularly inconsistent with ref reassignment in C# 7.3.

Possible combinations of ref and ref readonly:

public class C {
    int a = 0;
    readonly int b = 0;

    public void M() {
        ref int ra1 = ref a;
        ref int ra2 = ref readonly a;
        ref readonly int ra3 = ref a;
        ref readonly int ra4 = ref readonly a;
        
        ref int rb1 = ref b;
        ref int rb2 = ref readonly b;
        ref readonly int rb3 = ref b;
        ref readonly int rb4 = ref readonly b;
    }
}

In the Roslyn implementation, the initializations of ra2, ra4, rb2, and rb4 are invalid because ref readonly is not allowed in the initializer; and the initialization of rb1 is invalid because you cannot make a mutable reference to readonly int b.

If ref readonly were allowed and required in the initializer, then I think:

  • ra1 should be OK.
  • ra2 should be invalid (cannot convert ref readonly int to ref int).
  • ra3 should be OK (implicit conversion from ref int to ref readonly int).
  • ra4 should be OK.
  • rb1 should be invalid (cannot reference readonly int b with plain ref).
  • rb2 should be invalid (cannot convert ref readonly int to ref int).
  • rb3 should be invalid (cannot reference readonly int b with plain ref).
  • rb4 should be OK.

And the standard would need extra wording to achieve that.

@KalleOlaviNiemitalo
Copy link
Contributor

Declaring and using ref readonly locals in the C# 7.2 proposal shows the initialization ref readonly var r1 = ref M1(); without using ref readonly in the initializer.

@Nigel-Ecma
Copy link
Contributor

@KalleOlaviNiemitalo – It could be said that your rb1 example shows C# as being less clear than it could be; that said either design is valid. I would wager that this PR follows the intended design, not least because = ref is classified as an operator – though whether that was intended by the original design or was just a choice of the compiler team in this particular case I can’t say for certain… ;-)

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Jul 5, 2023

The difference between ref and the address-of operator & is most clear in ? :.

public class C {
    public unsafe void M() {
        int x = 1, y = 2, z = 3;
        int i = 0;
        ref int r = ref i == 0 ? ref x : ref (i == 1 ? ref y : ref z);
        int* p = i == 0 ? &x : (i == 1 ? &y : &z);
    }
}

SharpLab

The subexpression (i == 1 ? ref y : ref z) evaluates to a reference but it still needs ref in front of it in order to preserve the referenceness through the outer ? : expression.

@KalleOlaviNiemitalo
Copy link
Contributor

your rb1 example shows C# as being less clear than it could be

Because rb1 would be invalid in both designs, I don't understand your point.

@Nigel-Ecma
Copy link
Contributor

@KalleOlaviNiemitalo – I completely confused you as to the point I was making. Using an underscore to indicate a strong binding the compiler parses your first assignment example as:

ref int ra1 =_ref a;

i.e. ref is not a unary operator applied to an variable_reference which impacts how it is treated; = ref is a distinct operator from =; and something similar applies to the ref-conditional.

Is this binding a language design decision or a compiler implementation choice? As I said I’d wager which one it is but I don’t know, and I don't know where = ref readonly came from and its not illogical per se.

Let's wait for someone who knows.

@KalleOlaviNiemitalo
Copy link
Contributor

= ref readonly is not in the proposal Readonly references. I guess it was added by mistake when the C# standard draft was edited. Or did the editors use some other source than this proposal?

@BillWagner
Copy link
Member

I think the intention is to disallow ref readonly in the variable initializer (which makes this PR OK.)

Tagging @cston, @RikkiGibson and @jaredpar who would know for certain.

@KalleOlaviNiemitalo
Copy link
Contributor

dotnet/roslyn#118 "Proposal: Ref Returns and Locals" (from Jan 28, 2015) includes this example:

public static readonly ref int Max(
    readonly ref int first, readonly ref int second, readonly ref int third)
{
    readonly ref int max = first > second ? ref first : ref second;
    return max > third ? ref max : ref third;
}

which differs from the current implementation in three ways: (1) readonly ref instead of ref readonly; (2) initializes a ref local with = rather than = ref; (3) returns a ref with return rather than return ref. So, these aspects were apparently changed after that was posted.

Ref locals were mentioned in the following meeting notes and discussion issues, but those changes were not:

C# Design Meeting Sep 1 2015 shows the = ref initialization syntax and return ref. The syntax for ref assignment and "readonly refs" had not been decided yet.

More brief mentions of ref locals or readonly refs:

C# Language Design Notes for Feb 22, 2017 links to the ref readonly proposal dotnet/csharplang#38. Particularly dotnet/csharplang#38 (comment) ponders the differences between ref readonly, readonly ref, and readonly ref readonly. Doesn't seem to have discussion about = ref grammar.

C# Language Design Notes for Feb 28, 2017 introduces "Conditional operator over refs" dotnet/csharplang#223 and discusses how many ref keywords the syntax should require. There is a decision not to use condition ? ref readonly v1 : ref readonly v2, and a note that "ref is not part of the expression".

C# Language Design Notes for Mar 1, 2017 continues with conditional refs and approves the condition ? ref v1 : ref v2 syntax.

C# Language Design Notes for May 31, 2017 discusses modreq generated from some ref readonly. Doesn't discuss C# syntax for refs.

C# Language Design Notes for Jul 5, 2017 approves shipping the ref features in C# 7.2.

C# Language Design Notes for Aug 28, 2017 discusses ref-like types.

C# Language Design Notes for Sep 25. 2017 adds ref readonly locals. This shows the initialization ref readonly var r = ref a[1]; but also considers shorter syntax ref readonly int x = a[1]; near the end.

C# Language Design Notes for Sep 27, 2017 discusses argument syntax for ref readonly parameters, and decides to require ref in front of the argument expression but not ref readonly. This also introduces in parameters.

Milestone philosophy (Oct 2, 2017) again shows ref readonly tmp = ref Get(); syntax.

I think this shows that the language design team decided to initialize with = ref and not = ref readonly.

@jaredpar
Copy link
Member

jaredpar commented Jul 5, 2023

I think this shows that the language design team decided to initialize with = ref and not = ref readonly.

That is correct. This also follows to more recent features like [ref readonly] where ref is still a legal passing mechanism. The design team decided the complexity of allowing a ref readonly expression syntax didn't outweigh the benefits and stuck with simply ref.

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsurprisingly it has been confirmed that the “[language] design team decided the complexity of allowing a ref readonly expression syntax didn’t outweigh the benefits and stuck with simply ref”. So this is all good :-)

@BillWagner BillWagner merged commit 0df4171 into dotnet:draft-v7 Jul 6, 2023
@pkulikov pkulikov deleted the patch-2 branch July 8, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C# 7 §13.6.2 ref readonly in local_variable_initializer
6 participants