authorgravatar for mlugg@mlugg.co.ukMatthew Lugg <mlugg@mlugg.co.uk> 2023-11-10 07:57:54+00:00
committergravatar for mlugg@mlugg.co.ukMatthew Lugg <mlugg@mlugg.co.uk> 2023-11-19 09:55:07+00:00
logbaabc6013ea4f44082e69375214e76b5d803c5cb
tree00f89e2c48f72b581bedd7b2c8a068d28839368f
parent325e0f5f0e8a9ce2540ec3ec5b7cbbecac15257a
signature Commit is signed but in an unrecognized format.

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

2 files changed, 28 insertions(+), 9 deletions(-)

src/AstGen.zig+21-7
......@@ -1226,7 +1226,7 @@ fn awaitExpr(
12261226 try astgen.errNoteNode(gz.suspend_node, "suspend block here", .{}),
12271227 });
12281228 }
1229 const operand = try expr(gz, scope, .{ .rl = .none }, rhs_node);
1229 const operand = try expr(gz, scope, .{ .rl = .ref }, rhs_node);
12301230 const result = if (gz.nosuspend_node != 0)
12311231 try gz.addExtendedPayload(.await_nosuspend, Zir.Inst.UnNode{
12321232 .node = gz.nodeIndexToRelative(node),
......@@ -1248,7 +1248,7 @@ fn resumeExpr(
12481248 const tree = astgen.tree;
12491249 const node_datas = tree.nodes.items(.data);
12501250 const rhs_node = node_datas[node].lhs;
1251 const operand = try expr(gz, scope, .{ .rl = .none }, rhs_node);
1251 const operand = try expr(gz, scope, .{ .rl = .ref }, rhs_node);
12521252 const result = try gz.addUnNode(.@"resume", operand, node);
12531253 return rvalue(gz, ri, result, node);
12541254}
......@@ -2941,11 +2941,19 @@ fn checkUsed(gz: *GenZir, outer_scope: *Scope, inner_scope: *Scope) InnerError!v
29412941 const s = scope.cast(Scope.LocalPtr).?;
29422942 if (s.used == 0 and s.discarded == 0) {
29432943 try astgen.appendErrorTok(s.token_src, "unused {s}", .{@tagName(s.id_cat)});
2944 } else if (s.used != 0 and s.discarded != 0) {
2945 try astgen.appendErrorTokNotes(s.discarded, "pointless discard of {s}", .{@tagName(s.id_cat)}, &[_]u32{
2946 try gz.astgen.errNoteTok(s.used, "used here", .{}),
2947 });
2944 } else {
2945 if (s.used != 0 and s.discarded != 0) {
2946 try astgen.appendErrorTokNotes(s.discarded, "pointless discard of {s}", .{@tagName(s.id_cat)}, &[_]u32{
2947 try astgen.errNoteTok(s.used, "used here", .{}),
2948 });
2949 }
2950 if (s.id_cat == .@"local variable" and !s.used_as_lvalue) {
2951 try astgen.appendErrorTokNotes(s.token_src, "local variable is never mutated", .{}, &.{
2952 try astgen.errNoteTok(s.token_src, "consider using 'const'", .{}),
2953 });
2954 }
29482955 }
2956
29492957 scope = s.parent;
29502958 },
29512959 .defer_normal, .defer_error => scope = scope.cast(Scope.Defer).?.parent,
......@@ -7579,7 +7587,10 @@ fn localVarRef(
75797587 );
75807588
75817589 switch (ri.rl) {
7582 .ref, .ref_coerced_ty => return ptr_inst,
7590 .ref, .ref_coerced_ty => {
7591 local_ptr.used_as_lvalue = true;
7592 return ptr_inst;
7593 },
75837594 else => {
75847595 const loaded = try gz.addUnNode(.load, ptr_inst, ident);
75857596 return rvalueNoCoercePreRef(gz, ri, loaded, ident);
......@@ -10948,6 +10959,9 @@ const Scope = struct {
1094810959 /// Track the identifier where it is discarded, like this `_ = foo;`.
1094910960 /// 0 means never discarded.
1095010961 discarded: Ast.TokenIndex = 0,
10962 /// Whether this value is used as an lvalue after inititialization.
10963 /// If not, we know it can be `const`, so will emit a compile error if it is `var`.
10964 used_as_lvalue: bool = false,
1095110965 /// String table index.
1095210966 name: u32,
1095310967 id_cat: IdCat,
src/translate_c/ast.zig+7-2
......@@ -1625,13 +1625,18 @@ fn renderNode(c: *Context, node: Node) Allocator.Error!NodeIndex {
16251625 });
16261626 const main_token = try c.addToken(.equal, "=");
16271627 if (payload.value.tag() == .identifier) {
1628 // Render as `_ = @TypeOf(foo);` to avoid tripping "pointless discard" error.
1628 // Render as `_ = &foo;` to avoid tripping "pointless discard" and "local variable never mutated" errors.
1629 var addr_of_pl: Payload.UnOp = .{
1630 .base = .{ .tag = .address_of },
1631 .data = payload.value,
1632 };
1633 const addr_of: Node = .{ .ptr_otherwise = &addr_of_pl.base };
16291634 return c.addNode(.{
16301635 .tag = .assign,
16311636 .main_token = main_token,
16321637 .data = .{
16331638 .lhs = lhs,
1634 .rhs = try renderBuiltinCall(c, "@TypeOf", &.{payload.value}),
1639 .rhs = try renderNode(c, addr_of),
16351640 },
16361641 });
16371642 } else {