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

C# 7.x: in parameter mode #219

Merged
merged 33 commits into from
May 19, 2023
Merged

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Feb 5, 2021

Almost all the edits support the addition of the in parameter mode.

Support for the v7.2 feature, ref with this (as well as in with this) in an extension method was also added.

Fixes #264
Fixes #296

@RexJaeschke RexJaeschke added this to the C# 7.x milestone Feb 5, 2021
@RexJaeschke
Copy link
Contributor Author

@MadsTorgersen, Bill suggested I loop you in on this to see if one aspect of my resolution reflects what is actually going in with the implementation.

Input, output, and reference arguments must be passed to corresponding input, output, and reference parameters, and that's easy to handle. The new possibility, however, involves passing a value argument, which can now match either a value or an input parameter.

In 12.6.4.2, "Applicable function member," I added text to cover the new possibility along with the following example:

public static void M1(int p1) {}
public static void M1(in int p1) {}
int i = 10;
M1(i);      // M1(int) and M1(in int) are applicable

Then when calling M1, we have to chose from among these two overloads. To do that, I added a new section, 12.6.4.x, "Better argument-passing mode," which says that the value parameter method wins.

Then, in 12.6.2.3, "Run-time evaluation of argument lists," I added text to cover the value-is-transformed-to-input possibility along with the following example:

public static void M1(in int p1) {}
int i = 10;
M1(i);         // transformed to a temporary input argument

How does that sound?

@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Sep 16, 2021
@BillWagner BillWagner marked this pull request as draft February 3, 2022 15:19
@BillWagner BillWagner force-pushed the Rex-v7-in-parameter-mode branch from e0cadd6 to abbc3bc Compare April 3, 2022 02:16
standard/basic-concepts.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/variables.md Outdated Show resolved Hide resolved
standard/variables.md Outdated Show resolved Hide resolved
standard/basic-concepts.md Outdated Show resolved Hide resolved
standard/basic-concepts.md Outdated Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/interfaces.md Show resolved Hide resolved
standard/variables.md Show resolved Hide resolved
@gafter
Copy link
Member

gafter commented May 14, 2022

Please ensure dotnet/csharplang#945 is reflected in this specification.

@BillWagner BillWagner force-pushed the Rex-v7-in-parameter-mode branch from 6179ff2 to 15ea41f Compare October 2, 2022 21:37
@BillWagner BillWagner marked this pull request as ready for review October 5, 2022 22:19
@BillWagner BillWagner self-assigned this Dec 1, 2022
@BillWagner BillWagner mentioned this pull request Feb 14, 2023
@BillWagner BillWagner force-pushed the Rex-v7-in-parameter-mode branch from 15ea41f to bb62644 Compare April 13, 2023 21:37
@BillWagner
Copy link
Member

@BillWagner
Copy link
Member

@jskeet This one is now ready for review and merge.

I've rebased, updated all links, and responded to all remaining feedback.

Note for @gafter I believe dotnet/csharplang#945 is incorporated in this spec. The dates of comments and commits make it hard for me to be sure. Can you review?

standard/basic-concepts.md Show resolved Hide resolved
standard/variables.md Outdated Show resolved Hide resolved
standard/conversions.md Outdated Show resolved Hide resolved
standard/conversions.md Outdated Show resolved Hide resolved
standard/delegates.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
@BillWagner BillWagner added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Apr 19, 2023
standard/expressions.md Outdated Show resolved Hide resolved
standard/delegates.md Outdated Show resolved Hide resolved
standard/delegates.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Result 2023-04-19: nearly there, but not quite ready for "approve, Bill to fix then merge" - the type inference part probably needs review.

