From baabc6013ea4f44082e69375214e76b5d803c5cb Mon Sep 17 00:00:00 2001
From: mlugg <mlugg@mlugg.co.uk>
Date: Fri, 10 Nov 2023 07:57:54 +0000
Subject: [PATCH] compiler: add error for unnecessary use of 'var'

When a local variable is never used as an lvalue, we can determine that
`const` would be sufficient for this variable, so emit an error in this
case. More sophisticated checking is unfortunately not possible with
Zig's current analysis model, since whether an lvalue is actually
mutated depends on semantic analysis, in which some code paths may not
be analyzed, so attempting to determine this would result in false
positive compile errors.

It's worth noting that an unfortunate consequence of this is that any
field call `a.b()` will allow `a` to be `var`, even if `b` does not take
a pointer as its first parameter - this is again a necessary compromise
because the parameter type is not known until semantic analysis.

Also update `translate-c` to not trigger these errors. This is done by
replacing the `_ = @TypeOf(x)` emitted with `_ = &x` - the reference
there means that the local is permitted to be `var`. A similar strategy
will be used to prevent compile errors in the behavior tests, where we
sometimes want to force a value to be runtime-known.

Resolves: #224
---
 src/AstGen.zig          | 28 +++++++++++++++++++++-------
 src/translate_c/ast.zig |  9 +++++++--
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/src/AstGen.zig b/src/AstGen.zig
index 5638216ed1..58ba3ee11c 100644
--- a/src/AstGen.zig
+++ b/src/AstGen.zig
@@ -1226,7 +1226,7 @@ fn awaitExpr(
             try astgen.errNoteNode(gz.suspend_node, "suspend block here", .{}),
         });
     }
-    const operand = try expr(gz, scope, .{ .rl = .none }, rhs_node);
+    const operand = try expr(gz, scope, .{ .rl = .ref }, rhs_node);
     const result = if (gz.nosuspend_node != 0)
         try gz.addExtendedPayload(.await_nosuspend, Zir.Inst.UnNode{
             .node = gz.nodeIndexToRelative(node),
@@ -1248,7 +1248,7 @@ fn resumeExpr(
     const tree = astgen.tree;
     const node_datas = tree.nodes.items(.data);
     const rhs_node = node_datas[node].lhs;
-    const operand = try expr(gz, scope, .{ .rl = .none }, rhs_node);
+    const operand = try expr(gz, scope, .{ .rl = .ref }, rhs_node);
     const result = try gz.addUnNode(.@"resume", operand, node);
     return rvalue(gz, ri, result, node);
 }
@@ -2941,11 +2941,19 @@ fn checkUsed(gz: *GenZir, outer_scope: *Scope, inner_scope: *Scope) InnerError!v
                 const s = scope.cast(Scope.LocalPtr).?;
                 if (s.used == 0 and s.discarded == 0) {
                     try astgen.appendErrorTok(s.token_src, "unused {s}", .{@tagName(s.id_cat)});
-                } else if (s.used != 0 and s.discarded != 0) {
-                    try astgen.appendErrorTokNotes(s.discarded, "pointless discard of {s}", .{@tagName(s.id_cat)}, &[_]u32{
-                        try gz.astgen.errNoteTok(s.used, "used here", .{}),
-                    });
+                } else {
+                    if (s.used != 0 and s.discarded != 0) {
+                        try astgen.appendErrorTokNotes(s.discarded, "pointless discard of {s}", .{@tagName(s.id_cat)}, &[_]u32{
+                            try astgen.errNoteTok(s.used, "used here", .{}),
+                        });
+                    }
+                    if (s.id_cat == .@"local variable" and !s.used_as_lvalue) {
+                        try astgen.appendErrorTokNotes(s.token_src, "local variable is never mutated", .{}, &.{
+                            try astgen.errNoteTok(s.token_src, "consider using 'const'", .{}),
+                        });
+                    }
                 }
+
                 scope = s.parent;
             },
             .defer_normal, .defer_error => scope = scope.cast(Scope.Defer).?.parent,
@@ -7579,7 +7587,10 @@ fn localVarRef(
                 );
 
                 switch (ri.rl) {
-                    .ref, .ref_coerced_ty => return ptr_inst,
+                    .ref, .ref_coerced_ty => {
+                        local_ptr.used_as_lvalue = true;
+                        return ptr_inst;
+                    },
                     else => {
                         const loaded = try gz.addUnNode(.load, ptr_inst, ident);
                         return rvalueNoCoercePreRef(gz, ri, loaded, ident);
@@ -10948,6 +10959,9 @@ const Scope = struct {
         /// Track the identifier where it is discarded, like this `_ = foo;`.
         /// 0 means never discarded.
         discarded: Ast.TokenIndex = 0,
+        /// Whether this value is used as an lvalue after inititialization.
+        /// If not, we know it can be `const`, so will emit a compile error if it is `var`.
+        used_as_lvalue: bool = false,
         /// String table index.
         name: u32,
         id_cat: IdCat,
diff --git a/src/translate_c/ast.zig b/src/translate_c/ast.zig
index 0381f58cf9..8330e6785f 100644
--- a/src/translate_c/ast.zig
+++ b/src/translate_c/ast.zig
@@ -1625,13 +1625,18 @@ fn renderNode(c: *Context, node: Node) Allocator.Error!NodeIndex {
             });
             const main_token = try c.addToken(.equal, "=");
             if (payload.value.tag() == .identifier) {
-                // Render as `_ = @TypeOf(foo);` to avoid tripping "pointless discard" error.
+                // Render as `_ = &foo;` to avoid tripping "pointless discard" and "local variable never mutated" errors.
+                var addr_of_pl: Payload.UnOp = .{
+                    .base = .{ .tag = .address_of },
+                    .data = payload.value,
+                };
+                const addr_of: Node = .{ .ptr_otherwise = &addr_of_pl.base };
                 return c.addNode(.{
                     .tag = .assign,
                     .main_token = main_token,
                     .data = .{
                         .lhs = lhs,
-                        .rhs = try renderBuiltinCall(c, "@TypeOf", &.{payload.value}),
+                        .rhs = try renderNode(c, addr_of),
                     },
                 });
             } else {