Permit stackalloc in nested contexts. #28969

gafter · 2018-07-31T17:04:11Z

Permit stackalloc in nested contexts.
Fixes #26759
Additional tests are needed before integration into a product branch;
tracked by #28968

The championed feature is dotnet/csharplang#1412

The display for a stackalloc expression should be its type if it has one. Fixes dotnet#26759 Additional tests are needed before integration into a product branch; see dotnet#28968

jcouv · 2018-07-31T17:25:39Z

Please open a championed issue with a proposal doc, if there isn't one already. Thanks #Closed

gafter · 2018-07-31T17:35:56Z

The championed feature is dotnet/csharplang#1412

There is a draft spec in that issue. #Closed

jcouv · 2018-07-31T17:50:39Z

Thanks. If I may bother you with one more administrative task: could you add this feature to the features status doc (master branch)? #Closed

gafter · 2018-07-31T17:58:19Z

@jcouv Please see #28971 #Closed

jcouv · 2018-08-03T23:57:04Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

@@ -5524,7 +5527,7 @@ private bool IsUsingAliasInScope(string name)
                        {
                            // Can't dot into the null literal or stackalloc expressions.
                            if ((boundLeft.Kind == BoundKind.Literal && ((BoundLiteral)boundLeft).ConstantValueOpt == ConstantValue.Null) ||
-                                boundLeft.Kind == BoundKind.StackAllocArrayCreation)
+                                boundLeft.Kind == BoundKind.StackAllocArrayCreation && MessageID.IDS_FeatureNestedStackalloc.RequiredVersion() > Compilation.LanguageVersion)


IDS_FeatureNestedStackalloc [](start = 97, length = 27)

Seems like we should be reporting a language version error in some cases here. #Closed

As you can see from CannotDotIntoStackAllocExpression in StackAllocInitializerTests.cs, there would be a language version error here on the stackalloc expression itself, so there doesn't need to be one on the member access expression. This test just ensures our error recovery is the same in C# 7.3, as the member access is permitted in C# 8.

In reply to: 208325668 [](ancestors = 208325668,207691295)

Further testing has revealed that this was dead code in C# 7.3 because of the error on the stackalloc expression. I'll just remove the special case for stackalloc here.

In reply to: 208329378 [](ancestors = 208329378,208325668,207691295)

jcouv · 2018-08-04T00:01:11Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeExtensions.cs

+                return false;
+            }
+
+            return


return [](start = 12, length = 6)

Could you explain the tightening here? Is it observable? #Closed

It isn't observable in code that was previously valid. Now that we are making it valid, we need the tightening to narrow the places where we consider stackalloc to be a pointer to just those places where it was previously valid and a pointer.

In reply to: 207691737 [](ancestors = 207691737)

jcouv · 2018-08-04T00:07:30Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

