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

Support new stackalloc expression locations #1056

Closed

Conversation

RexJaeschke
Copy link
Contributor

No description provided.

@RexJaeschke RexJaeschke added meeting: discuss This issue should be discussed at the next TC49-TG2 meeting type: feature This issue describes a new feature Review: pending Proposal is available for review labels Mar 12, 2024
@RexJaeschke RexJaeschke added this to the C# 8.0 milestone Mar 12, 2024
@RexJaeschke RexJaeschke requested a review from BillWagner March 12, 2024 16:11
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.

This LGTM @RexJaeschke

@jskeet Let's put this on the agenda for the next meeting to approve.

@@ -277,6 +277,7 @@ In an unsafe context, the set of available implicit conversions ([§10.2](conver

- From any *pointer_type* to the type `void*`.
- From the `null` literal ([§6.4.5.7](lexical-structure.md#6457-the-null-literal)) to any *pointer_type*.
- From any *pointer_type* to the type `System.Span<T>`, where `T` is the referent type of *pointer_type*.
Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo Mar 14, 2024

Choose a reason for hiding this comment

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

That implicit conversion seems wrong to me. How would it decide the number of elements in the System.Span<T>?

using System;

public class C {
    public unsafe void M() {
        int* pointer = stackalloc int[3];
        
        Span<int> span;
 
        // error CS0029: Cannot implicitly convert type 'int*' to 'System.Span<int>'
        span = pointer;
    }
}

Instead, you should have an implicit conversion only from a stackalloc_expression to a span type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise error CS0029 here:

Span<int> span = (int*)null;

var deduces a pointer type, though:

public class C {
    public unsafe void M() {
        var a = stackalloc int[3];
        MustBePointer(ref a); // OK
    }
    
    static unsafe void MustBePointer(ref int* p) {}
}

But not when parenthesized:

public class C {
    public unsafe void M() {
        var a = (stackalloc int[3]);
        MustBePointer(ref a); // error CS1503: Argument 1: cannot convert from 'ref System.Span<int>' to 'ref int*'
    }
    
    static unsafe void MustBePointer(ref int* p) {}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This objection looks reasonable - and if we can just change it to an implicit conversion from stackalloc_expression, I think that should be simple. But I don't know the origin of this change, so that could be missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @KalleOlaviNiemitalo, the conversion from a pointer must be wrong.

An implicit conversion from a stackalloc_expression to Span<T> is not required as that is the type of such an expression in safe code, from §12.8.21:

The result of a stackalloc_expression is an instance of type Span<T>, where T is the unmanaged_type

In unsafe code it can be a pointer or a Span<T>, from §23.9:

In an unsafe context if a stackalloc_expression (§12.8.21) occurs as the initializing expression of a local_variable_declaration (§13.6.2), where the local_variable_type is either a pointer type (§23.3) or inferred (var), then the result of the stackalloc_expression is a pointer of type T * to be beginning of the allocated block, where T is the unmanaged_type of the stackalloc_expression.

In all other respects the semantics of local_variable_declarations (§13.6.2) and stackalloc_expressions (§12.8.21) in unsafe contexts follow those defined for safe contexts.

The added conversion wouldn’t make sense even in unsafe code, unless the resulting span had infinite length (and couldn’t be passed to safe code!)

Just delete this added conversion.

@jskeet
Copy link
Contributor

jskeet commented Mar 27, 2024

From discussion, we should reimplement this PR to look more like https://github.com/dotnet/csharplang/blob/main/proposals/csharp-8.0/nested-stackalloc.md - that doesn't talk about a conversion from pointer types, and explains the oddity of the change of type when the expression is parenthesized.

@jskeet
Copy link
Contributor

jskeet commented Mar 27, 2024

Neal is going to make the change - either in this PR or in one which supercedes it.

@jskeet jskeet removed the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Apr 24, 2024
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.

Removal of the 7.3 restrictions look fine.

The addition of the implicit conversion from a pointer to a Span needs to go.

@@ -277,6 +277,7 @@ In an unsafe context, the set of available implicit conversions ([§10.2](conver

- From any *pointer_type* to the type `void*`.
- From the `null` literal ([§6.4.5.7](lexical-structure.md#6457-the-null-literal)) to any *pointer_type*.
- From any *pointer_type* to the type `System.Span<T>`, where `T` is the referent type of *pointer_type*.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @KalleOlaviNiemitalo, the conversion from a pointer must be wrong.

An implicit conversion from a stackalloc_expression to Span<T> is not required as that is the type of such an expression in safe code, from §12.8.21:

The result of a stackalloc_expression is an instance of type Span<T>, where T is the unmanaged_type

In unsafe code it can be a pointer or a Span<T>, from §23.9:

In an unsafe context if a stackalloc_expression (§12.8.21) occurs as the initializing expression of a local_variable_declaration (§13.6.2), where the local_variable_type is either a pointer type (§23.3) or inferred (var), then the result of the stackalloc_expression is a pointer of type T * to be beginning of the allocated block, where T is the unmanaged_type of the stackalloc_expression.

In all other respects the semantics of local_variable_declarations (§13.6.2) and stackalloc_expressions (§12.8.21) in unsafe contexts follow those defined for safe contexts.

The added conversion wouldn’t make sense even in unsafe code, unless the resulting span had infinite length (and couldn’t be passed to safe code!)

Just delete this added conversion.

@gafter
Copy link
Member

gafter commented Nov 20, 2024

The precedence rules (grammar) for stackalloc_expression is no longer correct for C# 8. It is shown in the grammar as a primary_no_array_creation_expression, but we can see from the behavior of the compiler (and by the intent of the purpose of that grammar rule) that is should instead be primary_expression. Specifically, this is not allowed:

        var o = stackalloc nint[3][1];

but this is allowed

        var o = (stackalloc nint[3])[1];

I think it must be this way because stackalloc expressions may contain an initializer.

@gafter
Copy link
Member

gafter commented Nov 20, 2024

I'm working on a replacement PR for this.

@gafter
Copy link
Member

gafter commented Nov 20, 2024

This PR has been revised and incorporated into #1211

@gafter gafter closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: pending Proposal is available for review type: feature This issue describes a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants