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

[stackalloc-init] Improve error tolerance when parsing invalid implicit stackalloc arrays #24914

Merged
merged 3 commits into from
Feb 27, 2018

Conversation

alrz
Copy link
Member

@alrz alrz commented Feb 17, 2018

No description provided.

@alrz alrz requested a review from a team as a code owner February 17, 2018 21:17
@alrz
Copy link
Member Author

alrz commented Feb 17, 2018

The error message should probably be changed. I'll wait for review feedback before changing it.

@CyrusNajmabadi
Copy link
Member

The error message should probably be changed. I'll wait for review feedback before changing it.

Agreed.

@sharwell
Copy link
Member

@jinujoseph Shouldn't this be assigned to the compiler team instead of me?

@jcouv jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Feb 22, 2018
@jcouv jcouv assigned VSadov and unassigned sharwell Feb 22, 2018
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@jcouv
Copy link
Member

jcouv commented Feb 22, 2018

Tagging @VSadov @gafter for review. Thanks

@VSadov
Copy link
Member

VSadov commented Feb 23, 2018

LGTM, but we need a better error. Any good ideas?

Example of where the current error might sound strange:
(there is no type here)

                // (4,28): error CS1575: A stackalloc expression requires [] after type
                //         var x = stackalloc[3,] { 1, 2, 3 };

@jcouv jcouv added this to the 15.7 milestone Feb 23, 2018
@jcouv
Copy link
Member

jcouv commented Feb 23, 2018

I think the error message we use elsewhere is something like "expected ]". Maybe that's better?

@VSadov
Copy link
Member

VSadov commented Feb 23, 2018

@jcouv - just to clarify, are you suggesting simply putting an error on the first unexpected token and produce some standard " expected ']' " ?

Considering that the stackalloc is a fairly advanced feature, this would seem sufficient.

@jcouv
Copy link
Member

jcouv commented Feb 23, 2018

Yes, that's what I was suggesting.

For context, here's the error we just added for implicit array creation (https://github.com/dotnet/roslyn/pull/24706/files#diff-0d8acdaeed6fd4e02198f6e3745d6d2dR286): "Invalid rank specifier: expected ',' or ']'".

For stackalloc (var x = stackalloc[3,] { 1, 2, 3 };), we could use "Invalid rank specificier: expected ']'".

@alrz
Copy link
Member Author

alrz commented Feb 24, 2018

Invalid rank specificier: expected ']'".

to be clear, should this be a new error or we should update the existing one?

@jcouv
Copy link
Member

jcouv commented Feb 26, 2018

@alrz It seems that error CS1575: A stackalloc expression requires [] after type is still useful.
Also, we can't use error CS0178: Invalid rank specifier: expected ',' or ']' since comma doesn't make sense in the context of stackalloc.
So this would be a new error code and message.

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@gafter
Copy link
Member

gafter commented Feb 27, 2018

@VSadov There are two reviews of this PR now.

@alrz
Copy link
Member Author

alrz commented Feb 27, 2018

I should add the new error. will push it asap.

@@ -5256,4 +5256,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureStackAllocInitializer" xml:space="preserve">
<value>stackalloc initializer</value>
</data>
<data name="ERR_InvalidStackAllocArray" xml:space="preserve">
<value>"Invalid rank specificier: expected ']'</value>
Copy link
Member

Choose a reason for hiding this comment

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

specificier [](start = 25, length = 11)

"specificier"??? I think this is a typo.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Still LGTM
Thanks!

@jcouv jcouv merged commit 82f521c into dotnet:features/stackalloc-init Feb 27, 2018
@alrz alrz deleted the stackalloc-parse branch March 12, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants