Always shadow for-loop control variables in macro bodies

[JJH090501]

Problem

A macro whose body writes to the control variable of a for loop fails to
compile when the compiler itself runs on Lua 5.5:

local macro shout!(x: Expression)
   local out = block('statements')
   for entry in ("a;b"):gmatch("[^;]+") do
      entry = entry .. "!"
   end
   table.insert(out, `$x`)
   return out
end

shout!(print('hi'))

$ lua5.5 ./tl run macro_bug.tl
[string "shout"]:3: attempt to assign to const variable 'entry'

Numeric for in a macro body fails the same way. The file works on 5.1, 5.3
and 5.4 — 5.5 is the first version to make the control variable a constant.

Fix

Macro bodies keep targeting the 5.1 subset, as discussed. Instead of detecting
whether a body writes to a control variable, macro_eval now inserts the
shadowing local into every for in the macro body before parsing. local x = x
is valid in every supported target and is a no-op when nothing writes to it, so
no detection is needed and the traversal from the previous revision is gone.

Both forin and fornum are covered. 39 lines in teal/macro_eval.tl; no
other source file is touched.

Tests

spec/lang/macro/for_control_var_spec.lua adds three cases: a numeric for
body that writes its control variable, a generic for body that writes its
control variable, and a loop that never writes one. Each asserts the expanded
output rather than just that parsing succeeded, so the number of iterations the
macro body actually performed is encoded in the result.

Two of the three fail on Lua 5.5 without this change. On 5.1–5.4 they pass
either way, since those versions allow the assignment — which is also why CI
does not catch this today (lua-version: ["5.4", "5.3", "5.2", "5.1", "luajit"]).

make selfbuild is clean and busted --suppress-pending spec/ passes.

Related

Numeric for has the same problem outside macros: lua_compat.adjust_code has
no fornum case, so ordinary Teal code that writes to a numeric loop variable
type checks and then emits invalid Lua on 5.5. That is the issue @bjornbm ran
into in #1159; it reproduces on master without this change and needs
lua_compat and visitors changes, so it is out of scope here. Happy to take
it separately if nobody else is on it.

[hishamhm]

Thank you for looking into this!

Note: I used AI Assistant for English Comment and some test cases.

Thanks for disclosing. I am not keen on reviewing large AI-generated PRs (and their very long PR descriptions 😅 ), but I'm happy to review contributor code and answer questions.

Would you prefer the forin shadow logic extracted into a leaf module (depending only on teal.ast) so lua_compat and macro_eval can share it instead of duplicating? Happy to redo it that way.

This experimental macro feature still has some rough edges in the design. One of them is the Lua version that the macro bodies use: for now we assumed that 5.1 would be a usable "lowest common denominator". But if we start using the running Lua version, we risk that we'll start to have Teal modules including macros that only work in one specific Lua version.

So I think we should keep targeting a single Lua version, effectively forcing a "Lua subset" for macro code, like we already do for the standard library availability inside macros. This would make macro code stable across target Lua VMs.

For this specific problem of the for variables, I think the easiest approach here that does not involve all that additional traversal would be to simply always patch in the shadow variable in all for loops in macro_eval. Given the orthogonal shape of blocks, this should be possible without encoding lots of assumptions about the shape of the tree (that is, all those rules in lines 211-255 and 268-281.

[JJH090501]

Thanks for your detailed review! I was planning a backend framework for lua 5.3+ with Teal support.

I think I understand what you mean, and what i should do now. I'll simplify the implementation and keep the macro code compatible with the Lua 5.1 subset.

I'd appreciate one more chance to revise the PR based on your feedback. :)

[JJH090501]

Pushed the rewrite. The macro body still targets the 5.1 subset; the shadow is
now inserted unconditionally in every for in the body, both forin and
fornum, so the detection traversal is gone. 37 lines in
teal/macro_eval.tl plus three test cases — nothing else touched.

(Force-pushed, so the history changed. :))

[Frityet]

Hmm, I could see this continue to cause issues in the future, it is pretty clear that a blanket just "target lua 5.1" really won't work. I think we need to just target whatever Lua version that is running the macro.

@hishamhm Thoughts?