Skip to content

closureiters: fix undeclared C identifier when using template/macro bindings in async and/or second operand#25761

Open
emizzle wants to merge 1 commit intonim-lang:develfrom
emizzle:fix/closureiters/hoist-second-statement
Open

closureiters: fix undeclared C identifier when using template/macro bindings in async and/or second operand#25761
emizzle wants to merge 1 commit intonim-lang:develfrom
emizzle:fix/closureiters/hoist-second-statement

Conversation

@emizzle
Copy link
Copy Markdown

@emizzle emizzle commented Apr 22, 2026

Summary

lowerStmtListExprs handles the second operand of and/or in closure iterators (i.e., async procs) differently from the first: when the second operand is a nkStmtListExpr (produced by template/macro expansion or await), its statements were placed in ifBody — the body of the short-circuit if block — instead of the outer result node.

This meant that variables declared in those statements (e.g., via an injected let from a template like bindOpt) were scoped to the short-circuit block and inaccessible in the enclosing if body, producing "use of undeclared identifier" errors at the C level.

The fix mirrors how the first operand is already handled (line 661): hoist the statements to result so they are visible in the enclosing scope. The expression part (ex) remains in ifBody and continues to be properly short-circuited. Plain (non-stmtListExpr) second operands are unaffected.

Note: as a consequence, side-effectful statements in the second operand (e.g., variable declarations from template expansion) are no longer short-circuited. This only affects patterns that previously failed to compile.

Minimal reproducible example

import std/asyncdispatch, std/options

template bindOpt(name: untyped; expr: untyped): bool =
  let opt = expr
  let name {.inject.} = if opt.isSome: opt.get() else: 0
  opt.isSome

proc test() {.async.} =
  proc getA(): Future[Option[int]] {.async.} = return some(1)
  proc getB(): Option[int] = some(2)

  # Before this fix, `b` was "undeclared identifier" at C compilation:
  if bindOpt(a, await getA()) and bindOpt(b, getB()):
    doAssert a == 1
    doAssert b == 2

waitFor test()

Changes

  • compiler/closureiters.nim: change ifBody.add(st) to result.add(st) for the second operand of and/or, mirroring the existing first-operand handling
  • tests/async/tasync_and_stmtlistexpr.nim: regression test covering the fixed bug, plus two tests confirming that plain (non-stmtListExpr) second operands remain properly short-circuited
  • changelog.md: entry added under "Compiler changes"

…indings in async and/or second operand

`lowerStmtListExprs` handles the second operand of `and`/`or` in closure
iterators (i.e., async procs) differently from the first: when the second
operand is a `nkStmtListExpr` (produced by template/macro expansion or
`await`), its statements were placed in `ifBody` — the body of the
short-circuit `if` block — instead of the outer `result` node.

This meant that variables declared in those statements (e.g., via an
injected `let` from a template like `bindOpt`) were scoped to the
short-circuit block and inaccessible in the enclosing `if` body, producing
"use of undeclared identifier" errors at the C level.

The fix mirrors how the first operand is already handled (line 661): hoist
the statements to `result` so they are visible in the enclosing scope. The
expression part (`ex`) remains in `ifBody` and continues to be properly
short-circuited. Plain (non-stmtListExpr) second operands are unaffected.

Note: as a consequence, side-effectful *statements* in the second operand
(e.g., variable declarations from template expansion) are no longer
short-circuited. This only affects patterns that previously failed to
compile.
@Araq
Copy link
Copy Markdown
Member

Araq commented Apr 22, 2026

Note: as a consequence, side-effectful statements in the second operand
(e.g., variable declarations from template expansion) are no longer
short-circuited. This only affects patterns that previously failed to compile.

Unacceptable. Instead keep the "it fails to compile" state.

@emizzle
Copy link
Copy Markdown
Author

emizzle commented Apr 22, 2026

Unacceptable. Instead keep the "it fails to compile" state.

It's a trade off. "Unacceptable" seems more like a preference/opinion.

@emizzle
Copy link
Copy Markdown
Author

emizzle commented Apr 22, 2026

Perhaps this would be more acceptable if it was only enabled with a feature flag?

@Araq
Copy link
Copy Markdown
Member

Araq commented Apr 22, 2026

and/or operators must be short-circuited.

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