Skip to content

fix: type inference for bitwise operators on integer variables#3394

Open
RomanSpector wants to merge 2 commits intoLuaLS:masterfrom
RomanSpector:fix/bitwise-operator-inference
Open

fix: type inference for bitwise operators on integer variables#3394
RomanSpector wants to merge 2 commits intoLuaLS:masterfrom
RomanSpector:fix/bitwise-operator-inference

Conversation

@RomanSpector
Copy link
Copy Markdown

Summary

Bitwise operators (<<, >>, &, |, ~) on integer-typed variables returned unknown instead of integer. Constant folding also failed for chained expressions.

Before

local PACK_YZ      = 1 << 20        -- integer = 1048576 (OK)
local PACK_X       = (PACK_YZ << 1) -- unknown (WRONG)
local PACK_YZ_MASK = ((PACK_X << 1) - 1) -- unknown (WRONG)

After

local PACK_YZ      = 1 << 20        -- integer = 1048576
local PACK_X       = (PACK_YZ << 1) -- integer = 2097152
local PACK_YZ_MASK = ((PACK_X << 1) - 1) -- integer = 4194303

Changes

script/vm/operator.lua: Add integer type fallback for bitwise operators when operands are integer-typed but not literal constants. This matches existing behavior for arithmetic operators (+, -, *, %).

script/vm/value.lua: vm.getInteger now traces through getlocal to the local definition's value expression when the compiled node doesn't preserve the literal integer. This enables constant folding for chained bitwise/arithmetic expressions.

Test Plan

  • New tests: a << 1 and (a << 1) - 1 where a = 1 << 20
  • All existing tests pass

Bitwise operators (<<, >>, &, |, ~) on integer-typed variables
returned 'unknown' instead of 'integer'. For example:

  local a = 1 << 20  -- integer = 1048576 (OK)
  local b = a << 1   -- was: unknown, now: integer = 2097152

Two fixes:
- operator.lua: add integer type fallback for bitwise ops when
  operands are integer-typed but not literal constants (matching
  existing behavior for +, -, *, % operators)
- value.lua: vm.getInteger now traces through getlocal to the
  local definition's value when the compiled node doesn't preserve
  the literal, enabling constant folding for chained expressions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves type inference for bitwise operators on integer variables and fixes constant value computation for chained bitwise expressions in hover tooltips. Key changes include updating vm.binarySwitch to explicitly set the result type to 'integer' for bitwise operations and modifying vm.getInteger to resolve local values more effectively. Review feedback identifies a logic error in vm.getInteger regarding the handling of union types (mixed integer and non-integer) and suggests refactoring duplicated operand inference logic into a helper function to improve maintainability.

Comment on lines +142 to +144
if hasNonInteger and not result then
result = nil
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The logic for handling mixed types appears to be incorrect. If a variable can be both an integer and another non-integer type (a union), getInteger should return nil. With the current change, if an integer value is found (result is set) and a non-integer type is also found (hasNonInteger is true), the condition hasNonInteger and not result will be false. This causes the function to incorrectly return the integer value instead of nil for a union type.

To fix this, result should be cleared if any non-integer type is found, before attempting the recursive lookup.

    if hasNonInteger then
        result = nil
    end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Changed to unconditionally clear result when hasNonInteger is true, so union types (e.g. integer|string) correctly return nil.

Comment on lines +273 to +275
local uri = guide.getUri(source)
local infer1 = vm.getInfer(source[1])
local infer2 = vm.getInfer(source[2])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This block of code to get the URI and inferred types of operands is duplicated in several places within this file (e.g., lines 327-329, 343-345, 353-355, and 398-400). To improve maintainability and reduce redundancy, consider extracting this logic into a helper function.

For example, you could define this function at the top of the file:

local function getInfers(source)
    return guide.getUri(source), vm.getInfer(source[1]), vm.getInfer(source[2])
end

And then replace the duplicated blocks with a single call:

            local uri, infer1, infer2 = getInfers(source)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Extracted getOperandInfers(source) helper, replaced all 5 duplicated blocks.

- value.lua: clear result when any non-integer type is found in node,
  preventing incorrect integer return for union types (integer|string)
- operator.lua: extract getOperandInfers() helper to deduplicate
  URI/infer retrieval across 5 call sites

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

1 participant