Skip to content

Fix parser to generate error when array has more than 32 dimensions#16276

Merged
iSazonov merged 2 commits intoPowerShell:masterfrom
daxian-dbw:parser
Oct 29, 2021
Merged

Fix parser to generate error when array has more than 32 dimensions#16276
iSazonov merged 2 commits intoPowerShell:masterfrom
daxian-dbw:parser

Conversation

@daxian-dbw
Copy link
Copy Markdown
Member

PR Summary

Fix #16275

An array cannot have more than 32 dimensions. See the doc of MarkArrayType(int rank)) for details.

The fix is to not create the ArrayTypeName instance when the number of dimensions exceeds 32, but instead, generate a parsing error.

PR Checklist

@ghost ghost assigned iSazonov Oct 19, 2021
Copy link
Copy Markdown
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Looks sensible to me, gives a way clearer error and one that PSRL should be able to communicate to the user visually as well. 👍

Not sure about the test failures though, not super familiar with this area of the code.

@daxian-dbw daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Oct 20, 2021

// The dimensions for an array must be less than or equal to 32.
// Search the doc for 'Type.MakeArrayType(int rank)' for more details.
if (dim > 32)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would we get better UX in PSRL if we move the error report in cycle above?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm. We'd need to be very careful to ensure we continue to consume , tokens until we hit a ] or something unexpected if we want to do that.

This way is a bit simpler to follow, and I think the only UX difference we'd really get would be that maybe we'd be able to pinpoint exactly the point at which there are too many commas, rather than reporting an error for the whole type token.

I don't know if that really gives much to aid the user, though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is no difference in UX in PSRL. PSRL will just change the last token of the user prompt to red color (not always, depending on the user's custom prompt function) in the event of a parsing error.

maybe we'd be able to pinpoint exactly the point at which there are too many commas, rather than reporting an error for the whole type token.

I think it's clearer to give user the exact number of dimensions the array is declared with. Moving the error report in the loop will only detect that it's more than 32 now, but won't get you the exact number of dimensions.

@daxian-dbw
Copy link
Copy Markdown
Member Author

@iSazonov I removed the Backport-7.2.x-Approved label because this is not a regression, and thus doesn't meet the bar for 7.2. We already finalized the commits to be included in 7.2.

@daxian-dbw
Copy link
Copy Markdown
Member Author

@iSazonov I think you can merge this one now.

@iSazonov iSazonov merged commit 979f105 into PowerShell:master Oct 29, 2021
@daxian-dbw daxian-dbw deleted the parser branch October 29, 2021 16:06
@ghost
Copy link
Copy Markdown

ghost commented Dec 16, 2021

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
…owerShell#16276)

An array cannot have more than 32 dimensions. See the doc of MarkArrayType(int rank)) https://docs.microsoft.com/en-us/dotnet/api/system.type.makearraytype?view=net-5.0#System_Type_MakeArrayType_System_Int32_ for details.

The fix is to not create the ArrayTypeName instance when the number of dimensions exceeds 32, but instead, generate a parsing error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parsing an array with too many dimensions throws exception

4 participants