@@ -759,7 +759,9 @@ protected BoundExpression BindInferredVariableInitializer(DiagnosticBag diagnost

            BoundExpression expression = BindValue(initializer, diagnostics, valueKind);

-            if (expression is BoundStackAllocArrayCreation boundStackAlloc)
+            if (expression is BoundStackAllocArrayCreation boundStackAlloc &&


if [](start = 12, length = 2)

I didn't understand the purpose of this change. #Closed

Now that we're relaxing the locations where a stackalloc expression can appear, we need to give it a natural type of a pointer type only in the syntactic context where that is supposed to work. The old code did that even in erroneous locations because it didn't matter. The first test ensures we are in a local variable initializer. The second test ensures that it is not enclosed in some kind of no-op expression like a parenthesized expression, checked, etc, which removes it from the context where it should be treated as a pointer.

In reply to: 207692207 [](ancestors = 207692207)

gafter · 2018-08-10T03:07:51Z

@dotnet/roslyn-compiler May I please have a couple of reviews of this initial implementation of a tiny language feature for C# 8.0? #Closed

cston · 2018-08-10T15:04:00Z

src/Compilers/CSharp/Portable/BoundTree/BoundSpillSequence.cs

+                builder.Add(new BoundExpressionStatement(expression.Syntax, expression, expression.HasErrors));
+            }
+
+            return builder.ToImmutableAndFree();


return builder.ToImmutableAndFree(); [](start = 12, length = 36)

Consider using SelectAsArray:

return expressions.SelectAsArray(expression => new BoundExpression(...)); ``` #Resolved

cston · 2018-08-10T15:24:20Z

    public void InvalidPositionForStackAllocSpan()

Should the test be renamed?

Or perhaps leave the test name but add a C#7.3 compilation in addition to the C#8.0 compilation. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StackAllocInitializerTests.cs:1579 in 303f32e. [](commit_id = 303f32e, deletion_comment = False)

cston · 2018-08-10T15:40:43Z

src/Compilers/CSharp/Test/Semantic/Semantics/UnsafeTests.cs

-            CreateCompilation(text, options: TestOptions.UnsafeReleaseDll).VerifyDiagnostics(
-                // (4,14): error CS1525: Invalid expression term 'stackalloc'
+            CreateCompilationWithMscorlibAndSpan(text, options: TestOptions.UnsafeReleaseDll).VerifyDiagnostics(
+                // (4,14): error CS8346: Conversion of a stackalloc expression of type 'int' to type 'int*' is not possible.


CS8346: Conversion of a stackalloc expression of type 'int' to type 'int*' is not possible. [](start = 33, length = 91)

The error seems confusing to me since the same conversion is possible for a local declaration. Should this be a specific error that stackalloc is not allowed in this context? #Resolved

But it is allowed in this context. It just always gives you a Span<int>.

Local declarations are treated specially, and even though the looks syntactically the same as a local declaration, it isn't.

In reply to: 209301880 [](ancestors = 209301880)

gafter · 2018-08-10T17:40:35Z

@cston I've added a new iteration that I believe responds to your comments. Do you have any additional comments?

gafter · 2018-08-13T18:11:58Z

@dotnet/roslyn-compiler May I please have a second review?

jcouv · 2018-08-13T21:56:09Z

I'm looking.

jcouv · 2018-08-13T23:47:32Z

src/Compilers/CSharp/Portable/BoundTree/BoundSpillSequence.cs

+            ImmutableArray<BoundExpression> sideEffects,
+            BoundExpression value,
+            TypeSymbol type,
+            bool hasErrors = false)


hasErrors [](start = 17, length = 9)

I didn't see any callers pass this parameter. Would it make sense to remove this parameter? ~~or just inline the hasErrors: false into two users of this constructor?~~

I'd rather leave this in its most general form. The only purpose of this method is to adapt callers who want to pass an array of expression side-effects. It is not intended to be tailored to particular clients.

jcouv

LGTM Thanks (iteration 3)

* Permit stackalloc in nested contexts. (#28969) Fixes #26759

Permit stackalloc in nested contexts.

ac47ff6

The display for a stackalloc expression should be its type if it has one. Fixes dotnet#26759 Additional tests are needed before integration into a product branch; see dotnet#28968

gafter added Area-Compilers Feature Request labels Jul 31, 2018

gafter added this to the 16.0 milestone Jul 31, 2018

gafter self-assigned this Jul 31, 2018

gafter requested a review from a team as a code owner July 31, 2018 17:04

jcouv reviewed Aug 3, 2018

View reviewed changes

jcouv reviewed Aug 4, 2018

View reviewed changes

Remove dead code.

303f32e

jcouv self-assigned this Aug 8, 2018

gafter closed this Aug 9, 2018

gafter reopened this Aug 9, 2018

gafter requested a review from a team August 10, 2018 03:08

cston reviewed Aug 10, 2018

View reviewed changes

cston approved these changes Aug 10, 2018

View reviewed changes

Adjust tests per review comments.

1815f9d

cston approved these changes Aug 10, 2018

View reviewed changes

jcouv reviewed Aug 13, 2018

View reviewed changes

jcouv approved these changes Aug 13, 2018

View reviewed changes

gafter merged commit 4029225 into dotnet:features/nested-stackalloc Aug 14, 2018

This was referenced Aug 14, 2018

Permit stackalloc in nested expressions #26759

Closed

stackalloc expressions have limited syntax #22046

Closed

gafter pushed a commit that referenced this pull request May 1, 2019

merge features/nested-stackalloc (#35385)

7517f5d

* Permit stackalloc in nested contexts. (#28969) Fixes #26759

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit stackalloc in nested contexts. #28969

Permit stackalloc in nested contexts. #28969

gafter commented Jul 31, 2018 •

edited by jcouv

Loading

jcouv commented Jul 31, 2018 •

edited

Loading

gafter commented Jul 31, 2018 •

edited by jcouv

Loading

jcouv commented Jul 31, 2018 •

edited

Loading

gafter commented Jul 31, 2018 •

edited by jcouv

Loading

jcouv Aug 3, 2018 •

edited

Loading

gafter Aug 7, 2018

gafter Aug 7, 2018

jcouv Aug 4, 2018 •

edited

Loading

gafter Aug 7, 2018

jcouv Aug 4, 2018 •

edited

Loading

gafter Aug 7, 2018

gafter commented Aug 10, 2018 •

edited by jcouv

Loading

cston Aug 10, 2018 •

edited by gafter

Loading

cston commented Aug 10, 2018 •

edited by gafter

Loading

cston Aug 10, 2018 •

edited by gafter

Loading

gafter Aug 10, 2018

gafter commented Aug 10, 2018

gafter commented Aug 13, 2018

jcouv commented Aug 13, 2018

jcouv Aug 13, 2018 •

edited

Loading

gafter Aug 14, 2018

jcouv left a comment

Permit stackalloc in nested contexts. #28969

Permit stackalloc in nested contexts. #28969

Conversation

gafter commented Jul 31, 2018 • edited by jcouv Loading

jcouv commented Jul 31, 2018 • edited Loading

gafter commented Jul 31, 2018 • edited by jcouv Loading

jcouv commented Jul 31, 2018 • edited Loading

gafter commented Jul 31, 2018 • edited by jcouv Loading

jcouv Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Aug 7, 2018

Choose a reason for hiding this comment

gafter Aug 7, 2018

Choose a reason for hiding this comment

jcouv Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Aug 7, 2018

Choose a reason for hiding this comment

jcouv Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Aug 7, 2018

Choose a reason for hiding this comment

gafter commented Aug 10, 2018 • edited by jcouv Loading

cston Aug 10, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston commented Aug 10, 2018 • edited by gafter Loading

cston Aug 10, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Aug 10, 2018

Choose a reason for hiding this comment

gafter commented Aug 10, 2018

gafter commented Aug 13, 2018

jcouv commented Aug 13, 2018

jcouv Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Aug 14, 2018

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

gafter commented Jul 31, 2018 •

edited by jcouv

Loading

jcouv commented Jul 31, 2018 •

edited

Loading

gafter commented Jul 31, 2018 •

edited by jcouv

Loading

jcouv commented Jul 31, 2018 •

edited

Loading

gafter commented Jul 31, 2018 •

edited by jcouv

Loading

jcouv Aug 3, 2018 •

edited

Loading

jcouv Aug 4, 2018 •

edited

Loading

jcouv Aug 4, 2018 •

edited

Loading

gafter commented Aug 10, 2018 •

edited by jcouv

Loading

cston Aug 10, 2018 •

edited by gafter

Loading

cston commented Aug 10, 2018 •

edited by gafter

Loading

cston Aug 10, 2018 •

edited by gafter

Loading

jcouv Aug 13, 2018 •

edited

Loading