Changes for ref-valued methods, properties, et al. #837

Nigel-Ecma · 2023-07-06T04:26:39Z

This is a first draft of changes to address #825.

At this stage only ref-valued properties have been considered, other ref returning functions will follow later.

The grammar has been extended to be as prescriptive as reasonably possible as befits a language specification[1], this means that former statements such as it being illegal to have a set accessor for a ref-valued property are now covered by the grammar. However the usual verbose description of the grammar remains!

The term ref-valued rather than ref-returning is used, a term closer to the language of fields. To distingush the two kinds of properties, which do follow different rules, the corresponding and somewhat ugly term non-ref-valued is used (non-ref-returning would be just as ugly).

Please fire off brickbats, and maybe even bouquets, on the direction of this. TIA

[1] The grammar used by a compiler may be less restrictive to support more descriptive error messages, that is of course an implementation issue!

… locally at the moment :( )

standard/classes.md

Typos found in the first stage have been fixed. Comments on the first stage also addressed some longer standing issues, these have been addressed which means a few more files have been impacted. In particular double-spaces have been removed and some files have bene impacted just for that. There has been knowen issues with the sharing the clauses related to accessors, currently sub-clauses of Properties, between properties and indexer. Some duplication has been removed as part of this, and some *Note*s added. A re-org of some of this material could be warranted but this PR does not (currently) do that. Please review and fire submit your brickbats, and even bouquets, so we can get this sorted before Thursday if possible!

…again!

standard/classes.md

…ethods. The existing grammar for *method_declaration* *et al.* tended towards the less restrictive with the prose detailing the myriad of restrictions/combinations allowe/etc. It was quite gnarly at times and adding in returns-by-ref methods, and their set of restrictions, doesn’t help… The new grammar is a little more precise, though still liberal, which allows some of the prose to be removed (e.g. no need to say something is disallowed when its a grammartical impossibility) while adding in prose for the returns-by-ref case. I.e. I took some of the gnarly stuff out and replaced it new gnarly stuff, I make no claim it is over *less* gnarly just less than it would be wthout the grammar refactor… Of course the balance between grammar & prose semantic restrictions is subjective rather than objective, feel free to discuss. Still to do are local & anonymous functions, interfaces, a review of the delegate stuff, maybe something in the overrides… As before, please review and fire off your brickbats & bouquets and spot those sneaky typos!

jskeet

A few comments, but otherwise looks fine to me - which isn't to say I've verified it's complete; only that the changes here do make sense to me!

standard/classes.md

Nigel-Ecma · 2023-07-12T04:22:53Z

Comment for last commit – somehow I managed to kill vi, rather than git as usual, my corpse collection expands! This means I have to remember what I typed…

This is the last set of changes to address #825 & #826 and covers local functions, delegate and interfaces.

The grammar phrase 'ref' 'readonly'? was getting used a lot and the prose around it sometimes tortuous, so the grammar rule ref_kind : 'ref' | 'ref' 'readonly' ; (longhand to make the two alternatives clear) has been introduced and the prose simplified by using it.

The grammars for local functions, delegates & interfaces have also been updated to follow those for methods et al. to keep things consistent.

Quite a few typos were found during the editing and these have been fixed, but don’t panic a similar number have probably been added so typo hunting is still required! ;-)

Please fire off brickbats & bouquets as usual, but further changes prior to the next meeting are unlikely.

Now everything is in theory done the “draft” status of this PR will be removed.

standard/delegates.md

standard/classes.md

jskeet · 2023-08-07T15:29:48Z

@Nigel-Ecma: Can you take another pass at the comments on this PR now? I'm happy for merge as-is with future work noted in another issue, if that simplifies things.

Nigel-Ecma · 2023-08-07T19:06:45Z

@jskeet – Will do, hopefully later this week…

…

On 8/08/2023, at 3:29 am, Jon Skeet ***@***.***> wrote: @Nigel-Ecma <https://github.com/Nigel-Ecma>: Can you take another pass at the comments on this PR now? I'm happy for merge as-is with future work noted in another issue, if that simplifies things. — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSYVW63DVWZU3SXDXHKZYLXUECXPANCNFSM6AAAAAAZ72CFBU>. You are receiving this because you were mentioned.

Nigel-Ecma · 2023-08-10T05:22:49Z

@jskeet – Reviewed remaining comments, pushed two to issue #886, all marked as resolved. If you're happy merge away!

standard/statements.md

jskeet

Just a very few comments now - sorry not to be able to merge immediately, but I think you'll agree they should be fixed first.

standard/classes.md

standard/statements.md

…nock-on effect that a clause list constructed from those with notes was also wrong. Thanks to Jon for spotting this! Also a few typos fixed and minor wording changes.

