starlark: fix bug in int(string, base=int) #344
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, when int was called with an explicit base,
it would report an error if the digit string starts
with a base prefix for a different base, such as int("0b101", 16).
Now, it uses the base prefix only if it matches the requested
base, so the example above would return 0x0b101, as would
int("0x0b101", 16).
The int(string, int) case has been split out for clarity.
Update doc.
Fixes #337
Change-Id: I74244d2039c7a14a5cc959c92f8cf1fa78ba448a
jayconrod
approved these changes
Jan 25, 2021
bazel-io
pushed a commit
to bazelbuild/bazel
that referenced
this pull request
Feb 3, 2021
Previously, when int was called with an explicit base,
it would report an error if the digit string starts
with a base prefix for a different base, such as int("0b101", 16).
Now, it uses the base prefix only if it matches the requested
base, so the example above would return 0x0b101, as would
int("0x0b101", 16).
See github.com/google/starlark-go/pull/344
PiperOrigin-RevId: 355479386
mahmoudimus
added a commit
to verygoodsecurity/starlarky
that referenced
this pull request
Feb 10, 2021
…21089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573) Changelog for (Master)[bazelbuild/bazel@be96ade#diff-37cd73cae488b2421089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573]: - Builtins injection: Rename _internal to _builtins and add functionality … This object is used in @_builtins .bzl files but is not accessible to user code. The `toplevel` and `native` fields give access to the *native* (pre-injected) values of symbols whose post-injected values are available to user .bzl files. For instance, -`_builtins.toplevel.CcInfo` in @_builtins code gives the original CcInfo definition from the Java code, even if `CcInfo` in a regular .bzl file refers to an injected value. (To avoid ambiguity, `CcInfo` itself is not a valid top-level symbol for @_builtins .bzl files.) The `internal` field contains any value registered via ConfiguredRuleClassProvider.Builder#addStarlarkBuiltinsInternal(). The `getFlag()` method can retrieve the values of StarlarkSemantics flags. Because of how flags are stored, it requires that a default value be given. - Starlark: better errors on integer overflow … Before: ``` >> [1] * (1 << 30) * (1 << 5) Exception in thread "main" net.starlark.java.eval.Starlark$UncheckedEvalException: java.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 1073741824 out of bounds for object array[0] (Starlark stack: [<toplevel>@<stdin>:1:1]) net.starlark.java.eval.Starlark.fastcall(Starlark.java:621) net.starlark.java.eval.Starlark.execFileProgram(Starlark.java:892) ... exit 1 ``` Now: ``` >> [1] * (1 << 30) * (1 << 5) Traceback (most recent call last): File "<stdin>", line 1, column 21, in <toplevel> Error: got 34359738368 for repeat, want value in signed 32-bit range ``` - starlark: int: ignore base prefix unless matches explicit base … Previously, when int was called with an explicit base, it would report an error if the digit string starts with a base prefix for a different base, such as int("0b101", 16). Now, it uses the base prefix only if it matches the requested base, so the example above would return 0x0b101, as would int("0x0b101", 16). See github.com/google/starlark-go/pull/344 - starlark: delete StarlarkFile.parseWithPrelude … It was an obstacle to interpreter optimizations, as it caused a single StarlarkFile to contain statements whose locations come from different files. The previous workspace logic used parseWithPrelude to concatenate all the statements of the WORKSPACE files (prefix + main + suffix) and then split them into chunks, ignoring---crucially, several days work revealed---file boundaries. The splitChunks logic now achieves the same effect by representing chunks as lists of nonempty partial files, and calling execute() for each partial file in the chunk. The chunk splitting tests have been rewritten, clarified, and expanded to exercise the chunk-spans-file-boundaries case. Many thanks to Jon Brandvein for hours of help trying to figure out what the workspace logic does. It is not our team's finest work. Also: - minor consequent simplifications to parser and lexer. - narrow the scope of various try blocks (sorry, messy diff). - starlark: remove redundant pattern validity check in Printer … - starlark: support %x, %X, and %o conversions in 'string % number' … Also, improve operand type error message to show type, not value. See github.com/bazelbuild/starlark/pull/154 for spec change. - Starlark: StarlarkInt.{floordiv,mod} now work with 64-bit integers … * No special case for 32-bit integers, division is slow anyway * Switch `mod` to use `Math.floorMod` for simplicity/safety Closes #12667. - starlark: delete deprecated EvalException(Location) constructor … ...as there are, at long last, no further uses within Bazel. Also, clarify doc comments regarding EvalException.callstack. - bazel analysis: preparatory cleanup to SRCTU error reporting … This change causes the error handling logic in StarlarkRuleConfiguredTargetUtil (SRCTU) to avoid the EvalException(Location) constructor. Instead, the auxiliary location, if any, of provider instantiation is simply prepended to the error message (see the infoError function) in anticipation of being printed after a newline. For example: ERROR p/BUILD:1:1:\n foo.bzl:1:2: <provider-related message> createTarget uses a distinct exception, BadImplementationFunction, for errors that arise not from the Starlark implementation function but from the post-processing of the providers. A follow-up change will replace this exception by directly reporting events to the handler, thus permitting both higher quality structured errors capable of reporting more than one relevant location, and multiple errors in a single pass. However, that change is too tricky to accomplish in this CL. Also: - move "advertised provider" and "orphaned file" checks into createTarget. - add comments to document this particularly painful code. - delete EvalException.getDeprecatedLocation. - Starlark: long StarlarkInt multiply without BigInteger … Use Hacker's Delight 8-2. Closes #12643. - bazel packages: use EventHandler not EvalException in .bzl "export" o… … …peration This allows us to delete one of the deprecated EvalException(Location,...) constructors. Similar follow-up changes (events not exceptions) will be required to eliminate the remaining such constructor. As a bonus, this approach allows multiple errors to be reported at once.
mahmoudimus
added a commit
to verygoodsecurity/starlarky
that referenced
this pull request
Feb 10, 2021
…21089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573) Changelog for (Master)[bazelbuild/bazel@be96ade#diff-37cd73cae488b2421089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573]: - Builtins injection: Rename _internal to _builtins and add functionality … This object is used in @_builtins .bzl files but is not accessible to user code. The `toplevel` and `native` fields give access to the *native* (pre-injected) values of symbols whose post-injected values are available to user .bzl files. For instance, -`_builtins.toplevel.CcInfo` in @_builtins code gives the original CcInfo definition from the Java code, even if `CcInfo` in a regular .bzl file refers to an injected value. (To avoid ambiguity, `CcInfo` itself is not a valid top-level symbol for @_builtins .bzl files.) The `internal` field contains any value registered via ConfiguredRuleClassProvider.Builder#addStarlarkBuiltinsInternal(). The `getFlag()` method can retrieve the values of StarlarkSemantics flags. Because of how flags are stored, it requires that a default value be given. - Starlark: better errors on integer overflow … Before: ``` >> [1] * (1 << 30) * (1 << 5) Exception in thread "main" net.starlark.java.eval.Starlark$UncheckedEvalException: java.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 1073741824 out of bounds for object array[0] (Starlark stack: [<toplevel>@<stdin>:1:1]) net.starlark.java.eval.Starlark.fastcall(Starlark.java:621) net.starlark.java.eval.Starlark.execFileProgram(Starlark.java:892) ... exit 1 ``` Now: ``` >> [1] * (1 << 30) * (1 << 5) Traceback (most recent call last): File "<stdin>", line 1, column 21, in <toplevel> Error: got 34359738368 for repeat, want value in signed 32-bit range ``` - starlark: int: ignore base prefix unless matches explicit base … Previously, when int was called with an explicit base, it would report an error if the digit string starts with a base prefix for a different base, such as int("0b101", 16). Now, it uses the base prefix only if it matches the requested base, so the example above would return 0x0b101, as would int("0x0b101", 16). See github.com/google/starlark-go/pull/344 - starlark: delete StarlarkFile.parseWithPrelude … It was an obstacle to interpreter optimizations, as it caused a single StarlarkFile to contain statements whose locations come from different files. The previous workspace logic used parseWithPrelude to concatenate all the statements of the WORKSPACE files (prefix + main + suffix) and then split them into chunks, ignoring---crucially, several days work revealed---file boundaries. The splitChunks logic now achieves the same effect by representing chunks as lists of nonempty partial files, and calling execute() for each partial file in the chunk. The chunk splitting tests have been rewritten, clarified, and expanded to exercise the chunk-spans-file-boundaries case. Many thanks to Jon Brandvein for hours of help trying to figure out what the workspace logic does. It is not our team's finest work. Also: - minor consequent simplifications to parser and lexer. - narrow the scope of various try blocks (sorry, messy diff). - starlark: remove redundant pattern validity check in Printer … - starlark: support %x, %X, and %o conversions in 'string % number' … Also, improve operand type error message to show type, not value. See github.com/bazelbuild/starlark/pull/154 for spec change. - Starlark: StarlarkInt.{floordiv,mod} now work with 64-bit integers … * No special case for 32-bit integers, division is slow anyway * Switch `mod` to use `Math.floorMod` for simplicity/safety Closes #12667. - starlark: delete deprecated EvalException(Location) constructor … ...as there are, at long last, no further uses within Bazel. Also, clarify doc comments regarding EvalException.callstack. - bazel analysis: preparatory cleanup to SRCTU error reporting … This change causes the error handling logic in StarlarkRuleConfiguredTargetUtil (SRCTU) to avoid the EvalException(Location) constructor. Instead, the auxiliary location, if any, of provider instantiation is simply prepended to the error message (see the infoError function) in anticipation of being printed after a newline. For example: ERROR p/BUILD:1:1:\n foo.bzl:1:2: <provider-related message> createTarget uses a distinct exception, BadImplementationFunction, for errors that arise not from the Starlark implementation function but from the post-processing of the providers. A follow-up change will replace this exception by directly reporting events to the handler, thus permitting both higher quality structured errors capable of reporting more than one relevant location, and multiple errors in a single pass. However, that change is too tricky to accomplish in this CL. Also: - move "advertised provider" and "orphaned file" checks into createTarget. - add comments to document this particularly painful code. - delete EvalException.getDeprecatedLocation. - Starlark: long StarlarkInt multiply without BigInteger … Use Hacker's Delight 8-2. Closes #12643. - bazel packages: use EventHandler not EvalException in .bzl "export" o… … …peration This allows us to delete one of the deprecated EvalException(Location,...) constructors. Similar follow-up changes (events not exceptions) will be required to eliminate the remaining such constructor. As a bonus, this approach allows multiple errors to be reported at once.
mahmoudimus
added a commit
to verygoodsecurity/starlarky
that referenced
this pull request
Mar 30, 2021
…21089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573) Changelog for (Master)[bazelbuild/bazel@be96ade#diff-37cd73cae488b2421089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573]: - Builtins injection: Rename _internal to _builtins and add functionality … This object is used in @_builtins .bzl files but is not accessible to user code. The `toplevel` and `native` fields give access to the *native* (pre-injected) values of symbols whose post-injected values are available to user .bzl files. For instance, -`_builtins.toplevel.CcInfo` in @_builtins code gives the original CcInfo definition from the Java code, even if `CcInfo` in a regular .bzl file refers to an injected value. (To avoid ambiguity, `CcInfo` itself is not a valid top-level symbol for @_builtins .bzl files.) The `internal` field contains any value registered via ConfiguredRuleClassProvider.Builder#addStarlarkBuiltinsInternal(). The `getFlag()` method can retrieve the values of StarlarkSemantics flags. Because of how flags are stored, it requires that a default value be given. - Starlark: better errors on integer overflow … Before: ``` >> [1] * (1 << 30) * (1 << 5) Exception in thread "main" net.starlark.java.eval.Starlark$UncheckedEvalException: java.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 1073741824 out of bounds for object array[0] (Starlark stack: [<toplevel>@<stdin>:1:1]) net.starlark.java.eval.Starlark.fastcall(Starlark.java:621) net.starlark.java.eval.Starlark.execFileProgram(Starlark.java:892) ... exit 1 ``` Now: ``` >> [1] * (1 << 30) * (1 << 5) Traceback (most recent call last): File "<stdin>", line 1, column 21, in <toplevel> Error: got 34359738368 for repeat, want value in signed 32-bit range ``` - starlark: int: ignore base prefix unless matches explicit base … Previously, when int was called with an explicit base, it would report an error if the digit string starts with a base prefix for a different base, such as int("0b101", 16). Now, it uses the base prefix only if it matches the requested base, so the example above would return 0x0b101, as would int("0x0b101", 16). See github.com/google/starlark-go/pull/344 - starlark: delete StarlarkFile.parseWithPrelude … It was an obstacle to interpreter optimizations, as it caused a single StarlarkFile to contain statements whose locations come from different files. The previous workspace logic used parseWithPrelude to concatenate all the statements of the WORKSPACE files (prefix + main + suffix) and then split them into chunks, ignoring---crucially, several days work revealed---file boundaries. The splitChunks logic now achieves the same effect by representing chunks as lists of nonempty partial files, and calling execute() for each partial file in the chunk. The chunk splitting tests have been rewritten, clarified, and expanded to exercise the chunk-spans-file-boundaries case. Many thanks to Jon Brandvein for hours of help trying to figure out what the workspace logic does. It is not our team's finest work. Also: - minor consequent simplifications to parser and lexer. - narrow the scope of various try blocks (sorry, messy diff). - starlark: remove redundant pattern validity check in Printer … - starlark: support %x, %X, and %o conversions in 'string % number' … Also, improve operand type error message to show type, not value. See github.com/bazelbuild/starlark/pull/154 for spec change. - Starlark: StarlarkInt.{floordiv,mod} now work with 64-bit integers … * No special case for 32-bit integers, division is slow anyway * Switch `mod` to use `Math.floorMod` for simplicity/safety Closes #12667. - starlark: delete deprecated EvalException(Location) constructor … ...as there are, at long last, no further uses within Bazel. Also, clarify doc comments regarding EvalException.callstack. - bazel analysis: preparatory cleanup to SRCTU error reporting … This change causes the error handling logic in StarlarkRuleConfiguredTargetUtil (SRCTU) to avoid the EvalException(Location) constructor. Instead, the auxiliary location, if any, of provider instantiation is simply prepended to the error message (see the infoError function) in anticipation of being printed after a newline. For example: ERROR p/BUILD:1:1:\n foo.bzl:1:2: <provider-related message> createTarget uses a distinct exception, BadImplementationFunction, for errors that arise not from the Starlark implementation function but from the post-processing of the providers. A follow-up change will replace this exception by directly reporting events to the handler, thus permitting both higher quality structured errors capable of reporting more than one relevant location, and multiple errors in a single pass. However, that change is too tricky to accomplish in this CL. Also: - move "advertised provider" and "orphaned file" checks into createTarget. - add comments to document this particularly painful code. - delete EvalException.getDeprecatedLocation. - Starlark: long StarlarkInt multiply without BigInteger … Use Hacker's Delight 8-2. Closes #12643. - bazel packages: use EventHandler not EvalException in .bzl "export" o… … …peration This allows us to delete one of the deprecated EvalException(Location,...) constructors. Similar follow-up changes (events not exceptions) will be required to eliminate the remaining such constructor. As a bonus, this approach allows multiple errors to be reported at once.
la-luo
pushed a commit
to la-luo/starlarky
that referenced
this pull request
Sep 8, 2021
…21089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573) Changelog for (Master)[bazelbuild/bazel@be96ade#diff-37cd73cae488b2421089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573]: - Builtins injection: Rename _internal to _builtins and add functionality … This object is used in @_builtins .bzl files but is not accessible to user code. The `toplevel` and `native` fields give access to the *native* (pre-injected) values of symbols whose post-injected values are available to user .bzl files. For instance, -`_builtins.toplevel.CcInfo` in @_builtins code gives the original CcInfo definition from the Java code, even if `CcInfo` in a regular .bzl file refers to an injected value. (To avoid ambiguity, `CcInfo` itself is not a valid top-level symbol for @_builtins .bzl files.) The `internal` field contains any value registered via ConfiguredRuleClassProvider.Builder#addStarlarkBuiltinsInternal(). The `getFlag()` method can retrieve the values of StarlarkSemantics flags. Because of how flags are stored, it requires that a default value be given. - Starlark: better errors on integer overflow … Before: ``` >> [1] * (1 << 30) * (1 << 5) Exception in thread "main" net.starlark.java.eval.Starlark$UncheckedEvalException: java.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 1073741824 out of bounds for object array[0] (Starlark stack: [<toplevel>@<stdin>:1:1]) net.starlark.java.eval.Starlark.fastcall(Starlark.java:621) net.starlark.java.eval.Starlark.execFileProgram(Starlark.java:892) ... exit 1 ``` Now: ``` >> [1] * (1 << 30) * (1 << 5) Traceback (most recent call last): File "<stdin>", line 1, column 21, in <toplevel> Error: got 34359738368 for repeat, want value in signed 32-bit range ``` - starlark: int: ignore base prefix unless matches explicit base … Previously, when int was called with an explicit base, it would report an error if the digit string starts with a base prefix for a different base, such as int("0b101", 16). Now, it uses the base prefix only if it matches the requested base, so the example above would return 0x0b101, as would int("0x0b101", 16). See github.com/google/starlark-go/pull/344 - starlark: delete StarlarkFile.parseWithPrelude … It was an obstacle to interpreter optimizations, as it caused a single StarlarkFile to contain statements whose locations come from different files. The previous workspace logic used parseWithPrelude to concatenate all the statements of the WORKSPACE files (prefix + main + suffix) and then split them into chunks, ignoring---crucially, several days work revealed---file boundaries. The splitChunks logic now achieves the same effect by representing chunks as lists of nonempty partial files, and calling execute() for each partial file in the chunk. The chunk splitting tests have been rewritten, clarified, and expanded to exercise the chunk-spans-file-boundaries case. Many thanks to Jon Brandvein for hours of help trying to figure out what the workspace logic does. It is not our team's finest work. Also: - minor consequent simplifications to parser and lexer. - narrow the scope of various try blocks (sorry, messy diff). - starlark: remove redundant pattern validity check in Printer … - starlark: support %x, %X, and %o conversions in 'string % number' … Also, improve operand type error message to show type, not value. See github.com/bazelbuild/starlark/pull/154 for spec change. - Starlark: StarlarkInt.{floordiv,mod} now work with 64-bit integers … * No special case for 32-bit integers, division is slow anyway * Switch `mod` to use `Math.floorMod` for simplicity/safety Closes #12667. - starlark: delete deprecated EvalException(Location) constructor … ...as there are, at long last, no further uses within Bazel. Also, clarify doc comments regarding EvalException.callstack. - bazel analysis: preparatory cleanup to SRCTU error reporting … This change causes the error handling logic in StarlarkRuleConfiguredTargetUtil (SRCTU) to avoid the EvalException(Location) constructor. Instead, the auxiliary location, if any, of provider instantiation is simply prepended to the error message (see the infoError function) in anticipation of being printed after a newline. For example: ERROR p/BUILD:1:1:\n foo.bzl:1:2: <provider-related message> createTarget uses a distinct exception, BadImplementationFunction, for errors that arise not from the Starlark implementation function but from the post-processing of the providers. A follow-up change will replace this exception by directly reporting events to the handler, thus permitting both higher quality structured errors capable of reporting more than one relevant location, and multiple errors in a single pass. However, that change is too tricky to accomplish in this CL. Also: - move "advertised provider" and "orphaned file" checks into createTarget. - add comments to document this particularly painful code. - delete EvalException.getDeprecatedLocation. - Starlark: long StarlarkInt multiply without BigInteger … Use Hacker's Delight 8-2. Closes #12643. - bazel packages: use EventHandler not EvalException in .bzl "export" o… … …peration This allows us to delete one of the deprecated EvalException(Location,...) constructors. Similar follow-up changes (events not exceptions) will be required to eliminate the remaining such constructor. As a bonus, this approach allows multiple errors to be reported at once.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when int was called with an explicit base,
it would report an error if the digit string starts
with a base prefix for a different base, such as int("0b101", 16).
Now, it uses the base prefix only if it matches the requested
base, so the example above would return 0x0b101, as would
int("0x0b101", 16).
The int(string, int) case has been split out for clarity.
Update doc.
Fixes #337