Skip to content

[lua] Strip trailing ".0" from whole-valued floats in Std.string - #13005

Open
jdonaldson wants to merge 1 commit into
HaxeFoundation:developmentfrom
jdonaldson:lua-tostring-whole-float
Open

[lua] Strip trailing ".0" from whole-valued floats in Std.string#13005
jdonaldson wants to merge 1 commit into
HaxeFoundation:developmentfrom
jdonaldson:lua-tostring-whole-float

Conversation

@jdonaldson

Copy link
Copy Markdown
Member

On Lua 5.3+, numbers split into integer/float subtypes, so tostring(1.0) yields "1.0" (via %.14g) while tostring(1) yields "1". This made Std.string on Lua print whole-valued floats with a spurious trailing .0Std.string(2.0*3)"6.0", an abstract toString"100.0(m)" — diverging from every other target (js/python/neko/eval → 6, 100(m)). Lua 5.1/LuaJIT have no subtypes and never showed this.

Std.string(x:Dynamic) erases the static Int/Float distinction, so the fix belongs at runtime in _hx_tostring, the single funnel for number→string conversion. Lua's float formatter only ever appends an exact .0 to integer-looking output, so a 2-char suffix strip is sufficient — scientific notation (1e+15), fractions (1.5), and NaN/Infinity are untouched, and on Lua 5.1/LuaJIT it is a no-op.

Testing

Built from source (5.0.0-preview.1) and ran tests/unit on Lua 5.5.0 (which has the subtype split, so the bug manifests). This re-enables the TestBasetypes and Issue3345 (#3345) assertions that were guarded with #if !lua solely because of this bug.

  • All 11529 assertions pass with the fix.
  • Reverting just the runtime change (keeping the un-guarded tests) reproduces exactly the 5 expected failures:
    • Issue3345: "100.0(m)" / "-100.0(m)" instead of "100(m)" / "-100(m)"
    • TestBasetypes: "(2.0,4.0,6.0)", "(2.0,2.0,2.0)", "(4.0,4.0,4.0)" instead of the integer-formatted expectations.

On Lua 5.3+, numbers are split into integer and float subtypes, so
`tostring(1.0)` yields "1.0" (via %.14g) while `tostring(1)` yields "1".
This made `Std.string` on Lua print whole-valued floats with a trailing
".0" ("(2.0,4.0,6.0)", "100.0(m)"), diverging from every other target
(js/python/neko/eval → "(2,4,6)", "100(m)").

`Std.string(x:Dynamic)` erases the static Int/Float distinction, so the
divergence must be resolved at runtime in `_hx_tostring`, the single
funnel for number→string conversion. Lua's float formatter only ever
appends an exact ".0" to integer-looking output, so a 2-char suffix
strip is sufficient: scientific notation, fractions and NaN/Infinity are
untouched, and on Lua 5.1/LuaJIT (no subtypes) it is a no-op.

Re-enables the TestBasetypes and Issue3345 assertions that were guarded
with `#if !lua` solely because of this bug.
@jdonaldson

Copy link
Copy Markdown
Member Author

The main benefit here is making Lua tests run equivalently to the other platforms for simple tests. I had thought adding in an adjustment like this would add overhead, but it's only a few nanoseconds. This is technically a soft breaking change, so I thought the Haxe 5.0 version preview was the place to do it.

-- formatter (%.14g) only ever appends an exact ".0" to
-- integer-looking output, so a suffix check is sufficient: scientific
-- notation ("1e+15"), fractions ("1.5") and NaN/Infinity are
-- untouched, and on Lua 5.1/LuaJIT (no ".0" suffix) this is a no-op.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment is quite verbose, and it will be included in the user's output lua file so maybe it's best to shorten or remove it. (If we move more of this lua code to haxe like in #12600, we can have more freedom to add comments etc)

The comment also seems to not mention Lua 5.2.

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.

Good call, I'll update it on the next go-round.

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.

2 participants