KalleOlaviNiemitalo · 2023-08-17T03:44:10Z

standard/statements.md

@@ -356,7 +356,7 @@ The value of a local variable is obtained in an expression using a *simple_name*

 The scope of a local variable declared in a *local_variable_declaration* is the block in which the declaration occurs. It is an error to refer to a local variable in a textual position that precedes the *local_variable_declarator* of the local variable. Within the scope of a local variable, it is a compile-time error to declare another local variable or constant with the same name.

-A local variable declaration that declares multiple variables is equivalent to multiple declarations of single variables with the same type and `ref`/`ref readonly` prefix. Furthermore, a variable initializer in a local variable declaration corresponds exactly to an assignment statement that is inserted immediately after the declaration.
+A local variable declaration that declares multiple variables is equivalent to multiple declarations of single variables with the same type and *ref_kind*. Furthermore, a variable initializer in a local variable declaration corresponds exactly to an assignment statement that is inserted immediately after the declaration.


It no longer corresponds exactly, because of ref safety.

using System; public class C { public void M() { Span<int> s1 = stackalloc int[1]; // OK Span<int> s2; s2 = stackalloc int[1]; // error CS8353 } }

I've filed #899 for this.

Nigel-Ecma · 2023-08-17T04:47:50Z

The raised error is: CS8353: A result of a stackalloc expression of type 'Span<int>' cannot be used in this context because it may be exposed outside of the containing method What logic is the compiler using to determine s2 may be exposed but not s1? Am I missing the glaringly obvious, its been a long day, or is something amiss here?

…

On 17/08/2023, at 3:44 pm, KalleOlaviNiemitalo ***@***.***> wrote: public void M() { Span<int> s1 = stackalloc int[1]; // OK Span<int> s2; s2 = stackalloc int[1]; // error CS8353 }

KalleOlaviNiemitalo · 2023-08-17T05:08:50Z

Once a Span local has been initialised with stackalloc, it is apparently OK to assign other stackalloc expressions to it:

using System;
public class C {
    public void M() {
        Span<int> s1 = stackalloc int[1]; // OK
        
        s1 = stackalloc int[1]; // OK
    }
}

I get the impression that the ref safety aspects of the local variable depend on whether and how it is initialised, and they then affect whether further assignments are considered safe. But I haven't checked whether this is already specified adequately.

KalleOlaviNiemitalo · 2023-08-17T05:15:56Z

9.7.2.1 (Local variable ref safe context):

If v is a reference variable, its ref-safe-context is the same as the ref-safe-context of its initializing expression.

Otherwise its ref-safe-context is the context in which it was declared.

So this is how the ref-safe-context depends on the initialiser. However the term "reference variable" defined in 9.7.1 does not cover Span<int> s1 as this declaration does not include the ref modifier.

KalleOlaviNiemitalo · 2023-08-17T05:22:10Z

Likewise in 9.7.2.8 (Limitations on reference variables) there's:

For a ref reassignment ref e1 = ref e2, the ref-safe-context of e2 must be at least as wide a context as the ref-safe-context of e1.

which does not cover assignment of Span<T> as that is done with = rather than = ref.

jskeet

This looks good to me; I've filed a new issue for the latest problem noted by @KalleOlaviNiemitalo, but that wasn't introduced in this PR, so I'll merge this in the interests of making progress.

jskeet · 2023-08-17T06:02:16Z

standard/statements.md

@@ -356,7 +356,7 @@ The value of a local variable is obtained in an expression using a *simple_name*

 The scope of a local variable declared in a *local_variable_declaration* is the block in which the declaration occurs. It is an error to refer to a local variable in a textual position that precedes the *local_variable_declarator* of the local variable. Within the scope of a local variable, it is a compile-time error to declare another local variable or constant with the same name.

-A local variable declaration that declares multiple variables is equivalent to multiple declarations of single variables with the same type and `ref`/`ref readonly` prefix. Furthermore, a variable initializer in a local variable declaration corresponds exactly to an assignment statement that is inserted immediately after the declaration.
+A local variable declaration that declares multiple variables is equivalent to multiple declarations of single variables with the same type and *ref_kind*. Furthermore, a variable initializer in a local variable declaration corresponds exactly to an assignment statement that is inserted immediately after the declaration.


I've filed #899 for this.

jskeet · 2023-08-17T06:03:35Z

Ah... can't merge immediately due to conflicts. @Nigel-Ecma are you happy to rebase, or would you like me to do so?

Nigel-Ecma · 2023-08-17T08:37:46Z

@jskeet – I’ll take a look my tomorrow and if you haven’t done it in your today I’ll see if I can do it without my usual git-breaking skill…

…

On 17/08/2023, at 6:03 pm, Jon Skeet ***@***.***> wrote: Ah... can't merge immediately due to conflicts. @Nigel-Ecma <https://github.com/Nigel-Ecma> are you happy to rebase, or would you like me to do so? — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSYVW5N4PTLJPCBRQM2I73XVWX4RANCNFSM6AAAAAAZ72CFBU>. You are receiving this because you were mentioned.

KalleOlaviNiemitalo · 2023-08-17T09:30:14Z

OK, the rule for how the initializer of a Span<T> local variable affects the ref safety of that variable is actually specified in §16.4.12.3 (Local variable safe context):

Otherwise if the variable’s declaration has an initializer then the variable’s safe-context is the same as the safe-context of that initializer.

That explains the CS8353 error. It is not necessary to change §9.7.2 to cover ref struct variables in addition to reference variables.

It could be useful to add a note to §9.7.2.1 (Ref safe contexts / General):

Note: Safe context for expressions of ref struct type is defined in §16.4.12. end note

§16.4.12.1 (Safe context constraint / General) already has a link to §9.7.2:

There are three different safe-context values, the same as the ref-safe-context values defined for reference variables (§9.7.2): declaration-block, function-member, and caller-context.

Filed this as #901.

jskeet · 2023-08-17T10:35:16Z

I’ll take a look my tomorrow and if you haven’t done it in your today I’ll see if I can do it without my usual git-breaking skill…

I've managed to fix everything, but I can't force push to your branch. I'll force push to my fork, create a PR from that and merge it.

(This was originally in dotnet#837.) The grammar has been extended to be as prescriptive as reasonably possible as befits a language specification[1], this means that former statements such as it being illegal to have a set accessor for a ref-valued property are now covered by the grammar. However the usual verbose description of the grammar remains! The term ref-valued rather than ref-returning is used, a term closer to the language of fields. To distingush the two kinds of properties, which do follow different rules, the corresponding and somewhat ugly term non-ref-valued is used (non-ref-returning would be just as ugly). [1] The grammar used by a compiler may be less restrictive to support more descriptive error messages, that is of course an implementation issue! Fixes dotnet#825.

(This was originally in #837.) The grammar has been extended to be as prescriptive as reasonably possible as befits a language specification[1], this means that former statements such as it being illegal to have a set accessor for a ref-valued property are now covered by the grammar. However the usual verbose description of the grammar remains! The term ref-valued rather than ref-returning is used, a term closer to the language of fields. To distingush the two kinds of properties, which do follow different rules, the corresponding and somewhat ugly term non-ref-valued is used (non-ref-returning would be just as ugly). [1] The grammar used by a compiler may be less restrictive to support more descriptive error messages, that is of course an implementation issue! Fixes #825.

Nigel-Ecma · 2023-08-17T19:57:48Z

Rebased as PR #900, so can close this one.

Nigel-Ecma added 2 commits July 6, 2023 16:04

Draft of changes for ref-valued properties

01551e0

Typo: missing semicolon (yes I don't have the grammar checker running…

22ceaf0

… locally at the moment :( )

Nigel-Ecma requested review from jskeet, BillWagner and gafter July 6, 2023 04:36

KalleOlaviNiemitalo suggested changes Jul 6, 2023

View reviewed changes

Nigel-Ecma added 3 commits July 8, 2023 16:33

Yup, a grammer cut'n'paste error – must get my local checker running …

cd13a47

…again!

Fix markdown lint issues (I hope)

1af3f87

KalleOlaviNiemitalo suggested changes Jul 8, 2023

View reviewed changes

standard/classes.md Outdated Show resolved Hide resolved

standard/classes.md Outdated Show resolved Hide resolved

standard/classes.md Outdated Show resolved Hide resolved

KalleOlaviNiemitalo mentioned this pull request Jul 8, 2023

C# 7.3 ArrayTypeMismatchException in ref initialization/assignment #838

Open

Nigel-Ecma added 3 commits July 8, 2023 21:40

Fix typos and other reported small issues (thanks Kalle)

f36e3a3

Correct some x-refs

af69d86

jskeet reviewed Jul 11, 2023

View reviewed changes

Nigel-Ecma mentioned this pull request Jul 11, 2023

C# 7.3 interface_method_declaration etc. grammar does not allow return by reference #826

Closed

Commit comment to follow...

2a9e318

Nigel-Ecma changed the title ~~Draft of changes for ref-valued properties~~ Changes for ref-valued methods, properties, et al. Jul 12, 2023

Nigel-Ecma marked this pull request as ready for review July 12, 2023 04:25

KalleOlaviNiemitalo suggested changes Jul 12, 2023

View reviewed changes

standard/delegates.md Show resolved Hide resolved

standard/classes.md Show resolved Hide resolved

standard/classes.md Show resolved Hide resolved

Nigel-Ecma added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Jul 12, 2023

Nigel-Ecma added this to the C# 7.x milestone Jul 12, 2023

jskeet mentioned this pull request Aug 7, 2023

Reword "statement body" to "block body" for properties #877

Open

Nigel-Ecma mentioned this pull request Aug 10, 2023

Investigate combinations of unsafe & ref #886

Closed

Nigel-Ecma mentioned this pull request Aug 14, 2023

C# 7.3 method_body etc. grammar does not allow => ref #825

Closed

KalleOlaviNiemitalo reviewed Aug 14, 2023

View reviewed changes

standard/statements.md Show resolved Hide resolved

Nigel-Ecma removed the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Aug 15, 2023

Nigel-Ecma requested review from jskeet and KalleOlaviNiemitalo August 15, 2023 22:47

jskeet approved these changes Aug 16, 2023

View reviewed changes

standard/classes.md Show resolved Hide resolved

standard/classes.md Outdated Show resolved Hide resolved

standard/classes.md Outdated Show resolved Hide resolved

standard/classes.md Outdated Show resolved Hide resolved

standard/statements.md Outdated Show resolved Hide resolved

A replicated Note had been pasted into one too many clauses, with a k…

a0e7238

…nock-on effect that a clause list constructed from those with notes was also wrong. Thanks to Jon for spotting this! Also a few typos fixed and minor wording changes.

Nigel-Ecma requested a review from jskeet August 16, 2023 23:02

KalleOlaviNiemitalo reviewed Aug 17, 2023

View reviewed changes

jskeet approved these changes Aug 17, 2023

View reviewed changes

jskeet mentioned this pull request Aug 17, 2023

Local variable initialization is not always equivalent to immediate assignment after declaration #899

Closed

jskeet approved these changes Aug 17, 2023

View reviewed changes

jskeet mentioned this pull request Aug 17, 2023

Changes for ref-valued properties, indexers and methods #900

Merged

KalleOlaviNiemitalo mentioned this pull request Aug 17, 2023

§9.7.2.1: Reference §16.4.12 for clarity #901

Closed

Nigel-Ecma closed this Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for ref-valued methods, properties, et al. #837

Changes for ref-valued methods, properties, et al. #837

Nigel-Ecma commented Jul 6, 2023

jskeet left a comment

Nigel-Ecma commented Jul 12, 2023 •

edited

Loading

jskeet commented Aug 7, 2023

Nigel-Ecma commented Aug 7, 2023 via email

Nigel-Ecma commented Aug 10, 2023

jskeet left a comment

KalleOlaviNiemitalo Aug 17, 2023

jskeet Aug 17, 2023

Nigel-Ecma commented Aug 17, 2023 via email

KalleOlaviNiemitalo commented Aug 17, 2023

KalleOlaviNiemitalo commented Aug 17, 2023

KalleOlaviNiemitalo commented Aug 17, 2023

jskeet left a comment

jskeet Aug 17, 2023

jskeet commented Aug 17, 2023

Nigel-Ecma commented Aug 17, 2023 via email

KalleOlaviNiemitalo commented Aug 17, 2023 •

edited

Loading

jskeet commented Aug 17, 2023

Nigel-Ecma commented Aug 17, 2023

Changes for ref-valued methods, properties, et al. #837

Changes for ref-valued methods, properties, et al. #837

Conversation

Nigel-Ecma commented Jul 6, 2023

jskeet left a comment

Choose a reason for hiding this comment

Nigel-Ecma commented Jul 12, 2023 • edited Loading

jskeet commented Aug 7, 2023

Nigel-Ecma commented Aug 7, 2023 via email

Nigel-Ecma commented Aug 10, 2023

jskeet left a comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo Aug 17, 2023

Choose a reason for hiding this comment

jskeet Aug 17, 2023

Choose a reason for hiding this comment

Nigel-Ecma commented Aug 17, 2023 via email

KalleOlaviNiemitalo commented Aug 17, 2023

KalleOlaviNiemitalo commented Aug 17, 2023

KalleOlaviNiemitalo commented Aug 17, 2023

jskeet left a comment

Choose a reason for hiding this comment

jskeet Aug 17, 2023

Choose a reason for hiding this comment

jskeet commented Aug 17, 2023

Nigel-Ecma commented Aug 17, 2023 via email

KalleOlaviNiemitalo commented Aug 17, 2023 • edited Loading

jskeet commented Aug 17, 2023

Nigel-Ecma commented Aug 17, 2023

Nigel-Ecma commented Jul 12, 2023 •

edited

Loading

KalleOlaviNiemitalo commented Aug 17, 2023 •

edited

Loading