standard/basic-concepts.md Show resolved Hide resolved
standard/basic-concepts.md Outdated Show resolved Hide resolved
- For a reference or output parameter, the variable reference is evaluated and the resulting storage location becomes the storage location represented by the parameter in the function member invocation. If the variable reference given as a reference or output parameter is an array element of a *reference_type*, a run-time check is performed to ensure that the element type of the array is identical to the type of the parameter. If this check fails, a `System.ArrayTypeMismatchException` is thrown.
- For a value parameter, if the parameter’s passing mode is value
- the argument expression is evaluated and an implicit conversion ([§10.2](conversions.md#102-implicit-conversions)) to the corresponding parameter type is performed. The resulting value becomes the initial value of the value parameter in the function member invocation.
- otherwise, the parameter’s passing mode is input, in which case, an unnamed *variable_reference* is created with the same type as that of the corresponding parameter. The argument expression is evaluated and an implicit conversion ([§10.2](conversions.md#102-implicit-conversions)) to the corresponding parameter type is performed. The resulting value becomes the initial value of the unnamed *variable_reference* whose storage location becomes the storage location represented by the input parameter in the function member invocation.
Copy link
Contributor

@jskeet jskeet Apr 19, 2023

Choose a reason for hiding this comment

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

Action from 2023-04-19: @BillWagner to log the need for a note about odd aliasing later, and assign to @jskeet.

See #782

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/variables.md Outdated Show resolved Hide resolved
standard/delegates.md Outdated Show resolved Hide resolved
@KalleOlaviNiemitalo
Copy link
Contributor

If I understand correctly, this PR would require all three calls to choose Fun(in int), but that's not what happens at sharplab.io:

using System;

class C {
    static void Fun(in int i) {
        Console.WriteLine("Fun(in int)");
    }

    static void Fun(long l) {
        Console.WriteLine("Fun(long)");
    }

    static void Main() {
        Fun(1); // calls Fun(in int)

        Fun((dynamic)1); // calls Fun(long)

        Fun(i: (dynamic)1); // RuntimeBinderException
    }
}

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented May 5, 2023

Another variant. Here, I think both calls should go to Fun(ref int, long).

using System;

class C {
    static void Fun(in int i, int j) {
        Console.WriteLine("Fun(in int, int)");
    }

    static void Fun(ref int i, long j) {
        Console.WriteLine("Fun(ref int, long)");
    }

    static void Main() {
        int i = 0;

        Fun(ref i, 0); // calls Fun(ref int, long)
        Fun(ref i, (dynamic)0); // calls Fun(in int, int)
        // Fun(in i, (dynamic)0); // error CS8364
    }
}

It seems to me that this mismatch should be considered a bug in the runtime binder implementation, rather than something to be changed in this PR. Whether the bug will ever be fixed is a different matter.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

I think I may be a bit confused by this in general, but I hope the comments are helpful.

standard/expressions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
@BillWagner
Copy link
Member

If I understand correctly, this PR would require all three calls to choose Fun(in int), but that's not what happens at sharplab.io:

using System;

class C {
    static void Fun(in int i) {
        Console.WriteLine("Fun(in int)");
    }

    static void Fun(long l) {
        Console.WriteLine("Fun(long)");
    }

    static void Main() {
        Fun(1); // calls Fun(in int)

        Fun((dynamic)1); // calls Fun(long)

        Fun(i: (dynamic)1); // RuntimeBinderException
    }
}

Great catch! I added that when an argument is dynamic, there must be an identity conversion to the parameter type if passed as an in parameter.

@BillWagner BillWagner force-pushed the Rex-v7-in-parameter-mode branch from 7fa5104 to a48694c Compare May 19, 2023 18:55
@BillWagner BillWagner merged commit 50ff244 into v7-ref-features May 19, 2023
@BillWagner BillWagner deleted the Rex-v7-in-parameter-mode branch May 19, 2023 19:08
BillWagner added a commit that referenced this pull request Jun 7, 2023
* Add rules for `ref` safety (#742)

* first pass at safety rules

This mostly incorporates the feature spec language.

* Add rule for `out` parameters

Out parameters have a ref safe to escape scope of the current method, not the calling method.

* Add readonly rule.

* fix headers

no code fences in headers

* Apply suggestions from code review

Co-authored-by: Jon Skeet <skeet@pobox.com>

* updates from reviews

Updates from code review

* style fix

* respond to feedback

Ref like fields do have storage, but that storage may refer to a variable that is a struct parameter or varialbe.

* respond to feedback

Respond to existing feedback.

* Introduce definitions

Introduce better definitions for the "lifetime" of reference variables. I avoided the term "lifetime' because of its runtime connotation. Instead, I used the "scope" where a "variable declaration" is valid.

From there, next define the safe scopes and the ref safe scopes for different variable classifications.

Finally, I shortened the terms *safe-to-escape-scope* and *ref-safe-to-escape-scope* to "*safe-scope* and *ref-safe-scope*. The text doesn't make it clear why the "escape" term is used, and it doesn't seem to add clarity.

* add examples

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Jon Skeet <skeet@pobox.com>

* address review comments

This addresses most of the comments in the most recent reviews.

The next commit will make an attempt to use a single term for *ref_safe_scope*.

* Remove *safe_scope* rules

Once I push these, I'll add notes about which rules must be added to #601

* Update standard/variables.md

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* Update standard/variables.md

* Respond to review comments.

* Feedback from April meeting

This include comments, and fixing the formatting after consulting with Rex.

* remove ref struct descriptions

These belong in the PR for ref structs, and in the section on ref structs.

* Apply suggestions from code review

Co-authored-by: Jon Skeet <skeet@pobox.com>

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* remove missing xref

This needs to be added to #213 once this PR is merged.

* Updates from 5/17 meeting.

Updates from the 5/17 meeting.

* fix build warning

---------

Co-authored-by: Jon Skeet <skeet@pobox.com>
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* C# 7.x: ref struct (#601)

* Split out `ref struct` from #33.

* Add ref safety rules as they apply to ref structs

Declare that `ref structs` have a *ref_safe_scope* that matches their initializing expressions. Restrict copying of a `ref struct` (by value) to its *ref_safe_scope*. Then, define what the *ref_safe_scope* is depending on the initializing expression.

* forgot to finish one sentence.

* Add note on iterators and async methods

The previous normative language was

* respond to review feedback.

* Respond to meeting feedback, part 1

Respond to all meeting feedback *except* updating the rules on *safe_to_escape*.

That's coming in the next commit.

* incorporate safe rules.

Pull all safety rules related to safe-scope from PR on ref variables
into the section on ref structs.

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Jon Skeet <jonskeet@google.com>

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* respond to feedback.

* Update per 5/17 committee meeting.

* update definition

* found one more comment to address

---------

Co-authored-by: Neal Gafter <nmgafter@fb.com>
Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
Co-authored-by: Jon Skeet <jonskeet@google.com>
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* C# 7.x: ref locals and returns (#213)

* Update expressions.md

* Update statements.md

* Update classes.md

* Update delegates.md

* Relocate ('ref' 'readonly'?)? to local_variable_declaration

I had this grammar extension in the wrong place

* Add support for ref readonly iteration variables to foreach

* Minor tweak to v7 spec for ? ref : ref

* build fixes

* fix merge error

* one more build issue

* light editing

* fix section references

* fix link errors

* edit to address feedback, link to ref safety rules.

* fix markdown lint error

* Apply suggestions from code review

Co-authored-by: Jon Skeet <jonskeet@google.com>

* respond to feedback

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Jon Skeet <jonskeet@google.com>

* Update per April meeting feedback.

* formatting while checking grammar

* updates from 5/17 committee meeting.

* remove blank line.

* fix warnings

---------

Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
Co-authored-by: Jon Skeet <jonskeet@google.com>

* C# 7.x: in parameter mode (#219)

* Update basic-concepts.md

* Update variables.md

* Update conversions.md

* Update structs.md

* Update interfaces.md

* Update delegates.md

* Update unsafe-code.md

* Update documentation-comments.md

* Update classes.md

* Update expressions.md

* include support for local functions

* fix merge tag

* fix build warnings

* build fixes

* build fixes, round 1

* build fixes, part 2

* one last link fix

* light edits based on earlier feedback.

* respond to remaining feedbac,.

* one final edit....

* clarification on `ref` extension methods

* Apply suggestions from code review

Co-authored-by: Jon Skeet <jonskeet@google.com>

* respond to feedback

* Update standard/expressions.md

Co-authored-by: Jon Skeet <jonskeet@google.com>

* Update standard/expressions.md

* Apply suggestions from code review

Co-authored-by: Jon Skeet <jonskeet@google.com>

* Update per April meeting notes.

* respond to feedback.

* Exclude dynamic implicit conversions

A dynamic expression can't be passed as an `in` parameter if an implicit conversion is required.

* Clarify restrictions on `in` parameters

Dynamically bound expressions can't use the `in` modifier.

* Apply suggestions from code review

Co-authored-by: Neal Gafter <neal@gafter.com>

* Updates from 5/17 committee meeting.

* fix warning

---------

Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
Co-authored-by: Jon Skeet <jonskeet@google.com>
Co-authored-by: Neal Gafter <neal@gafter.com>

* C# 7.x: Add initializer list to `stackalloc` (#238)

* Update unsafe-code.md

* Update unsafe-code.md

* Move most of stackalloc spec from unsafe to here

* Impact of moving most of stackalloc spec from unsafe to expressions

* Moved most of stackalloc spec to expressions

* Fix links to new stackalloc spec location

* Add Span & ReadOnlySpan types

* fix build issues

* address feedback

* Stack initializers are only allowed as local variable initializers

This clarifies and simplifies some of the language for this PR.

* fix markdown lint issue

* respond to feedback.

* fix build issues

* one more round of build issues

* respond to feedback.

* decisions from 5/17 meeting.

* add safe context rules.

---------

Co-authored-by: Bill Wagner <wiwagn@microsoft.com>

* fix build warnings

* Apply suggestions from code review

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* fix references

* add closing backticks

One grammar rule was missing the closing backticks.

This caused several build errors.

* respond to feedback through clause 15 (classes)

This commit responds to feedback with the 👍 emoji through clause 15.

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* rearrange conditional specification

* Update standard/expressions.md

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* Update standard/variables.md

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* edits based on feedback.

* Update standard/expressions.md

Co-authored-by: Jon Skeet <jonskeet@google.com>

* respond to feedback comments

* Apply suggestions from code review

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* more feedback

* Update standard/classes.md

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* Update standard/classes.md

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* Update standard/expressions.md

* Update standard/expressions.md

* Update standard/expressions.md

Co-authored-by: Bill Wagner <wiwagn@microsoft.com>

* Update standard/variables.md

* edits during the meeting.

* Update standard/expressions.md

Co-authored-by: KalleOlaviNiemitalo <kon@iki.fi>

* edits, part 1

* respond to meeting discussion and feedback

Address all remaining conversations for this PR.

Addresses feedback from 06/05/2023 ECMA committee meeting.

---------

Co-authored-by: Jon Skeet <skeet@pobox.com>
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>
Co-authored-by: Neal Gafter <neal@gafter.com>
Co-authored-by: Neal Gafter <nmgafter@fb.com>
Co-authored-by: Jon Skeet <jonskeet@google.com>
Co-authored-by: Rex Jaeschke <rex@RexJaeschke.com>
Co-authored-by: KalleOlaviNiemitalo <kon@iki.fi>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting Review: pending Proposal is available for review
Projects
No open projects
Status: 🏗 In progress
Status: 🔖 Ready
Development

Successfully merging this pull request may close these issues.

ref-safe-to-escape rule for in parameter Information about "in" function arguments missing
6 participants