Skip to content

Conversation

@yuvimittal
Copy link
Member

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

#32

How to test these changes

  • ...

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • 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:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more
    complexity.
  • 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 .

@yuvimittal yuvimittal marked this pull request as draft December 11, 2025 17:16
@github-actions
Copy link

OSL ChatGPT Reviewer

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

src/irx/builders/llvmliteir.py

  • Correctness: Nested non-constant tuples produce pointer-typed fields in the outer struct, making the tuple LLVM type depend on constant-folding. This is a material type-instability bug. Normalize tuple elements to values before computing elem_tys and before stores. For tuple elements that lowered to an alloca (pointer to struct), load them first. Suggested patch (L. approx right after popping v from result_stack inside the for elem loop):

    def _tuple_element_as_value(self, v: ir.Value) -> ir.Value:
    """Normalize tuple element to a value (load if it is a tuple pointer)."""
    from llvmlite import ir
    if isinstance(v.type, ir.PointerType) and isinstance(getattr(v.type, "pointee", None), ir.LiteralStructType):
    return self._llvm.ir_builder.load(v, name="tuple.elem.load")
    return v

    usage after v is popped

    if isinstance(elem, astx.LiteralTuple): # ensure nested tuples are values
    v = self._tuple_element_as_value(v)

  • Safety/lifetime: Returning a pointer to a stack alloca as the non-constant result can easily escape if downstream passes/returns it without a load. If the contract must remain “pointer on non-constant,” add an assertion or enforce a load-at-boundary. Preferably, build an SSA struct value via insertvalue and return the value to avoid lifetime hazards. If you keep the current approach, at least add a load before pushing when the consumer expects a value (L. end of method, before self.result_stack.append).


raise Exception("LiteralTuple: failed to lower an element.")
llvm_vals.append(v)

n = len(llvm_vals)
Copy link
Contributor

@iihimanshuu iihimanshuu Dec 15, 2025

Choose a reason for hiding this comment

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

L1267: Variable n is assigned but never used after that. Just use len(llvm_vals) directly in the empty check or remove the variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

L1267: Variable n is assigned but never used after that. Just use len(llvm_vals) directly in the empty check or remove the variable.

Its a draft, will open for review once it is complete ! Thanks

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