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: Add initializer list to stackalloc #238

Merged
merged 17 commits into from
May 19, 2023

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Mar 15, 2021

I cloned a bunch of text from array initializers, omitting the ability to have multidimensional array allocation and arrays of arrays.

A very long time after I first created this PR, I discovered that other functionality was added earlier than V7.3. Starting in V7.1, stackalloc itself can appear outside of unsafe code, provided it is used in the context of a Span or ReadOnlySpan. As a result, I smuggled into this PR (via its branch) more edits. They make stackalloc a primary operator, and almost all of the semantics of it are moved from unsafe.md to expressions.md.

@RexJaeschke RexJaeschke added this to the C# 7.x milestone Mar 15, 2021
@RexJaeschke RexJaeschke requested a review from BillWagner March 15, 2021 14:19
@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Sep 16, 2021
@RexJaeschke RexJaeschke removed the request for review from BillWagner December 24, 2021 12:21
@RexJaeschke RexJaeschke marked this pull request as draft December 24, 2021 12:21
@RexJaeschke RexJaeschke removed the Review: pending Proposal is available for review label Dec 24, 2021
@RexJaeschke
Copy link
Contributor Author

There is one unresolved issue remaining logged on Teams: The C# standard currently states, "If there is not enough memory available to allocate a block of the given size, a System.StackOverflowException is thrown." However, this is not true. My tests show that the program terminates with the message “Stack overflow.” written to the console, and exit code -1073741571 (which might be arbitrary). My try/catch clauses for System.StackOverflowException and System.Exception are ignored. Any ideas on how I should spec the behavior I observe?

Comment on lines 2408 to 2945
- Otherwise, the result has type `System.Span<T>` and maps to the newly allocated block. Apart from being used as the initializer of a local variable, this result shall be permitted in the following contexts only:
- The right operand of an *assignment_operator* that is not embedded in some larger expression
- The second and third operands in a *conditional_expression*
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this would be invalid then:

using System;
public class C {
    public void M() {
        _ = ((Span<byte>)stackalloc byte[1])[0];
    }
}

Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo Dec 27, 2021

Choose a reason for hiding this comment

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

Oh right, it is invalid in C# 7, but valid in C# 8.

Copy link
Contributor

Choose a reason for hiding this comment

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

The proposed wording would allow the following stackalloc_initializer because it is the second operand in a conditional_expression, yet the compiler rejects it in C# 7.

using System;
public class C {
    public void M() {
        _ = ((Span<byte>)(true ? stackalloc byte[1] : default(Span<byte>)))[0];
    }
}

Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo Dec 27, 2021

Choose a reason for hiding this comment

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

assignment_operator includes compound assignment operators, so the wording would allow this as well:

using System;
public class C {
    public static C operator +(C c, Span<byte> s) { return c; }
    public static void M(C c) {
        c += stackalloc byte[1];
    }
}

But the compiler rejects it in C# 7.

Copy link
Contributor

Choose a reason for hiding this comment

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

If stackalloc_initializer can be used in assignment_operator or conditional_expression, then that needs some changes in the grammar too. So far, stackalloc_initializer is referenced only from local_variable_initializer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, it is invalid in C# 7, but valid in C# 8.

C# 7 seems the wiser choice here, why is it valid in C# 8?

Copy link
Contributor

Choose a reason for hiding this comment

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

The proposed wording would allow the following stackalloc_initializer because it is the second operand in a conditional_expression, yet the compiler rejects it in C# 7.

using System;
public class C {
    public void M() {
        _ = ((Span<byte>)(true ? stackalloc byte[1] : default(Span<byte>)))[0];
    }
}

Was the text meant to imply “The second and third operands in a”, i.e. neither or both branches? One could imagine a reason for that restriction.

Copy link
Member

Choose a reason for hiding this comment

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

Admittedly, my head hurts now.

The previous example fails because the stackalloc is part of a "nested expression". The following variation compiles in C# 7.3:

_ = true ? stackalloc byte[1] : default(Span<byte>);

I'm asking for a definition of "nested expression". It doesn't occur in the standard, and I can't find the term used in LDM notes.

Copy link
Member

Choose a reason for hiding this comment

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

This answer turns out to simplify the PR somewhat. In C# 7.3, a stack_initializer can be used only as a local_variable_initializer. In that context, the result is T*, with an implicit conversation to Span<T> for use in safe contexts.

That means the stackalloc initializer can be used as either or both the 2nd and 3rd operands of a conditional_expression.

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

My try/catch clauses for System.StackOverflowException and System.Exception are ignored.

I'm not sure whether the runtime considers a stack overflow a corrupted state exception as described in HandleProcessCorruptedStateExceptionsAttribute.

@KalleOlaviNiemitalo
Copy link
Contributor

Also dotnet/runtime#40302 for catchable stack overflow.

