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

Investigate combinations of unsafe & ref #886

Closed
Nigel-Ecma opened this issue Aug 10, 2023 · 5 comments · Fixed by #943
Closed

Investigate combinations of unsafe & ref #886

Nigel-Ecma opened this issue Aug 10, 2023 · 5 comments · Fixed by #943
Assignees
Milestone

Comments

@Nigel-Ecma
Copy link
Contributor

Nigel-Ecma commented Aug 10, 2023

In PR #837 @KalleOlaviNiemitalo raised two comments regarding combinations of unsafe & ref that they believe should be allowed. They are being pushed to this issue to allow the PR to proceed.

This should be allowed:

unsafe delegate ref int D(int* p);

link

This should be allowed:

class C {
    unsafe ref int M(int* p) => ref *p;
}

link


Associated WorkItem - 157262

@KalleOlaviNiemitalo
Copy link
Contributor

This also applies to local_function_declaration.

@jskeet
Copy link
Contributor

jskeet commented Sep 6, 2023

Meeting 2023-09-06: Assigning to Bill to investigate further.

@BillWagner
Copy link
Member

@KalleOlaviNiemitalo

This also applies to local_function_declaration.

Can you clarify what you mean here? Are you saying we need to define rules for local functions in unsafe code, or ref safety in local functions?

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Sep 14, 2023

I mean 3.6.4 Local function declarations currently specifies this grammar:

local_function_declaration
    : local_function_modifier* return_type local_function_header local_function_body
    | ref_kind ref_return_type local_function_header ref_local_function_body
    ;

local_function_header
    : identifier '(' formal_parameter_list? ')'
    | identifier type_parameter_list '(' formal_parameter_list? ')' type_parameter_constraints_clause*
    ;

local_function_modifier
    : 'async'
    | unsafe_modifier   // unsafe code support
    ;

This grammar does not allow using unsafe_modifier together with ref_kind on the return type. So, for example, it does not allow the following, which should be allowed:

class C {
     static int i;
     void M() {
         unsafe ref int L() {
             int* p = null;
             return ref i;
         }         
    }
}

This is only a grammar error and not related to ref-safe-context.

Could be fixed like so:

local_function_declaration
    : local_function_modifier* return_type local_function_header local_function_body
    | ref_local_function_modifier* ref_kind ref_return_type local_function_header ref_local_function_body
    ;

local_function_header
    : identifier '(' formal_parameter_list? ')'
    | identifier type_parameter_list '(' formal_parameter_list? ')' type_parameter_constraints_clause*
    ;

local_function_modifier
    : 'async'
    | ref_local_function_modifier
    ;

ref_local_function_modifier
    : unsafe_modifier   // unsafe code support
    ;

Then in C# 8, 'static' could be added to ref_local_function_modifier and it would also become valid as local_function_modifier.

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Sep 14, 2023

My two comments that were quoted in #886 (comment) likewise apply to grammar only. I had not even considered the ref safety aspects of pointer dereference in unsafe code. Maybe you'll want to leave this issue open until ref safety has been reviewed, but I hope the delegate_declaration, method_declaration, and local_function_declaration grammar can be fixed for C# 7.3 as it seems easy and I don't think it will cause any problems in the semantics because unsafe can already be specified in an outer class or struct declaration.

BillWagner added a commit to BillWagner/csharpstandard that referenced this issue Sep 19, 2023
Add the rules for relaxing ref safety restrictions in an `unsafe` context.

I read through the grammar, and I don't believe any changes are necessary to allow pointer variables with the `ref`, `in`, or `out` modifiers. However, text regarding `out` and `ref` parameters has been modified to include `in` parameters. Text regarding returning pointer types now includes returning pointer types by `ref` or `readonly ref`.

Fixes dotnet#886
BillWagner added a commit to BillWagner/csharpstandard that referenced this issue Sep 20, 2023
`unsafe` and ref returns are allowed for delegates, local functions, and methods. Update the grammar for that part.

Fixes dotnet#886

See dotnet#941 (review)
jskeet pushed a commit that referenced this issue Sep 20, 2023
`unsafe` and ref returns are allowed for delegates, local functions, and methods. Update the grammar for that part.

Fixes #886

See #941 (review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
4 participants