From de6dcc5b7991d993e5e866f462852ffa36b15ecf Mon Sep 17 00:00:00 2001 From: MineRobber9000 Date: Tue, 7 Jul 2026 17:37:26 -0400 Subject: [PATCH 1/2] Add check to handle recursive definitions (like "x=[x]") correctly --- cs/CodeGenerator.cs | 65 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 2 deletions(-) diff --git a/cs/CodeGenerator.cs b/cs/CodeGenerator.cs index 66866e2..3cdaab7 100644 --- a/cs/CodeGenerator.cs +++ b/cs/CodeGenerator.cs @@ -352,8 +352,29 @@ public Int32 Visit(AssignmentNode node) { // Get or allocate register for this variable Int32 varReg; + // hold the register for the variable in the recursive case (-1 = no need) + Int32 pendingReg = -1; if (_variableRegs.TryGetValue(node.Variable, out varReg)) { - // Variable already has a register - reuse it + // In the case of lists and maps, it's possible to define a variable which + // relies on the variable's prior value; for example, ensureImport from the + // importUtil library has the code: + // if moduleName isa string then moduleName = [moduleName] + // In that case and that case alone, we need to allocate a temporary register + // for the new value of the variable and then copy the new value into the right + // register. + // Note that a similar case does not need to be added for IndexedAssignmentNode + // since that allocates a new register for the value in all cases. + if (CheckForRecursiveDefinition(node.Variable, node.Value)) { + // Check and make sure this isn't something like "x = x" + // (we only need to check if the node is an IdentifierNode, since an + // IdentifierNode with any other name wouldn't trigger the recursive + // definition check) + IdentifierNode valueIdNode = node.Value as IdentifierNode; + if (valueIdNode == null) { + pendingReg = varReg; + varReg = AllocReg(); + } + } } else { // Hmm. Should we allocate a new register for this variable, or // just claim the target register as our storage? I'm going to alloc @@ -367,7 +388,7 @@ public Int32 Visit(AssignmentNode node) { // on every path that assigns the variable, including conditional branches // (e.g. the else clause of a single-line if), or the variable would be // undefined at runtime when only that path executes. - EnsureNamed(node.Variable, varReg); + if (pendingReg == -1) EnsureNamed(node.Variable, varReg); // If the RHS is a function expression, note the current function count so we // can assign the variable name to the resulting FuncDef afterward. FunctionNode rhsFunc = node.Value as FunctionNode; @@ -381,6 +402,16 @@ public Int32 Visit(AssignmentNode node) { if (rhsFuncDef != null) rhsFuncDef.Name = node.Variable; } + if (pendingReg != -1) { + // We allocated a temporary register earlier, so now we need to finish the + // assignment. Load the variable's actual register with the value of the + // temporary register and then free the temporary register to be used later. + _emitter.EmitABC(Opcode.LOAD_rA_rB, pendingReg, varReg, + 0, $"r{pendingReg} = r{varReg}"); + FreeReg(varReg); + varReg = pendingReg; + } + // Note that we don't FreeReg(varReg) here, as we need this register to // continue to serve as the storage for this variable for the life of // the function. (TODO: or until some lifetime analysis determines that @@ -392,6 +423,36 @@ public Int32 Visit(AssignmentNode node) { return varReg; } + private Boolean CheckForRecursiveDefinition(String variableName, ASTNode node) { + // IdentifierNode: is it the right variable? + IdentifierNode identifierNode = node as IdentifierNode; + if (identifierNode != null) { + return identifierNode.Name == variableName; + } + + // ListNode: check every item in the list + ListNode listNode = node as ListNode; + if (listNode != null) { + for (Int32 i = 0; i < listNode.Elements.Count; ++i) { + if (CheckForRecursiveDefinition(variableName, listNode.Elements[i])) return true; + } + return false; + } + + // MapNode: check all keys and values in the map + MapNode mapNode = node as MapNode; + if (mapNode != null) { + for (Int32 i = 0; i < mapNode.Keys.Count; ++i) { + if (CheckForRecursiveDefinition(variableName, mapNode.Keys[i])) return true; + if (CheckForRecursiveDefinition(variableName, mapNode.Values[i])) return true; + } + return false; + } + + // All other cases: return false. + return false; + } + public Int32 Visit(IndexedAssignmentNode node) { Int32 containerReg = node.Target.Accept(this); Int32 indexReg = node.Index.Accept(this); From b32faf1e1a9ec479e25d773f5180e469b4d8d82b Mon Sep 17 00:00:00 2001 From: MineRobber9000 Date: Wed, 8 Jul 2026 01:45:31 -0400 Subject: [PATCH 2/2] Fix recursive definitions in all cases, not just ones where the variable already exists --- cs/CodeGenerator.cs | 54 ++++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/cs/CodeGenerator.cs b/cs/CodeGenerator.cs index 3cdaab7..49a7d05 100644 --- a/cs/CodeGenerator.cs +++ b/cs/CodeGenerator.cs @@ -352,29 +352,8 @@ public Int32 Visit(AssignmentNode node) { // Get or allocate register for this variable Int32 varReg; - // hold the register for the variable in the recursive case (-1 = no need) - Int32 pendingReg = -1; if (_variableRegs.TryGetValue(node.Variable, out varReg)) { - // In the case of lists and maps, it's possible to define a variable which - // relies on the variable's prior value; for example, ensureImport from the - // importUtil library has the code: - // if moduleName isa string then moduleName = [moduleName] - // In that case and that case alone, we need to allocate a temporary register - // for the new value of the variable and then copy the new value into the right - // register. - // Note that a similar case does not need to be added for IndexedAssignmentNode - // since that allocates a new register for the value in all cases. - if (CheckForRecursiveDefinition(node.Variable, node.Value)) { - // Check and make sure this isn't something like "x = x" - // (we only need to check if the node is an IdentifierNode, since an - // IdentifierNode with any other name wouldn't trigger the recursive - // definition check) - IdentifierNode valueIdNode = node.Value as IdentifierNode; - if (valueIdNode == null) { - pendingReg = varReg; - varReg = AllocReg(); - } - } + // Variable already has a register - reuse it } else { // Hmm. Should we allocate a new register for this variable, or // just claim the target register as our storage? I'm going to alloc @@ -382,12 +361,35 @@ public Int32 Visit(AssignmentNode node) { // the target register when done. But we should probably return to // this later and see if we can optimize it more. varReg = AllocReg(); - _variableRegs[node.Variable] = varReg; + // Don't set it in _variableRegs just yet. + } + // In the case of lists and maps, it's possible to define a variable which + // relies on the variable's prior value; for example, ensureImport from the + // importUtil library has the code: + // if moduleName isa string then moduleName = [moduleName] + // In that case and that case alone, we need to allocate a temporary register + // for the new value of the variable and then copy the new value into the right + // register. + // Note that a similar case does not need to be added for IndexedAssignmentNode + // since that allocates a new register for the value in all cases. + // Holds the register for the variable in the recursive case (-1 = no need) + Int32 pendingReg = -1; + if (CheckForRecursiveDefinition(node.Variable, node.Value)) { + // Check and make sure this isn't something like "x = x" + // (we only need to check if the node is an IdentifierNode, since an + // IdentifierNode with any other name wouldn't trigger the recursive + // definition check) + IdentifierNode valueIdNode = node.Value as IdentifierNode; + if (valueIdNode == null) { + pendingReg = varReg; + varReg = AllocReg(); + } } // Emit a NAME op unless one already dominates this point. This must run // on every path that assigns the variable, including conditional branches // (e.g. the else clause of a single-line if), or the variable would be // undefined at runtime when only that path executes. + // Defer this line if we're doing a recursive definition. if (pendingReg == -1) EnsureNamed(node.Variable, varReg); // If the RHS is a function expression, note the current function count so we // can assign the variable name to the resulting FuncDef afterward. @@ -406,12 +408,18 @@ public Int32 Visit(AssignmentNode node) { // We allocated a temporary register earlier, so now we need to finish the // assignment. Load the variable's actual register with the value of the // temporary register and then free the temporary register to be used later. + EnsureNamed(node.Variable, pendingReg); _emitter.EmitABC(Opcode.LOAD_rA_rB, pendingReg, varReg, 0, $"r{pendingReg} = r{varReg}"); FreeReg(varReg); varReg = pendingReg; } + // If the variable didn't exist before, now it does. + if (!_variableRegs.ContainsKey(node.Variable)) { + _variableRegs[node.Variable] = varReg; + } + // Note that we don't FreeReg(varReg) here, as we need this register to // continue to serve as the storage for this variable for the life of // the function. (TODO: or until some lifetime analysis determines that