From 5337ba48a4ce647029b3ae1aadd59c790eccf35c Mon Sep 17 00:00:00 2001
From: Fernando Araoz <fernando@araozu.dev>
Date: Mon, 16 Dec 2024 07:10:06 -0500
Subject: [PATCH] fix: prevent memory leaks in module parser

---
 src/02_syntax/root.zig     | 28 ++++++++++++++++++----------
 src/02_syntax/variable.zig |  3 +--
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/src/02_syntax/root.zig b/src/02_syntax/root.zig
index edb9b7b..2a2c3a3 100644
--- a/src/02_syntax/root.zig
+++ b/src/02_syntax/root.zig
@@ -17,36 +17,32 @@ pub const Module = struct {
     pub fn init(target: *@This(), tokens: *const TokenStream, pos: usize, allocator: std.mem.Allocator) ParseError!void {
         var arrl = std.ArrayList(*statement.Statement).init(allocator);
         errdefer arrl.deinit();
+        errdefer for (arrl.items) |i| {
+            i.deinit();
+            allocator.destroy(i);
+        };
 
         const input_len = tokens.items.len;
         var current_pos = pos;
 
         // parse many statements
         while (current_pos < input_len) {
-            // FIXME: if a statement was added to the array list,
-            // and then one of these fails,
-            // will all previous statements leak memory?
             var stmt = try allocator.create(statement.Statement);
             errdefer allocator.destroy(stmt);
+
             const next_pos = try stmt.init(tokens, current_pos, allocator);
             current_pos = next_pos;
 
-            arrl.append(stmt) catch {
-                // TODO: free stmt if this fails
-                return ParseError.Error;
-            };
+            try arrl.append(stmt);
         }
 
         target.* = .{
-            // FIXME: is this copying the whole arraylist? should use a pointer?
             .statements = arrl,
             .alloc = allocator,
         };
     }
 
     pub fn deinit(self: @This()) void {
-        // FIXME: should deinit all elements inside the arraylist no? otherwise
-        // they will leak no?
         for (self.statements.items) |stmt| {
             stmt.deinit();
             self.alloc.destroy(stmt);
@@ -69,3 +65,15 @@ test "should parse a single statement" {
 
     defer module.deinit();
 }
+
+test "should clean memory if a statement parsing fails after one item has been inserted" {
+    const input = "var my_variable = 322 unrelated()";
+    const tokens = try lexic.tokenize(input, std.testing.allocator);
+    defer tokens.deinit();
+
+    var module: Module = undefined;
+    _ = module.init(&tokens, 0, std.testing.allocator) catch {
+        return;
+    };
+    defer module.deinit();
+}
diff --git a/src/02_syntax/variable.zig b/src/02_syntax/variable.zig
index de8d5dc..53704ce 100644
--- a/src/02_syntax/variable.zig
+++ b/src/02_syntax/variable.zig
@@ -43,7 +43,6 @@ pub const VariableBinding = struct {
         // parse expression
         if (pos + 3 >= tokens.items.len) return ParseError.Error;
 
-        // TODO: allocate on the stack
         const exp = allocator.create(expression.Expression) catch {
             return ParseError.OutOfMemory;
         };
@@ -103,7 +102,7 @@ test "should fail is it doesnt start with var" {
     try std.testing.expect(false);
 }
 
-test "should fail if the idenfier is missing" {
+test "should fail if the identifier is missing" {
     const input = "var ";
     const tokens = try lexic.tokenize(input, std.testing.allocator);
     defer tokens.deinit();