@KalleOlaviNiemitalo
Copy link
Contributor

The behavior on stack overflow could be implementation-defined, constrained such that if the implementation throws a catchable exception, then it must be a StackOverflowException. An implementation that throws e.g. ThreadAbortException or OutOfMemoryException instead, and lets a C# catch clause catch this exception, would not be conforming. I think requiring implementors to document the behavior would not be too burdensome.

@BillWagner BillWagner force-pushed the Rex-add-initializer-to-stackalloc branch 2 times, most recently from a5323c2 to 45b6118 Compare April 3, 2022 19:01
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.

As @KalleOlaviNiemitalo’s comments demonstrate this needs some work.

Comment on lines 2408 to 2945
- Otherwise, the result has type `System.Span<T>` and maps to the newly allocated block. Apart from being used as the initializer of a local variable, this result shall be permitted in the following contexts only:
- The right operand of an *assignment_operator* that is not embedded in some larger expression
- The second and third operands in a *conditional_expression*
Copy link
Contributor

Choose a reason for hiding this comment

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

The proposed wording would allow the following stackalloc_initializer because it is the second operand in a conditional_expression, yet the compiler rejects it in C# 7.

using System;
public class C {
    public void M() {
        _ = ((Span<byte>)(true ? stackalloc byte[1] : default(Span<byte>)))[0];
    }
}

Was the text meant to imply “The second and third operands in a”, i.e. neither or both branches? One could imagine a reason for that restriction.

@BillWagner BillWagner force-pushed the Rex-add-initializer-to-stackalloc branch from 45b6118 to 2ab269d Compare October 2, 2022 21:43
@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 force-pushed the Rex-add-initializer-to-stackalloc branch from 2ab269d to 65ff771 Compare February 14, 2023 15:20
@BillWagner BillWagner mentioned this pull request Feb 14, 2023
@BillWagner BillWagner force-pushed the Rex-add-initializer-to-stackalloc branch from 65ff771 to ea05665 Compare February 28, 2023 16:45
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

One outstanding question, but I've addressed the other comments.

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
Comment on lines 2408 to 2945
- Otherwise, the result has type `System.Span<T>` and maps to the newly allocated block. Apart from being used as the initializer of a local variable, this result shall be permitted in the following contexts only:
- The right operand of an *assignment_operator* that is not embedded in some larger expression
- The second and third operands in a *conditional_expression*
Copy link
Member

Choose a reason for hiding this comment

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

Admittedly, my head hurts now.

The previous example fails because the stackalloc is part of a "nested expression". The following variation compiles in C# 7.3:

_ = true ? stackalloc byte[1] : default(Span<byte>);

I'm asking for a definition of "nested expression". It doesn't occur in the standard, and I can't find the term used in LDM notes.

@BillWagner
Copy link
Member

@jskeet

This one should be ready for a final review. I don't think we have time to add more to the agenda for tomorrow, but can we add it to the list for review / approval asynchronously between this meeting and the next?

@jskeet
Copy link
Contributor

jskeet commented Mar 16, 2023

Just found this again - will review today/tomorrow.

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.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/portability-issues.md Outdated Show resolved Hide resolved
standard/portability-issues.md Show resolved Hide resolved
standard/unsafe-code.md Outdated Show resolved Hide resolved
standard/unsafe-code.md Outdated Show resolved Hide resolved
standard/unsafe-code.md Show resolved Hide resolved
@@ -2989,6 +2990,77 @@ A *default_value_expression* is a constant expression ([§12.23](expressions.md#
- one of the following value types: `sbyte`, `byte`, `short`, `ushort`, `int`, `uint`, `long`, `ulong`, `char`, `float`, `double`, `decimal`, `bool,`; or
- any enumeration type.

### §stack-allocation Stack allocation
Copy link
Member

Choose a reason for hiding this comment

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

"call stack" needs to be defined in this section.

standard/statements.md Outdated Show resolved Hide resolved
standard/unsafe-code.md Outdated Show resolved Hide resolved
@BillWagner
Copy link
Member

Discussion on 5/17 meeting:

  • stackalloc is an expression, with an optional initializer
  • search for all uses of stackalloc to remove any remaining references to unsafe code.
  • define stack in this PR
  • the result of a stackalloc expression must obey the ref safety rules

@BillWagner BillWagner changed the base branch from draft-v7 to v7-ref-features May 19, 2023 18:12
@BillWagner BillWagner force-pushed the Rex-add-initializer-to-stackalloc branch from 776504c to f7c40c4 Compare May 19, 2023 19:12
@BillWagner BillWagner merged commit 3f4f7f6 into v7-ref-features May 19, 2023
@BillWagner BillWagner deleted the Rex-add-initializer-to-stackalloc branch May 19, 2023 19:15
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
Projects
No open projects
Status: 🔖 Ready
Status: 🏗 In progress
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

5 participants