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

Adding fix to process 64 bit integers correctly. #5791

Merged
merged 8 commits into from
Feb 4, 2022

Conversation

davidcho23
Copy link
Contributor

@davidcho23 davidcho23 commented Jan 28, 2022

Currently, the valid 64 bit integer -9,223,372,036,854,775,808 is treated as an invalid integer.

This is because for a negative integer, a unary operation token is created for the minus sign and an integer token is created for the integer. This integer token value is always positive.

When validating the integer value is within the range of a 64 bit integer, -9,223,372,036,854,775,808 fails because its integer token has a value of 9,223,372,036,854,775,808 and is greater than the max 64 bit integer 9,223,372,036,854,775,807.

Many approaches were thought of such as:

  • Looking at what the Roslyn compiler does which is lexing"-123" as two separate tokens then checking for overflow later on. It didn't feel like a good idea to start lexing "-123" as a single token.
  • Handling in the parser would add complexity because you only want to check for the overflow after you've seen the full number.
  • Providing negative and positive range checks in the TypeAssignmentVisitor, which would work if you shortcut in the VisitUnaryOperationSyntax() method to not touch VisitIntegerLiteralSyntax() method. However, this could cause other things to break because this means a type check is forced on the syntax being shortcut.

Finally, the only appropriate way thought of to handle this issue would be to create a dedicated visitor in the emit limitation calculator, which is after type checking. Here, we can choose to shortcut the syntax without affecting other processes such as type checking.

Fixes #5324

@miqm
Copy link
Collaborator

miqm commented Jan 30, 2022

Hello, please follow the contribution guide and include reference to the issue you're fixing. Also, please correct your PR so for tests to pass.

@davidcho23 davidcho23 force-pushed the davidcho23/valid64BitSignedIntegers branch from ce1b7ef to c001488 Compare February 1, 2022 20:00
Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

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

Looks great! Just had a few comments to look over

Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@shenglol shenglol left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

:shipit:

@davidcho23 davidcho23 self-assigned this Feb 4, 2022
@davidcho23 davidcho23 merged commit 1faac71 into main Feb 4, 2022
@davidcho23 davidcho23 deleted the davidcho23/valid64BitSignedIntegers branch February 4, 2022 15:06
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.

Bicep treating ints as signed 32-bit values instead of unsigned 64-bit.
5 participants