feat: enhance safe_pop error diagnostics with context

[Gitjay11]

fixes: #145

This PR enhances the safe_pop() utility in llvmliteir.py to provide better error diagnostics during the LLVM IR generation process. Previously, safe_pop() would silently return None on an empty stack, which made it difficult to trace which AST visitor failed to push a required value. This change implements "Fail Fast" behavior by allowing an optional context string that triggers a descriptive IndexError upon stack underflow.

Key Changes:

1. Updated safe_pop() to support a context parameter.
2. Replaced all direct self.result_stack.pop() calls with context-aware safe_pop() calls.
3. Added descriptive context strings to all callers (e.g., "WhileStmt condition", "BinaryOp (lhs)").
4. Standardized error handling in the FunctionReturn and Block visitors and corrected indentation.

This PR is a:

• bug-fix
• maintenance

Additional information:

By making this change, developers will see exactly which AST node failed to produce a result on the stack, cutting down debugging time significantly for visitor implementation bugs.

[github-actions]

OSL ChatGPT Reviewer

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

src/irx/builders/llvmliteir.py

• Bug: Block visitor now loses the last computed value if a later node doesn’t push to the stack. Previously, the last non-empty result was preserved; now it gets overwritten with None. This will drop values and can break downstream code expecting the block’s result. Fix by only updating result when a pop actually returned a value (L.852):
  tmp: Optional[ir.Value] = safe_pop(self.result_stack)
  if tmp is not None:
  result = tmp

• Potential breaking behavior: safe_pop now raises IndexError when context is provided. Callers that previously relied on None and raised domain-specific Exceptions won’t execute those paths anymore. If you want consistent compiler error types, consider catching IndexError at the outer layer and rethrowing your domain-specific exception.

---

[github-actions]

OSL ChatGPT Reviewer

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

src/irx/builders/llvmliteir.py

• High risk: @TypeChecked will deep-validate the entire list argument on every safe_pop call because of the annotation list[ir.Value | ir.Function]. If result_stack ever contains placeholders (e.g., None or other helper types), this can raise TypeError before popping and also adds O(n) work per pop in hot code paths. Relax the parameter type to avoid deep checks and validate only the popped value. Also add the missing import for Any. (L.70)
  Suggested change:
  from typing import Any

  def safe_pop(lst: list[Any], context: str = "") -> Optional[ir.Value | ir.Function]:
  """safe_pop: pop from result stack with optional context and type check."""
  try:
  val = lst.pop()
  except IndexError:
  if context:
  raise IndexError(f"Popping from an empty stack: {context}")
  return None
  if val is not None and not isinstance(val, (ir.Value, ir.Function)):
  raise TypeError(f"Unexpected stack value type {type(val)!r} at {context or 'safe_pop'}")
  return val

---

[github-actions]

OSL ChatGPT Reviewer

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

src/irx/builders/llvmliteir.py

• safe_pop annotation uses Any but it’s not imported. This can raise at import time unless you have from future import annotations. Add: from typing import Any (L.1).

• get_function: you still accept any ir.Value from the stack. Enforce ir.Function to avoid latent misuses.
  Suggestion (replace the return statement) (L.295):
  def get_function(self, name: str) -> Optional[ir.Function]:
  """Ensure the popped value is an ir.Function."""
  ...
  fn = safe_pop(self.result_stack, "get_function")
  if not isinstance(fn, ir.Function):
  raise TypeError(f"Expected ir.Function, got {type(fn)!r}")
  return fn

• safe_pop currently allows a real None value to pass through, which will lead to AttributeError in places that dereference .type (e.g., UnaryOp "++"/"--"). If None should never be on the result stack, reject it explicitly when context is provided (L.79):
  def safe_pop(lst: list[Any], context: str = "") -> Optional[ir.Value | ir.Function]:
  """Reject None values on the stack when context is provided."""
  ...
  if val is None and context:
  raise TypeError(f"Unexpected None on stack at {context or 'safe_pop'}")

• The new behavior of safe_pop raising IndexError when context is provided changes prior semantics (previously returned None). This may surface IndexError from many visitors instead of your domain-specific checks. If that’s not intended, consider using a custom CompilerStackUnderflow or keep returning None with contextual error raised by the caller (L.70).

• visit(Block): you bypass the new type check by popping directly. Consider using safe_pop(self.result_stack) to benefit from type validation and preserve the previous “no-raise-on-empty” behavior (L.858).

---

[iihimanshuu]

Looks good to me!

[github-actions]

OSL ChatGPT Reviewer

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

src/irx/builders/llvmliteir.py

• Block result propagation regression: result is overwritten with None when a node doesn’t push, losing the last valid value. This changes previous behavior and can break semantics of expression blocks. Only update result when a value was actually popped. (L.865)
  Suggested change:
  def visit(self, block: astx.Block) -> None:
  """Translate ASTx Block"""
  result: Optional[ir.Value | ir.Function] = None
  for node in block.nodes:
  self.visit(node)
  v = safe_pop(self.result_stack)
  if v is not None:
  result = v
  if result is not None:
  self.result_stack.append(result)

• Initialization risk: removing llvm.initialize() may cause runtime init issues on some llvmlite versions/platforms (it historically ensured native target/asmprinter are set up). Consider restoring it unless you’ve verified all supported environments with the current init sequence. (L.206)

---

[Gitjay11]

hi @xmnlab @yuvimittal , I’d appreciate your feedback whenever you’re available. Thanks

[yuvimittal]

@Gitjay11 , the CI seems to fail, could you please look into it!
I am converting it to draft, so you can fix it and ping again i will review it

[Gitjay11]

hi @yuvimittal , All the checks have passed. you can review it now, Thankyou.

[github-actions]

OSL ChatGPT Reviewer

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

workflows/main.yaml

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.', 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

src/irx/builders/llvmliteir.py

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.', 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

tests/test_llvmlite_helpers.py

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.', 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

[yuvimittal]

@Gitjay11 , please dont make any changes in .yaml file

[yuvimittal]

don't make changes here

[yuvimittal]

i didnt understand why you removed cast!?
cast and safe_pop serve totally different purpose