-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add rules for ref
safety
#742
Add rules for ref
safety
#742
Conversation
standard/variables.md
Outdated
- A local of `ref struct` type and uninitialized at the point of declaration is *safe-to-return* from the entire enclosing method. Its *safe-to-escape-scope* is the calling method. | ||
- Otherwise the variable's type is a `ref struct` type, and the variable's declaration requires an initializer. The variable's *safe-to-escape-scope* is the same as the *safe-to-escape* of its initializer. | ||
|
||
### §ref-span-safety-parameters Parameter escape scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section doesn't handle a rule needed later in C# 11 (when ref
fields are allowed):
- A
ref
parameter can't be returned byref
as a field in anotherref
argument.
For example:
ref struct S {
ref int refField;
}
void M(ref S p1, ref int p2) {
p1.refField = ref p2;
}
These rules would allow that because both p1
and p2
have ref-safe-to-escape-scope of calling-method. But, doing so could be dangerous: p2
could be a local in the calling method, and p1
could be a static field in the type of the calling method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very shallow review - I'll do a more complete one when I have more mental energy. (I get very confused by all of this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't done a thorough review today but did find a possible typo and, to me at least, a lack of distinction between a variable/location and it’s contents. I know this stuff can be hard to comprehend for many (a student) – variables have names & values, and a name can itself be a value (aka reference, pointer, address – words which may have different semantics depending on the language). We just need to be clear what is escaping etc. using whatever language we decide or it will be confusing to many.
0072dcf
to
6799943
Compare
@jskeet I think this one is now ready. I'd like to have async reviews before our April meeting. If so, I can clean up the remaining ref related PRs for V7. |
Sounds good to me. We can discuss it in today's meeting to try to get anything sync out of the way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of comments that I hope are useful. The whole thing bamboozles me anyway, but I think it's also harder to read because a name change may have been incompletely implemented. (Either that, or we need to define safe-to-escape-scope somewhere...)
I'm hoping that next time I look, it'll make more sense to me :) (I know the gist of what we're trying to achieve, but the terminology somehow just won't stick in my brain.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responding to initial review
Just as an FYI: I'll look at this again tomorrow UK time, and answer questions then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm getting closer to approving :) At some point I think we'll need to just say "Good enough for now" and make any further changes later.
cea185f
to
06d38e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First update to resolve many of the review comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging @jaredpar @gafter @cston @RikkiGibson to look at the diffs for #d8dd5d4523dc88c51ad2dc7888968bb5659ad0de
From an earlier conversation, I'm splitting the rules for ref_safe_to_escape (not referred to as ref_safe_scope, and safe_to_escape.
The reason is that the safe_to_escape rules are only interesting in how they apply to ref struct
types. Any other variable type can be copied (by value) anywhere.
A ref struct
is constrained: It can only be copied within the ref_safe_scope of its referent. Moving these rules into the section on ref structs
makes that distinction more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a single comment but git decided it needed to be a review…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much clearer than before, thanks. I've raised various comments, but I think my overall feeling is:
- We need another pass at consistency of terms and dash-separation vs underscore_separation
- It feels like we're missing a definition of "safe to ref return". It's possible I just missed it though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gafter will have a look at this, and there are a few actions defined in comments, but otherwise, we're ready to merge.
@ BillWagner. Per your request, I just looked over this PR and saw a potential problem in the following:
I'm concerned about defining a new term called block, as we already have a grammar rule by that name. My first thought was to rename your 3 new terms ref-safe-block-scope, ref-safe-function-member-scope, and ref-safe-caller-scope. However, I realize they are rather long, and perhaps a little unwieldy. But it does directly relate them. In the definition of block, "The ref-safe-scope of a local variable is block.", if "block" is intended to be a reference to a grammar rule, it should be italicized. Likewise for "function-member" in "The ref-safe-scope of the fields of a |
I'll propose that we should use these terms. Yes, they are unwieldy, but so are the concepts. These terms directly relate to the concepts, as you say. I've been looking for terms that were both succinct and descriptive, and I've always come up short. I think descriptive wins. I'd like others thoughts before I make those edits. Tagging @gafter @jskeet @MadsTorgersen @Nigel-Ecma |
Agree, go wth the longer descriptive terms |
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>
This include comments, and fixing the formatting after consulting with Rex.
These belong in the PR for ref structs, and in the section on ref structs.
ae222ba
to
5103f42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but I think I'm still fine with it. There may well be nuances that I've missed - the whole thing is quite confusing, but that's just the nature of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but I think I'm still fine with it. There may well be nuances that I've missed - the whole thing is quite confusing, but that's just the nature of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but I think I'm still fine with it. There may well be nuances that I've missed - the whole thing is quite confusing, but that's just the nature of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but I think I'm still fine with it. There may well be nuances that I've missed - the whole thing is quite confusing, but that's just the nature of it.
Co-authored-by: Jon Skeet <skeet@pobox.com>
…pstandard into ref-span-safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm marking this a "request changes" but I'm only suggesting the change and others might manage better wordsmithing.
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>
This needs to be added to dotnet#213 once this PR is merged.
Updates from the 5/17 meeting.
* 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>
This updates the Variables clause with several sections for
ref
safety.This PR is primarily from taking the text from the Span safety proposal and editing into the style for the standard.
In addition, a few rules were added:
out
parameters can't be returned byref
struct
can't returnthis
byref
.in
parameter can't be returned byref
, only byref readonly
.The third rules makes this illegal: