Skip to content

fix(lua): compare float with int strictly in equality (#7587) - #7611

Open
bultodepapas wants to merge 3 commits into
EdgeTX:mainfrom
bultodepapas:fix/lua-float-integer-equality
Open

fix(lua): compare float with int strictly in equality (#7587)#7611
bultodepapas wants to merge 3 commits into
EdgeTX:mainfrom
bultodepapas:fix/lua-float-integer-equality

Conversation

@bultodepapas

Copy link
Copy Markdown
Contributor

Fixes #7587

Summary

0.50 == 0 (and 0.5 == 0) incorrectly returned true in the EdgeTX Lua runtime. This breaks floating point comparisons in Lua scripts.

Root cause

LUA_FLOORN2I is set to 1 in luaconf.h so EdgeTX API functions can accept unrounded floats where integers are expected. However, the same soft conversion was also used by luaV_equalobj for the == operator: comparing a float and an integer converted the float with luaV_tointeger(..., LUA_FLOORN2I) (i.e. flooring), so 0.5 was turned into 0 and 0.5 == 0 became true.

Stock Lua 5.3 uses the strict (tointegerns) conversion for equality: only integral floats compare equal to integers.

Fix

  • Add tointegerns macro in lvm.h (same as stock Lua 5.3): converts floats to integers only when the value is integral.
  • Use tointegerns in luaV_equalobj instead of tointeger.

LUA_FLOORN2I behaviour for API arguments is unchanged.

Tests

Added Lua.testFloatIntegerEquality in radio/src/tests/lua.cpp verifying:

  • 0.5 ~= 0 and 0.50 ~= 0 are now true
  • 1.0 == 1, 0.0 == 0 still hold

Verified by compiling the Lua runtime standalone and running the assertions.

Copilot AI review requested due to automatic review settings August 3, 2026 19:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes Lua numeric equality in the embedded Lua 5.3 VM so that float–integer comparisons via == are strict (only integral floats can equal integers), preventing cases like 0.5 == 0 incorrectly evaluating to true when LUA_FLOORN2I is enabled for API argument coercion.

Changes:

  • Added a strict integer-conversion macro (tointegerns) that only converts integral floats to integers.
  • Updated luaV_equalobj to use the strict conversion for float–integer equality.
  • Added a regression test covering non-integral and integral float–integer equality behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
radio/src/thirdparty/Lua/src/lvm.h Adds tointegerns macro for non-soft (strict) float→int conversion.
radio/src/thirdparty/Lua/src/lvm.c Switches mixed-variant numeric equality to strict integer conversion.
radio/src/tests/lua.cpp Adds a regression test ensuring 0.5 ~= 0 while 1.0 == 1 still holds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@J-Sorenson

Copy link
Copy Markdown

Thanks for throwing your hat in. Can you add tests for <= and >= with the same test values? I didn't try those in my original testing.

@bultodepapas

bultodepapas commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've added <= and >= tests for the same values in radio/src/tests/lua.cpp (commit 19dc95d):

  • 0.5 >= 0 / 0.50 >= 0 => true
  • 0.5 <= 0 / 0.50 <= 0 => false
  • 1.0 >= 1 / 1.0 <= 1 => true
  • 0.0 >= 0 / 0.0 <= 0 => true

Order comparisons (<, <=, >, >=) compare the float and integer directly as numbers (via luai_numlt/luai_numle), so they were already correct - only ==/~= went through the integer-conversion path. I verified all of the above by compiling the Lua runtime standalone.

Extend the EdgeTX#7587 regression test to cover:
- the reported scenario via a variable (v = 0.50)
- negative non-integral floats (-0.5)
- all comparison operators (==, ~=, <, >, <=, >=)
- mixed arithmetic still yielding floats (math.type)

Also document in lvm.h why equality uses strict (non-soft)
conversion while EdgeTX API argument coercion keeps the soft
LUA_FLOORN2I behavior.
@bultodepapas

bultodepapas commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

While extending the regression test, I checked the remaining Lua 5.3 surface for the same root cause (LUA_FLOORN2I softening integer coercion) and found one related spot worth flagging.

math.tointeger normally returns nil for non-integral floats, but EdgeTX currently inherits the soft conversion because math_toint in lmathlib.c uses lua_tointegerx. With LUA_FLOORN2I=1, this yields:

math.tointeger(3.7)  --> 3  -- stock Lua 5.3: nil
math.tointeger(0.5)  --> 0  -- stock Lua 5.3: nil

I deliberately did not change it here. The EdgeTX API layer relies on soft argument coercion, and changing lua_tointegerx globally would be out of scope and risky. This PR only restores strict mixed float/integer equality and documents the strict-versus-soft distinction in lvm.h.

@J-Sorenson

Copy link
Copy Markdown

Tests look good. I'd prefer leaving math.tointeger alone for now, as that is out of scope of the original issue. Changing that might affect existing LUA scripts that have been written recently and expect the "floor" behavior. Making that change will delay acceptance of the PR, since the devs would have to verify it doesn't break existing scripts. In contrast, your current fix would restore pre-existing behavior, which is what we want.

Ah, this didn't get the labels from the issue because you made it a standalone PR. I suggest you add the "bug", "triage", and "lua" labels in order for this PR to have any visibility.

@bultodepapas

bultodepapas commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up for maintainers: per the review feedback, this PR still needs the bug 🪲, triage, and lua labels for visibility. Contributors cannot apply repository labels, so could a maintainer add them when convenient?

The requested <= / >= regression coverage is included in commit 19dc95d0, and J-Sorenson confirmed that the tests look good. No further code changes are currently requested.

@bultodepapas

Copy link
Copy Markdown
Contributor Author

@pfeerick, when you have time, could you take a look at this Lua VM regression fix? The requested comparison coverage is in 19dc95d0, J-Sorenson confirmed the tests look good, and math.tointeger remains deliberately out of scope. I cannot formally assign upstream reviewers, so I am mentioning you here.

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.

Lua Floating point judgment error

3 participants