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

Restrict usage of stack allocation expression #840

Closed
wants to merge 2 commits into from

Conversation

RexJaeschke
Copy link
Contributor

V7 restricts the contexts in which a stackalloc_expression may appear. However, these restrictions were inadvertently lost during the review of that feature's PR.

As it happens, V8 removes those restrictions.

In order for V7 to actually track the V7 implementation, this PR adds those restrictions. The committee can decide if it wishes to do this or to abandon this PR.

@RexJaeschke RexJaeschke added this to the C# 7.x milestone Jul 8, 2023
@RexJaeschke RexJaeschke requested a review from jskeet July 8, 2023 15:48
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.

I haven't validated the changes against any other spec, but they seem reasonable. Will look forward to the reversion PR when we've shipped v7 :)

@@ -3095,6 +3095,11 @@ When a *stackalloc_expression* includes both *expression* and *stackalloc_initia

A stack allocation initializer of the form `stackalloc T[E]` requires `T` to be an *unmanaged_type* and `E` to be an expression implicitly convertible to type `int`. The operator allocates `E * sizeof(T)` bytes from the call stack. The result is a pointer, of type `T*`, to the newly allocated block. For use in safe contexts, a *stackalloc_expression* has an implicit conversion from `T*` to `Span<T>`. As pointer contexts require unsafe code, see [§12.8.21](expressions.md#12821-stack-allocation) for more information.

Apart from being used as the initializer of a local variable, a *stackalloc_expression* shall be used in the following contexts only:

- The right operand of an *assignment_operator* that is not embedded in some larger expression.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you show a valid program in which a stackalloc_expression is the right operand of an assignment_operator?

using System;
class C {
    void M() {
        Span<int> span1 = stackalloc int[1]; // OK, but that's not an *assignment_operator*.

        Span<int> span2;
        span2 = stackalloc int[1]; // error CS8353: A result of a stackalloc expression of type 'Span<int>' cannot be used in this context because it may be exposed outside of the containing method
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Older discussion about stackalloc near #238 (comment), but this error CS8353 thing should be addressed first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assignment doesn't work with a pointer, either:

unsafe class C {
    void M() {
        int* ptr1 = stackalloc int[1]; // OK, but that's not an *assignment_operator*.

        int* ptr2;
        ptr2 = stackalloc int[1]; // error CS8346: Conversion of a stackalloc expression of type 'int' to type 'int*' is not possible.
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the rule should be something like this:

A stackalloc_expression shall be used in the following contexts only:

  • As the initializer of a local variable.
  • As the second and/or third operand of a conditional_expression that is in a context where stackalloc_expression would be permitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

The above would not allow Span<int> span = (Span<int>)stackalloc int[1];, which should be allowed. However, if one just added a third allowed context:

  • As the operand of a cast_expression that is in a context where stackalloc_expression would be permitted.

then that might also allow int* ptr = (int*)stackalloc int[1];, which should not be allowed. I write "might" because I have not checked whether the draft already includes some other text that disallows this.

The impression I get is that a stackalloc_expression preferentially converts to Span<T>, and the conversion to T* is only available in an initializer of a variable of type T*.

Copy link
Contributor

Choose a reason for hiding this comment

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

a valid program in which a stackalloc_expression is the right operand of an assignment_operator

Perhaps like this... although I'm not sure whether this is valid in C# 7 or only C# 8

using System;
public class C {
    public void M() {
        C c;
        c = stackalloc int[1];
    }
    
    public static implicit operator C(Span<int> span) => null;
}

Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo Aug 5, 2023

Choose a reason for hiding this comment

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

Again not sure about C# 7 vs 8:

unsafe struct C {
    static void M() {
        C c1 = stackalloc int[1]; // OK
        C c2 = (C)stackalloc int[1]; // error CS8346
        C c3 = (stackalloc int[1]); // error CS8346
    }
    
    public static implicit operator C(void* ptr) => default;
}

@RexJaeschke RexJaeschke requested a review from BillWagner August 4, 2023 17:14
@RexJaeschke
Copy link
Contributor Author

@Bill, can you please look at Kalle's feedback?

@BillWagner
Copy link
Member

@RexJaeschke @KalleOlaviNiemitalo

I looked at this, and I wonder if the answer could be simpler if we leverage the grammar.

In the current v7 spec, a stackalloc_initializer is one of the productions allowed in a primary_no_array_creation_expression.

In earlier versions, the stackalloc_initializer is one of productions allowed in a local_variable_initializer.

That's the main change in the feature spec for the v8 feature. (Note that the feature spec relies on the v6 spec, where stackalloc is only allowed in the local_variable_initializer_unsafe production. Otherwise, the change is the same.

Would the same change work here?

/cc @Nigel-Ecma for a check of my grammar analysis.

Background: While trying some constructs, the following is legal C# 8, but not legal C# 7.3:

internal class Program
{
    private static void Main(string[] args)
    {
        M(stackalloc int[5]); // error stackalloc not allowed in nested expressions.
    }

    private static void M(Span<int> span)
    {
    }
}

@jskeet
Copy link
Contributor

jskeet commented Aug 7, 2023

@BillWagner: Could you take a look at @KalleOlaviNiemitalo's comments? This area is one I find confusing.

@@ -3095,6 +3095,11 @@ When a *stackalloc_expression* includes both *expression* and *stackalloc_initia

A stack allocation initializer of the form `stackalloc T[E]` requires `T` to be an *unmanaged_type* and `E` to be an expression implicitly convertible to type `int`. The operator allocates `E * sizeof(T)` bytes from the call stack. The result is a pointer, of type `T*`, to the newly allocated block. For use in safe contexts, a *stackalloc_expression* has an implicit conversion from `T*` to `Span<T>`. As pointer contexts require unsafe code, see [§12.8.21](expressions.md#12821-stack-allocation) for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anything specify the Length of the resulting Span<T>? The T* does not have that information.

It might be better to specify stackalloc this way

  1. the result of the stackalloc expression is a Span<T> whose length is exactly E or the number of expressions in the stackalloc element initialiser list, and which describes newly allocated memory (the actual allocation might be larger than this span, e.g. for stack alignment)
  2. If there is an initialiser list, then the expressions are evaluated and the results stored to elements of the span
  3. If the stackalloc expression is an initialiser of a local variable and there is syntactically nothing else in the initialiser (not even parentheses), then the span can be converted to a T* that points to the first element. (Does this require unsafe context at the variable declaration even if the variable does not have 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.

@KalleOlaviNiemitalo – That error probably came from copying text for the unsafe section. Fixed in PR #888. Thanks for spotting this.

@Nigel-Ecma Nigel-Ecma self-requested a review August 10, 2023 02:06
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 others have discussed, the restrictions in 7.3 seem a little more involved. I also noted there was some (not always agreeing) duplication, due I expect to mixing text from the unsafe code section. At this point Github beat me and I couldn’t add the changes as comments so I’ve made PR #885 to address 7.3 and do some de-duplication – it's opening comment is my "review". Continue here or switch to #885 as folk decide.

@BillWagner
Copy link
Member

closed in favor of #885

@BillWagner BillWagner closed this Aug 14, 2023
@RexJaeschke RexJaeschke deleted the RexJaeschke-patch-2 branch August 15, 2023 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants