Skip to content

Commit

Permalink
feat(linter): add homeless-try lint rule (#105)
Browse files Browse the repository at this point in the history
`homeless-try` looks for `try` used outside of error union-returning
functions.

Part of #3
  • Loading branch information
DonIsaac authored Nov 22, 2024
1 parent 247e526 commit 32ea1ed
Show file tree
Hide file tree
Showing 13 changed files with 257 additions and 13 deletions.
12 changes: 12 additions & 0 deletions src/linter/lint_context.zig
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ pub fn ast(self: *const Context) *const Ast {
return &self.semantic.ast;
}

pub inline fn scopes(self: *const Context) *const Semantic.ScopeTree {
return &self.semantic.scopes;
}

pub inline fn symbols(self: *const Context) *const Semantic.SymbolTable {
return &self.semantic.symbols;
}

pub inline fn links(self: *const Context) *const Semantic.NodeLinks {
return &self.semantic.node_links;
}

// ============================ ERROR REPORTING ============================

pub fn spanN(self: *const Context, node_id: Ast.Node.Index) LabeledSpan {
Expand Down
1 change: 1 addition & 0 deletions src/linter/rules.zig
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
pub const NoUndefined = @import("./rules/no_undefined.zig");
pub const NoUnresolved = @import("./rules/no_unresolved.zig");
pub const HomelessTry = @import("./rules/homeless_try.zig");
191 changes: 191 additions & 0 deletions src/linter/rules/homeless_try.zig
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
//! ## What This Rule Does
//! Checks for `try` statements used outside of error-returning functions.
//!
//! As a `compiler`-level lint, this rule checks for errors also caught by the
//! Zig compiler.
//!
//! ## Examples
//! Examples of **incorrect** code for this rule:
//! ```zig
//! const std = @import("std");
//!
//! var not_in_a_function = try std.heap.page_allocator.alloc(u8, 8);
//!
//! fn foo() void {
//! var my_str = try std.heap.page_allocator.alloc(u8, 8);
//! }
//!
//! fn bar() !void {
//! const Baz = struct {
//! property: u32 = try std.heap.page_allocator.alloc(u8, 8),
//! };
//! }
//!```
//!
//! Examples of **correct** code for this rule:
//! ```zig
//! fn foo() !void {
//! var my_str = try std.heap.page_allocator.alloc(u8, 8);
//! }
//!```

const std = @import("std");
const util = @import("util");
const _source = @import("../../source.zig");
const semantic = @import("../../semantic.zig");
const _rule = @import("../rule.zig");

const Ast = std.zig.Ast;
const Node = Ast.Node;
const Symbol = semantic.Symbol;
const Scope = semantic.Scope;
const Loc = std.zig.Loc;
const Span = _source.Span;
const LinterContext = @import("../lint_context.zig");
const Rule = _rule.Rule;
const NodeWrapper = _rule.NodeWrapper;

// Rule metadata
const HomelessTry = @This();
pub const meta: Rule.Meta = .{
.name = "homeless-try",
.category = .compiler,
};

const CONTAINER_FLAGS: Scope.Flags = .{ .s_struct = true, .s_enum = true, .s_union = true };

// Runs on each node in the AST. Useful for syntax-based rules.
pub fn runOnNode(_: *const HomelessTry, wrapper: NodeWrapper, ctx: *LinterContext) void {
const scope_flags: []const Scope.Flags = ctx.scopes().scopes.items(.flags);

const node = wrapper.node;
if (node.tag != Node.Tag.@"try") return;

const curr_scope = ctx.links().scopes.items[wrapper.idx];
var it = ctx.scopes().iterParents(curr_scope);

while (it.next()) |scope| {
const flags = scope_flags[scope.int()];
const is_function = flags.s_function;
const is_block = flags.s_block;
if (is_function and !is_block) {
checkFnDecl(ctx, scope, wrapper.idx);
return;
}
if (flags.intersects(CONTAINER_FLAGS)) break;
}

ctx.diagnostic(
"`try` cannot be used outside of a function.",
.{ctx.spanT(ctx.ast().firstToken(wrapper.idx))},
);
}
fn checkFnDecl(ctx: *LinterContext, scope: Scope.Id, try_node: Node.Index) void {
const tags: []const Node.Tag = ctx.ast().nodes.items(.tag);
const decl_node = ctx.scopes().scopes.items(.node)[scope.int()];

if (tags[decl_node] != .fn_decl) {
if (comptime util.IS_DEBUG) {
util.assert(false, "function-bound scopes (w/o .s_block) should be bound to a function declaration node.", .{});
} else {
return;
}
}

const return_type: Node.Index = blk: {
var buf: [1]Node.Index = undefined;
const proto: Ast.full.FnProto = ctx.ast().fullFnProto(&buf, decl_node) orelse @panic(".fn_decl nodes always have a full fn proto available.");
break :blk proto.ast.return_type;
};
switch (tags[return_type]) {
// valid
.error_union => return,
else => {
const tok_tags: []const std.zig.Token.Tag = ctx.ast().tokens.items(.tag);
const prev_tok = ctx.ast().firstToken(return_type) - 1;
if (tok_tags[prev_tok] == .bang) return;
},
}

ctx.diagnostic(
"`try` cannot be used in functions that do not return errors.",
.{ctx.spanT(ctx.ast().firstToken(try_node))},
);
}

// Used by the Linter to register the rule so it can be run.
pub fn rule(self: *HomelessTry) Rule {
return Rule.init(self);
}

const RuleTester = @import("../tester.zig");
test HomelessTry {
const t = std.testing;

var homeless_try = HomelessTry{};
var runner = RuleTester.init(t.allocator, homeless_try.rule());
defer runner.deinit();

// Code your rule should pass on
const pass = &[_][:0]const u8{
\\const std = @import("std");
\\fn foo() std.mem.Allocator.Error!void {
\\ _ = try std.heap.page_allocator.alloc(u8, 8);
\\}
,
\\const std = @import("std");
\\fn foo() !void {
\\ _ = try std.heap.page_allocator.alloc(u8, 8);
\\}
,
\\const std = @import("std");
\\fn foo() anyerror!void {
\\ _ = try std.heap.page_allocator.alloc(u8, 8);
\\}
,
\\fn foo() anyerror![]u8 {
\\ const x = try std.heap.page_allocator.alloc(u8, 8);
\\ return x;
\\}
,
\\const Foo = struct {
\\ pub fn foo() anyerror![]u8 {
\\ const x = try std.heap.page_allocator.alloc(u8, 8);
\\ return x;
\\ }
\\};
,
\\const std = @import("std");
\\fn foo(alloc: ?std.mem.Allocator) ![]const u8 {
\\ if (alloc) |a| {
\\ const result = try a.alloc(u8, 8);
\\ @memset(&result, 0);
\\ return result;
\\ } else {
\\ return "foo";
\\ }
\\}
};

const fail = &[_][:0]const u8{
\\const std = @import("std");
\\fn foo() void {
\\ _ = try std.heap.page_allocator.alloc(u8, 8);
\\}",
,
\\const std = @import("std");
\\const x = try std.heap.page_allocator.alloc(u8, 8);
,
\\const std = @import("std");
\\fn foo() !void {
\\ const Bar = struct {
\\ baz: []u8 = try std.heap.page_allocator.alloc(u8, 8),
\\ };
\\}
};

try runner
.withPass(pass)
.withFail(fail)
.run();
}
18 changes: 18 additions & 0 deletions src/linter/rules/snapshots/homeless-try.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
× expected type expression, found 'invalid bytes'


× homeless-try: `try` cannot be used outside of a function.
╭─[homeless-try.zig:2:11]
1 │ const std = @import("std");
2const x = try std.heap.page_allocator.alloc(u8, 8);
· ───
╰────

× homeless-try: `try` cannot be used outside of a function.
╭─[homeless-try.zig:4:17]
3 │ const Bar = struct {
4baz: []u8 = try std.heap.page_allocator.alloc(u8, 8),
· ───
5 │ };
╰────

11 changes: 8 additions & 3 deletions src/linter/tester.zig
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ pub fn run(self: *RuleTester) !void {
try stderr.writeByte('\n');

switch (e) {
TestError.FailPassed => {
TestError.PassFailed => {
for (self.errors.items) |err| {
try self.fmt.format(&stderr, err);
try stderr.writeByte('\n');
Expand Down Expand Up @@ -113,9 +113,13 @@ fn runImpl(self: *RuleTester) LintTesterError!void {
else => {
self.diagnostic.message = BooStr.fmt(
self.alloc,
"Expected test case #{d} to pass:\n\n{s}\n\nError: {s}\n",
.{ i + 1, self.rule.meta.name, @errorName(e) },
"Pass test case #{d} failed: {s}\n\nSource:\n{s}\n",
.{ i + 1, @errorName(e), src },
) catch @panic("OOM");
if (pass_errors) |errors| {
defer errors.deinit();
try self.errors.appendSlice(self.alloc, errors.items);
}
return LintTesterError.PassFailed;
},
};
Expand Down Expand Up @@ -243,6 +247,7 @@ const MockRule = struct {
};
pub fn runOnNode(_: *const MockRule, _: NodeWrapper, _: *LinterContext) void {}
};

test RuleTester {
const t = std.testing;
var mock_rule = MockRule{};
Expand Down
7 changes: 7 additions & 0 deletions src/semantic/Scope.zig
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ pub const Flags = packed struct(FLAGS_REPR) {
const b: FLAGS_REPR = @bitCast(other);
return a == b;
}

/// Returns `true` if any flags in `other` are also enabled in `self`.
pub fn intersects(self: Flags, other: Flags) bool {
const a: FLAGS_REPR = @bitCast(self);
const b: FLAGS_REPR = @bitCast(other);
return a & b != 0;
}
};

/// Stores variable scopes created by a zig program.
Expand Down
14 changes: 12 additions & 2 deletions src/semantic/SemanticBuilder.zig
Original file line number Diff line number Diff line change
Expand Up @@ -487,9 +487,19 @@ fn visitBlock(self: *SemanticBuilder, statements: []const NodeIndex) !void {
}
}

fn visitContainer(self: *SemanticBuilder, _: NodeIndex, container: full.ContainerDecl) !void {
fn visitContainer(self: *SemanticBuilder, node: NodeIndex, container: full.ContainerDecl) !void {
const main_tokens: []const TokenIndex = self.AST().nodes.items(.main_token);
const tags: []const Token.Tag = self.AST().tokens.items(.tag);

const flags: Scope.Flags = switch (tags[main_tokens[node]]) {
.keyword_enum => .{ .s_enum = true },
.keyword_struct => .{ .s_struct = true },
.keyword_union => .{ .s_union = true },
// e.g. opaque
else => .{},
};
try self.enterScope(.{
.flags = .{ .s_block = true, .s_enum = container.ast.enum_token != null },
.flags = flags.merge(.{ .s_block = true }),
});
defer self.exitScope();
for (container.ast.members) |member| {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@
"children": [
{
"id": semantic.id.NominalId(u32)(1),
"flags": ["block"],
"flags": ["enum", "block"],
"bindings": {
"a": 2,
"b": 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@
"children": [
{
"id": semantic.id.NominalId(u32)(3),
"flags": ["block", "comptime"],
"flags": ["struct", "block", "comptime"],
"bindings": {
"inner": 3,
"Self": 4,
Expand Down Expand Up @@ -349,7 +349,7 @@

}, {
"id": semantic.id.NominalId(u32)(11),
"flags": ["block", "comptime"],
"flags": ["struct", "block", "comptime"],
"bindings": {
"inner": 10,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@
"children": [
{
"id": semantic.id.NominalId(u32)(3),
"flags": ["block"],
"flags": ["struct", "block"],
"bindings": {
"bar": 4,

Expand Down
2 changes: 1 addition & 1 deletion test/snapshots/snapshot-coverage/simple/pass/foo.zig.snap
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@
"children": [
{
"id": semantic.id.NominalId(u32)(1),
"flags": ["block"],
"flags": ["struct", "block"],
"bindings": {
"foo": 5,
"Bar": 6,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
"children": [
{
"id": semantic.id.NominalId(u32)(1),
"flags": ["block"],
"flags": ["struct", "block"],
"bindings": {
"a": 2,
"B": 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@
"children": [
{
"id": semantic.id.NominalId(u32)(1),
"flags": ["block"],
"flags": ["struct", "block"],
"bindings": {
"ptr": 2,
"writeAllFn": 3,
Expand Down Expand Up @@ -283,7 +283,7 @@
"children": [
{
"id": semantic.id.NominalId(u32)(4),
"flags": ["block"],
"flags": ["struct", "block"],
"bindings": {
"writeAll": 9,

Expand Down

0 comments on commit 32ea1ed

Please sign in to comment.