Skip to content

feat: minimal LiteralDict lowering support#174

Open
m-akhil-reddy wants to merge 6 commits intoarxlang:mainfrom
m-akhil-reddy:feat/literal-dict-30
Open

feat: minimal LiteralDict lowering support#174
m-akhil-reddy wants to merge 6 commits intoarxlang:mainfrom
m-akhil-reddy:feat/literal-dict-30

Conversation

@m-akhil-reddy
Copy link

Notes

  • This repository uses an AI bot for reviews. Keep your PR in Draft while
    you work. When you’re ready for a review, change the status to Ready for
    review
    to trigger a new review round. If you make additional changes and
    don’t want to trigger the bot, switch the PR back to Draft.
  • AI-bot comments may not always be accurate. Please review them critically and
    share your feedback; it helps us improve the tool.
  • Avoid changing code that is unrelated to your proposal. Keep your PR as short
    as possible to increase the chances of a timely review. Large PRs may not be
    reviewed and may be closed.
  • Don’t add unnecessary comments. Your code should be readable and
    self-documenting
    (guidance).
  • Don’t change core features without prior discussion with the community. Use
    our Discord to discuss ideas, blockers, or issues
    (https://discord.gg/Nu4MdGj9jB).
  • Do not include secrets (API keys, tokens, passwords), credentials, or
    sensitive data/PII in code, configs, logs, screenshots, or commit history. If
    something leaks, rotate the credentials immediately, invalidate the old key,
    and note it in the PR so maintainers can assist.
  • Do not commit large binaries or generated artifacts. If large datasets are
    needed for tests, prefer small fixtures or programmatic downloads declared in
    makim.yaml (e.g., a task that fetches data at test time). If a large binary is
    unavoidable, discuss first and consider Git LFS.

Pull Request description

How to test these changes

  • ...

Pull Request checklists

This PR is a:

  • bug-fix
  • [ Y] new feature
  • maintenance

About this PR:

  • [ Y] it includes tests.
  • [ Y] the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • [Y ] I have reviewed the changes and it contains no misspelling.
  • [ Y] The code is well commented, especially in the parts that contain more
    complexity.
  • [ Y] New and old tests passed locally.

Additional information

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved .

@github-actions
Copy link

github-actions bot commented Mar 1, 2026

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

src/irx/builders/llvmliteir.py

LGTM!


tests/test_literal_dict.py

  • (L.32) The empty-dict test claims default lowering to [0 x {i32, i32}] but doesn’t assert the element struct type. Add an assertion that const.type.element is a LiteralStructType of exactly two i32 fields; otherwise an incorrect default element type could slip by undetected.
  • (L.66) Strengthen the check to assert 32-bit width, not just IntType, e.g., ensure both fields are i32. As written, an i64 pair would pass.
  • (L.81, L.107) The regex matches for error messages are brittle; small text changes will break tests. Consider matching a stable substring or dropping match to rely on the exception type.
  • The tests couple tightly to translator internals via visitor.result_stack. If the translator API changes, these will break. Consider adding a small public helper on Builder to evaluate a node to a constant to decouple tests from internal stacks.

@github-actions
Copy link

github-actions bot commented Mar 1, 2026

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

docs/index.md

LGTM!


src/irx/builders/llvmliteir.py

  • Potential stack underflow: result_stack.pop() will raise IndexError if nothing was pushed; the subsequent None checks are dead code and won’t fire. This can surface as an unhandled IndexError instead of a compiler error. Consider guarding pops or using a safe-pop helper to raise a consistent IRx error with node context.
  • Inconsistent exception types: two bare Exception raises for key/value failures; elsewhere you use TypeError for unsupported cases. Use a consistent, project-specific error type to avoid surprising handlers.
  • Constant detection may reject common “literal” cases (e.g., strings). If string literals lower to GlobalValue or a non-ir.Constant value, all_constants will be False and the code will raise, even for syntactically constant dicts. Verify string-literal lowering and either broaden the “constant-like” predicate or fail with a clearer message for now.
  • Type mismatch for empty vs non-empty dicts: empty uses placeholder i32/i32, non-empty infers K/V from first pair. If empty dicts can flow into contexts expecting a specific K/V, this may later fail type checks. Ensure upstream inference handles this or add a TODO to harmonize placeholder with expected K/V types.

tests/test_literal_dict.py

  • Potential false negative in non-constant test: using astx.Variable as a dict key may raise before visit() (unhashable), masking the intended TypeError from the visitor. Make the value non-constant instead of the key. (L.110-L.112)
    elements={
    astx.LiteralInt32(1): var,
    }

  • The empty-dict test doesn’t assert the element type of the array; a regression could change the pair shape unnoticed. Add element-struct shape checks. (L.33)

    Verify element struct shape even for empty arrays

    assert isinstance(const.type.element, ir.LiteralStructType)
    elem = const.type.element
    assert len(elem.elements) == 2
    assert all(isinstance(t, ir.IntType) and t.width == 32 for t in elem.elements)

  • The homogeneous-dict test only checks shape, not contents. Strengthen by asserting key/value pairs and bit-widths. (L.67)

    Verify struct field widths

    assert all(isinstance(t, ir.IntType) and t.width == 32 for t in struct_ty.elements)

    Verify contents (order-independent)

    pairs = {(s.operands[0].constant, s.operands[1].constant) for s in const.operands}
    assert pairs == {(1, 10), (2, 20)}

  • Tests directly poke internal translator/result_stack, tightly coupling to internals and increasing brittleness across refactors. Consider exposing a small public API on Builder to evaluate an expression to a constant for tests, and use that instead.


Copy link
Author

@m-akhil-reddy m-akhil-reddy left a comment

Choose a reason for hiding this comment

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

i am facing some env issue in my pc

@github-actions
Copy link

github-actions bot commented Mar 1, 2026

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

docs/index.md

File deleted; no review.


src/irx/builders/llvmliteir.py

  • Correctness: Emitting [0 x {i32, i32}] for empty dicts is a latent type bug. That placeholder will not unify with the element type inferred from non-empty dicts or variable annotations, and will cause IR type mismatches at uses/PHIs. Prefer either:

    • derive the pair type from contextual/annotated types if available, or
    • explicitly reject empty dicts until typing is known, rather than emitting a placeholder.
      Suggested change (L.1668):
      def _build_empty_dict_constant(self) -> ir.Constant:
      """Build an empty dict constant with a safe/expected element type or fail fast."""
      raise TypeError("LiteralDict: empty dict without known key/value types is not supported")

    and replace the current empty-dict branch with a call to this helper.

  • Robustness: Popping from result_stack without guarding can raise IndexError and the bare Exception messages are inconsistent with the rest of the method’s TypeError contract. Change both key/value failures to raise TypeError and guard the pop (L.1657, L.1664):
    def _pop_result_or_type_error(self, what: str) -> ir.Value:
    """Pop a lowered value from the result stack or raise a TypeError."""
    if not self.result_stack:
    raise TypeError(f"LiteralDict: failed to lower {what}.")
    val = self.result_stack.pop()
    if val is None:
    raise TypeError(f"LiteralDict: failed to lower {what}.")
    return val

    usage:

    key_val = self._pop_result_or_type_error("key")
    val_val = self._pop_result_or_type_error("value")

  • Determinism: Iterating node.elements.items() assumes insertion-ordered mapping semantics. If astx.LiteralDict.elements is not guaranteed ordered, this will produce nondeterministic IR for the same source. Consider using an explicit ordered sequence from the AST (e.g., a list of pairs) or documenting/enforcing ordered semantics for elements (L.1651).


tests/test_literal_dict.py

  • Potential false-positive in non-constant test: using astx.Variable as a dict key may raise a Python TypeError for “unhashable type” before visitor.visit runs and the error message won’t match “only empty or all-constant”. Use a constant key and put the non-constant on the value side instead. Change to:

    • (L.110-112)
      elements={astx.LiteralInt32(1): var}
  • Tests tightly couple to LLVMLiteIRVisitor internals (result_stack), which is brittle and can break with refactors. Consider a small helper to encapsulate this pattern and centralize the internal access:

    • (L.16) add:
      def visit_and_pop_const(visitor: LLVMLiteIRVisitor, node: object) -> ir.Constant:
      """Visit node and return constant"""
      visitor.result_stack.clear()
      visitor.visit(node)
      const = visitor.result_stack.pop()
      assert isinstance(const, ir.Constant)
      return const

@github-actions
Copy link

github-actions bot commented Mar 1, 2026

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

docs/index.md

File deleted; no review.


src/irx/builders/llvmliteir.py

LGTM!


tests/test_literal_dict.py

  • Correctness: You don’t actually assert the i32 width anywhere. This could let an incorrect lowering (e.g., i64) slip through.

    • In the homogeneous test, strengthen the type check to enforce 32-bit ints (L.69):
      assert all(isinstance(t, ir.IntType) and t.width == 32 for t in struct_ty.elements)
    • In the empty test, also assert the element struct shape to match the docstring “[0 x {i32, i32}]” (insert after L.35):
      assert isinstance(const.type.element, ir.LiteralStructType)
      struct_ty = const.type.element
      assert len(struct_ty.elements) == EXPECTED_STRUCT_FIELDS
      assert all(isinstance(t, ir.IntType) and t.width == 32 for t in struct_ty.elements)
  • Test fragility: Using a non-literal AST node as a dict key may raise a Python “unhashable” error before your code under test runs, masking the intended TypeError.

    • Put the non-constant on the value side so the key remains a known-hashable literal (L.112-L.114):
      elements=cast(
      dict[astx.Literal, astx.Literal],
      {astx.LiteralInt32(1): var},
      )

@github-actions
Copy link

github-actions bot commented Mar 1, 2026

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

docs/index.md

LGTM!


src/irx/builders/llvmliteir.py

  • High-impact correctness: dicts containing string literals will likely hit the non-constant path and raise. In llvmlite, lowering a string literal usually yields an ir.GlobalVariable (not ir.Constant), so the all_constants check will be False even for “literal-only” dicts. Please treat globals as “const-like” so literal dicts of strings (and mixed consts/globals) don’t error. Replace the all_constants check with a const-like predicate and proceed with the constant array initializer when all entries are const-like (L.~1685).
    Suggested helper:
    def _is_const_like(self, v: ir.Value) -> bool:
    """Return True if v can appear in a constant initializer (constant or global)."""
    return isinstance(v, ir.Constant) or isinstance(v, ir.GlobalVariable)

    Then:
    all_const_like = all(self._is_const_like(k) and self._is_const_like(v) for k, v in llvm_pairs)

  • Potential semantic bug: iterating node.elements.items() (L.~1660) assumes a mapping with stable insertion order and also discards duplicate keys. If ASTx allows duplicate keys or enforces “last write wins,” this can silently change semantics. Prefer iterating an explicit ordered pairs list from the AST (e.g., node.pairs), or assert that elements preserves insertion order and duplicates are validated earlier.

  • Type/compat risk: empty dict returns [0 x {i32, i32}] while non-empty returns [N x {K, V}]. Mixing empty and non-empty dicts in the same expression path will produce type mismatches downstream. Consider converging on a canonical pair element type for all dict literals (e.g., {i8*, i8*} or target-pointer-sized ints) and bitcast constants on construction to avoid divergence. If out of scope, at least document this limitation.

  • Robustness: result_stack.pop() will raise IndexError if a child visit failed to push; the subsequent None check won’t run. Use a safe pop helper and raise a clear internal build error (L.~1664 and L.~1672).
    Suggested helper:
    def _pop_result(self, context: str) -> ir.Value:
    """Pop one value from result_stack or raise a clear IR build error."""
    try:
    v = self.result_stack.pop()
    except IndexError as e:
    raise RuntimeError(f"LiteralDict: no result produced while lowering {context}") from e
    if v is None:
    raise RuntimeError(f"LiteralDict: failed to lower {context}")
    return v

    And replace:
    key_val = self._pop_result("key")
    val_val = self._pop_result("value")


tests/test_literal_dict.py

  • Potential pre-visit failure due to unhashable dict key: astx.Variable is likely unhashable (common when nodes define eq without hash), so the test may error at dict construction rather than inside visitor.visit. To actually exercise the "non-constant" path, keep the key constant and make the value non-constant.
    Replace the mapping with a constant key and var as value (L.112-L.115):
    elements=cast(
    dict[astx.Literal, astx.Literal],
    {astx.LiteralInt32(1): var},
    )

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