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: accessing fixed fields without pinning #239

Merged
merged 5 commits into from
Apr 6, 2023

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Mar 15, 2021

While I think my solution allows the newer behavior, it might be too permissive.

See 'The main “challenge discussion in https://github.com/dotnet/csharplang/blob/main/proposals/csharp-7.3/indexing-movable-fixed-fields.md.

@RexJaeschke RexJaeschke added this to the C# 7.x milestone Mar 15, 2021
@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:09
@BillWagner
Copy link
Member

converted to draft: C# 7 feature.

@BillWagner BillWagner force-pushed the Rex-v7-accessing-fixed-fields-without-pinning branch from d48e6b7 to 3c30908 Compare April 3, 2022 19:04
@BillWagner BillWagner force-pushed the Rex-v7-accessing-fixed-fields-without-pinning branch from 3c30908 to 640d97e Compare October 2, 2022 21:45
@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 added the reQUEST Triggers an issue to be imported into Quest. label Dec 8, 2022
@jskeet
Copy link
Contributor

jskeet commented Jan 31, 2023

This isn't an area I'm particularly expert in, but it does look like this is too permissive.

In particular, I think it would imply that the second example in the feature doc should be valid - and it's not. The feature only adds the ability for the first example to become valid without a fixed statement.

I honestly don't know how we'd allow the indexing part at the moment. One for discussion, certainly.

@jskeet jskeet added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Jan 31, 2023
@KalleOlaviNiemitalo
Copy link
Contributor

Current compiler behaviour (SharpLab):

unsafe struct C {
    fixed int buf[9];
    void M() {
        _ = this.buf[6]; // OK   
        _ = (this.buf)[6]; // error, needs fixed statement
    }
}

So this seems to require a new rule that specifically allows the syntax for indexing a fixed-size buffer.

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Jan 31, 2023

Fixed-size buffers cannot be nested

unsafe struct C {
    public fixed int buf[9];
}

unsafe struct D {
    fixed C buf2[5]; // error, C not a fixed-size primitive type
    void M() {
        _ = (this.buf2[1].buf)[3];
    }
}

so the new rule need not state that the movability of this.buf2[1].buf depends on the movability of this.buf2[1] because such a construct cannot be defined anyway.

@BillWagner
Copy link
Member

