Rule Idea Board

[DonIsaac]

This is a mega-issue tracking the list of rules we want to implement. Rule requests are welcome.

1. no-undefined - ban undefined in assignments and initializations

• Allow when a // SAFETY: comment is present
• Allow self.* = undefined in deinit() functions

2. unsafe-optional-unwrap: bad optional unwrapping without a prior null check
3. use-after-deinit: ban references to foo after foo.deinit() calls (or other free-like calls)
4. no-const-reassign: do not reassign const-declared variables. Yes the compiler checks this, no they don't check it for tree-shaken code.
5. no-private-access: ban external references to _private container fields.

[DonIsaac]

I don't know what to call this, but @paperdave proposed it here

[DonIsaac]

Here's his full list (it needs triage)

[paperclover]

I don't know what to call this

"declaration can be accessed with a shorter path" / shorter_decl_path

[DonIsaac]

@paperdave decl-alias?

[DonIsaac]

unused-array-side-effects: Accidental mutation on a copied array

fn addEntry(self: *Foo, id: u32, item: u32) !void {
  // this creates a copy. It's dropped when the function returns
  var foos = self.foo[id]; // should be &self.foo[id];
  try foos.append(item);   // has no lasting side effects b/c foos is dropped
}

[MichaelBelousov]

unsafe-union-unwrap/check-union-payload-refs:

like unsafe-option-unwrap but enforces that a direct union variant access occurs in a scope where the union tag was checked and matches.

var a: union(enum) { int: i32, float: f32 } = .X;

if (a == .float) {
  std.debug.print("float: {}", .{a.float}); // legal
} else {
  std.debug.print("int: {}", .{a.float}); // illegal!
}

switch (a) {
  .int => std.debug.print("int: {}", .{a.float}), // illegal
}

[paperclover]

naming-convention: enforce standard library naming conventions declarations

[paperclover]

but enforces that a direct union variant access occurs in a scope where the union tag was checked and matches.

it's better to avoid this pattern entirely, if possible. these rules should encourage switch and captures:

var a: union(enum) { int: i32, float: f32 } = .X;

if (a == .float) {
  std.debug.print("float: {}", .{a.float}); // illegal
} else {
  std.debug.print("int: {}", .{a.float}); // illegal
}

// rewrite into
switch (a) {
  .float => |float| std.debug.print("float: {}", .{a.float}),
  .int => |int| std.debug.print("int: {}", .{a.int}),
}

[MichaelBelousov]

it's better to avoid this pattern entirely, if possible. these rules should encourage switch and captures

I agree in general, but it's the same reason to have unsafe-option-unwrap even when if (optional) |capture| exists.

There is code when you're checking multiple conditions and only one union variant where a switch may be cumbersome:

if (condition and a == .float) {
  std.debug.print("{}", .{a.float});
}

// versus

if (condition) switch (a) {
  .float => |v| {
    std.debug.print("{}", .{v});
  },
  else => {},
}

probably both unsafe-*-unwrap rules should mention the capture equivalents since those will also fix the rule (as opposed to making sure you access in a scope that checks the tag)

[DonIsaac]

but enforces that a direct union variant access occurs in a scope where the union tag was checked and matches.

it's better to avoid this pattern entirely, if possible. these rules should encourage switch and captures:

var a: union(enum) { int: i32, float: f32 } = .X;



if (a == .float) {

  std.debug.print("float: {}", .{a.float}); // illegal

} else {

  std.debug.print("int: {}", .{a.float}); // illegal

}



// rewrite into

switch (a) {

  .float => |float| std.debug.print("float: {}", .{a.float}),

  .int => |int| std.debug.print("int: {}", .{a.int}),

}

Should we add a rule banning it entirely?

[MichaelBelousov]

Should we add a rule banning it entirely?

Sure but I'd recommend allowing it with checks as the default.

It comes up often enough imo. A better example that I should have shown earlier is try checking that two separate variables have specific tags. You'd need a nested double switch if you don't allow tags checking in a single if statement

try rewriting this as a switch:

if (a == .float and b == .int) {
}

[paperclover]

i think we should split these into separate issues. this thread is getting very long. one issue per lint rule, or for related ones they can share one.

[nektro]

https://github.com/nektro/ziglint/issues has a bunch i havent implemented

[michaelbartnett]

Variant on unsafe-optional-unwrap:

Ban nullable.?, require people spell out nullable orelse unreachable if they aren't going to properly do a safe if-with-capture or nullable orelse <valid default expr>.

Though reading your description of it, if you're talking about doing a typescript/c# style analysis where you're able to confirm a prior null check passed, then allowing .? if that is satisfied would be pretty nice. Not sure that's in scope for a linter though.

[felipeasimos]

I just opened a draft PR to implement a line length check rule (#232 ). I just realized I jumped the gun an didn't bring the idea here first (sorry).

The idea is to just check if any line surpasses a given threshold. Currently the only thing left to implement is to let the user define this threshold through the config (for now 120 is hardcoded).

Is this rule a good fit for the project?

[felipeasimos]

Would checking for empty .zig files be a valid idea? I'm not sure how zlint discover files. If its through imports it wouldn't really make sense. However, I have seen projects where empty .zig files were left in the code so it would be useful

[DonIsaac]

Would checking for empty .zig files be a valid idea? I'm not sure how zlint discover files. If its through imports it wouldn't really make sense. However, I have seen projects where empty .zig files were left in the code so it would be useful

Yes

[felipeasimos]

From time to time I stumble in some comments in the zig community regarding anytype negatively. One of the most valid points being that it doesn't make the intent behind the code explicit, which kinda goes against the "easy to read" and "no hidden flow" objectives in the language itself. Maybe some people would find it useful having a no-anytype rule?

EDIT: to clarify the idea further, the rule would work like unsafe-undefined, forcing developers to clarify the use of anytype with a comment.