[stdlib] Fix atol parsing for prefixed integer literals with leading underscores

[jjvraw]

This addresses #3182 to correctly parse prefixed string literals. Changes include:

• Bug-fix
• Added appropriate test cases to verify the new behaviour

When the base 2, 8, 16 are set or inferred (base 0), for a string prefixed appropriately, leading underscores should be allowed as per Python's integer literal grammar:

integer      ::=  decinteger | bininteger | octinteger | hexinteger
decinteger   ::=  nonzerodigit (["_"] digit)* | "0"+ (["_"] "0")*
bininteger   ::=  "0" ("b" | "B") (["_"] bindigit)+
octinteger   ::=  "0" ("o" | "O") (["_"] octdigit)+
hexinteger   ::=  "0" ("x" | "X") (["_"] hexdigit)+
nonzerodigit ::=  "1"..."9"
digit        ::=  "0"..."9"
bindigit     ::=  "0" | "1"
octdigit     ::=  "0"..."7"
hexdigit     ::=  digit | "a"..."f" | "A"..."F"

Currently, this such a string passed to atol raises. The changes in this PR fixes this issue.

I should note that I have another PR (#3207) to refactor, and clean up, atol for reusability to introduce stol as per request #2639. If this PR is merged, I will update #3207, which will require a minor tweak.

[martinvuyk]

Thanks for taking the time to do this, could you add some of the suggestions and tests for the other PR here? I see the same behavior missing (underscore and 0 at the beginning of non base 10 numbers)

[martinvuyk]

LGTM!
btw. loved this refactor

    var start = pos
    if start + 1 < str_len:
        var prefix_char = str_ref[start + 1]
        if str_ref[start] == "0" and (
            (base == 2 and (prefix_char == "b" or prefix_char == "B"))
            or (base == 8 and (prefix_char == "o" or prefix_char == "O"))
            or (base == 16 and (prefix_char == "x" or prefix_char == "X"))
        ):
            start += 2
    return start

[jjvraw]

Thanks @martinvuyk :) But single newline seems to be consistent with other docstring, no?

mojo/stdlib/src/builtin/string.mojo

Lines 32 to 44 in 8bd1dbd

	fn ord(s: String) -> Int:
	"""Returns an integer that represents the given one-character string.
	Given a string representing one character, return an integer
	representing the code point of that character. For example, `ord("a")`
	returns the integer `97`. This is the inverse of the `chr()` function.
	Args:
	s: The input string, which must contain only a single character.
	Returns:
	An integer representing the code point of the given character.
	"""

[jjvraw]

I have repurposed this PR to focus on the mentioned issue.

[jjvraw]

We want was_last_underscore to be false only when an accepted prefix is present, which is not necessarily when real_base != 10, but rather only if real_base is 2,8,16 and it contains a prefix, according to the lexical syntax of Python.

Thoughts @soraros, @martinvuyk

[martinvuyk]

You're right, I hadn't seen "Base must be >= 2 and <= 36, or 0." so that means this could receive other bases that have no prefix. So yeah totally, leading underscore should fail
Python:

print(int("_123", 30))
ValueError: invalid literal for int() with base 30: '_123'

so it should be

real_base in (2, 8, 16)

nice catch, maybe we could add some tests with assert raises (and also passing ones) with bases other than the classics

[jjvraw]

Will for sure add some test cases with the next commit. We will have to add a check for whether there exists a prefix as well though.

So something along the lines of real_base in (2, 8, 16) and has_prefix, as we could have atol("_ff", 16) which should raise as invalid.

Was playing around with (real_base & 0b11010) != real_base and has_prefix which would've have been fun... 10 and 18 breaks it though.

[martinvuyk]

So something along the lines of real_base in (2, 8, 16) and has_prefix, as we could have atol("_ff", 16) which should raise as invalid.

yeah I guess str_ref[start] == "0" can be taken out of the if to a has_prefix var, nice catch again :D

Was playing around with (real_base & 0b11010) != real_base and has_prefix which would've have been fun...

Niiice, maybe this will work?

from bit import is_power_of_two
...
# first character as underscore is only valid for bases 2, 8, 16 with prefix
var was_last_digit_undescore = not (is_power_of_two(real_base) and has_prefix and real_base != 32)
...

[jjvraw]

I like the concept! Will have to catch 4 explicitly there as well.

We can use a mask, bitmask = (1 << 2) | (1 << 8) | (1 << 16) and then not ((1 << real_base) & bitmask) and has_prefix.

This seems to work. But it might be overcomplicated for a simple value range check.

[soraros]

yeah I think this was a fun exercise lol but let's just stick with tuple contains

I agree. At least we can defer further optimisation to a later PR should we find it necessary.

[jjvraw]

@martinvuyk, I couldn't get it to run. Cannot call cast on a Boolean.

[martinvuyk]

Then it will probably work by just putting int(real_base < 17 and has_prefix)

[jjvraw]

Same issue @martinvuyk, but now for Int.

[martinvuyk]

yeah but removing the .cast since it is a SIMD method, but never mind it's not important

[JoeLoser]

!sync

[JoeLoser]

!sync

[modularbot]

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

[modularbot]

Landed in 2aa4060! Thank you for your contribution 🎉