For discussion in the meeting. I think this works. (I can't make a suggestion on the PR, because the only change is a deletion):

Instead of deleting the single line, modify it as follows:

  • Otherwise, if E is a moveable variable (§22.4), the expression E.I must be one of the following, or a compile-time error occurs:
    • A fixed_pointer_initializer (§22.7)
    • An element_access (§11.710) expression where I is fixed-size buffer (§22.8).

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Feb 1, 2023

@BillWagner problems with that wording:

  • E.I does not match the element_access syntax defined in §11.7.10.1. Instead you could say E.I is the primary_no_array_creation_expression in element_access. It would be even better to reference the pointer_element_access syntax in §22.6.4.
  • The last list item "Otherwise, E references a fixed variable and the result of the expression is a pointer […]" is not reached so you need to specify the semantics of the E.I[E0] expression somewhere else. The type conversion of E0 apparently matches §22.6.4 but I'm not sure whether fixed-size buffer element access should be described as involving a pointer type.

@BillWagner
Copy link
Member

Update based on above comment:

For discussion in the meeting. I think this works. (I can't make a suggestion on the PR, because the only change is a deletion):

Instead of deleting the single line, modify it as follows:

  • Otherwise, if E is a moveable variable (§22.4), the expression E.I must be one of the following, or a compile-time error occurs:
    • A fixed_pointer_initializer (§22.7)
    • An pointer_element_access (§22.6.4) expression where I is fixed-size buffer (§22.8).
  • Otherwise, E references a fixed variable and the result of the expression is a pointer to the first element of the fixed-size buffer member I in E. The result is of type S*, where S is the element type of I, and is classified as a value.

Note that the last bullet item is in the spec now. I added it to the comment because I thought its presence addresses @KalleOlaviNiemitalo 's second bullet point above. (Namely, if E isn't a moveable variable, this bullet applies).

@BillWagner
Copy link
Member

ping @jskeet for Meeting agenda

@jskeet
Copy link
Contributor

jskeet commented Feb 1, 2023

@BillWagner: Not quite sure what you mean - it's got the "meeting: discuss" label, so I'd expect us to just find it that way. Anything else you wanted me to do?

@BillWagner
Copy link
Member

@BillWagner: Not quite sure what you mean - it's got the "meeting: discuss" label, so I'd expect us to just find it that way. Anything else you wanted me to do?

Nope. Just hoping we can resolve this one today.

@BillWagner
Copy link
Member

Results from committee meeting:

We think the latest change is close, but may need refinement.

@MadsTorgersen and @jskeet tagged for review

@KalleOlaviNiemitalo Can you add a comment on the confused emjoi above?

@KalleOlaviNiemitalo
Copy link
Contributor

@BillWagner, E.I (where E is an expression and I is an identifier) cannot match the pointer_element_access syntax either, although it can match the primary_no_array_creation_expression part of that. So the change in the proposed wording does not address the first bullet point in #239 (comment).

§22.6.4 Pointer element access defines the syntax like so:

pointer_element_access
    : primary_no_array_creation_expression '[' expression ']'
    ;

i.e. it would have to end with ']', which E.I cannot do.

RexJaeschke and others added 2 commits February 6, 2023 11:15
Still needs review and refinement.

This commit modifies the bullet that was removed in the original commit.
@BillWagner BillWagner force-pushed the Rex-v7-accessing-fixed-fields-without-pinning branch from 470189f to 21d9810 Compare February 6, 2023 16:15
Reading through the other areas in this clause, classifying the newly allowed behavior as an *element_access* worked more smoothly. It doesn't let other syntax "sneak" into the allowed constructs, and it's intuitive to understand: It's evaluated exactly as the pointer-element-access.
@BillWagner
Copy link
Member

Ping @jskeet @MadsTorgersen

I've updated this to restore the previous language for member access, and add new text for element_access (E[I]), that follows the rules for pointer_element_access. This should be ready for a final review and (hopefully) merging.

@BillWagner BillWagner removed their request for review February 9, 2023 20:24
standard/unsafe-code.md Outdated Show resolved Hide resolved
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.

I think this is too permissive as is, it should only capture the case of E.I[J].

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

Current status as of March 20:

I think I've addressed all remaining comments from Nigel and Jon. I didn't resolve the conversation from Jon above. It would be good to have additional opinions here.

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.

Just to be clear, this is two new features:

  • x.Buffer[index]
  • &x.Buffer

... neither of which would have been valid before? Assuming that's the case, this is fine.

@BillWagner
Copy link
Member

Just to be clear, this is two new features:

  • x.Buffer[index]
  • &x.Buffer

... neither of which would have been valid before? Assuming that's the case, this is fine.

Only the first is valid without fixed. The second returns a pointer, so requires fixed. The two features that now don't require pinning are:

var n = x.Buffer[index];
x.Buffer[inces] = n + 2; // Using the element in the buffer as a variable

@jskeet
Copy link
Contributor

jskeet commented Mar 27, 2023

Just to be clear, this is two new features:

  • x.Buffer[index]
  • &x.Buffer

... neither of which would have been valid before? Assuming that's the case, this is fine.

Only the first is valid without fixed. The second returns a pointer, so requires fixed.

Okay - so leaving aside pinning, was the second valid before now? I'm only mentioning it because it looks like it's new text.

@BillWagner
Copy link
Member

Okay - so leaving aside pinning, was the second valid before now? I'm only mentioning it because it looks like it's new text.

Yes, that's correct.

Copy link
Contributor

@MadsTorgersen MadsTorgersen left a comment

Choose a reason for hiding this comment

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

Looks good!

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.

LGTM

@BillWagner BillWagner merged commit 55fcc8b into draft-v7 Apr 6, 2023
@BillWagner BillWagner deleted the Rex-v7-accessing-fixed-fields-without-pinning branch April 6, 2023 12:58
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 reQUEST Triggers an issue to be imported into Quest. Review: pending Proposal is available for review
Projects
No open projects
Status: 🏗 In progress
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants