review

review patchsets using your default editor
Log | Files | Refs

commit bb1c0452c45545c3c8d4d5bf6ef4089405b2e458
parent 034e4d8dbf495159a775083b4411e2376dfb46c5
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date:   Wed, 23 Sep 2026 20:11:08 -0300

odin: measure the change with every deterministic check

check is the readings that need no model, ported check for check: the
message's entropy, compressibility, common words, venting, mood, body
and unknown names; the history's coupled files; the change's own
dismissals and deleted tests; names that stutter, shadow or abbreviate;
tests that assert nothing or assert a tautology; cloned bodies; long and
deep functions; comments that restate, task markers, commented-out code,
debug leftovers and swallowed errors; declarations nothing refers to and
code without tests. The rule catalogue prints from the binary.

change gains what the checks read: numstat counts, the subject history,
the temporal coupling counted from git log, the code below each comment,
the line a test skips on, and comments read by shape for the languages
no parser covers. On the 62-file commit 8070974 the Odin reading reports
the same 16 findings as the Go tool, id for id and message for message.

Diffstat:
Mjustfile | 2+-
Modin/change/change.odin | 424++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
Modin/change/change_test.odin | 106+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/check.odin | 366+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/check_test.odin | 1295+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/clones.odin | 381+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/coverage.odin | 218+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/formatting.odin | 47+++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/gaming.odin | 242+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/leftovers.odin | 228+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/message.odin | 746+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/names.odin | 188+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/prose.odin | 78++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/rules.odin | 183+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/shape.odin | 121+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodin/check/tests.odin | 185+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Modin/review/main.odin | 34+++++++++++++++++++++++++++++-----
Modin/tree/tree.odin | 23+++++++++++++++++++++++
18 files changed, 4838 insertions(+), 29 deletions(-)

diff --git a/justfile b/justfile @@ -41,7 +41,7 @@ odin-test: #!/usr/bin/env bash set -euo pipefail mkdir -p build - for p in frontend git tree change finding report; do + for p in frontend git tree change finding report check; do {{odin}} test odin/$p {{odin_flags}} -out:build/${p}_test done diff --git a/odin/change/change.odin b/odin/change/change.odin @@ -8,7 +8,9 @@ what new work is judged against. package change import "core:fmt" +import "core:os" import "core:path/filepath" +import "core:slice" import "core:strconv" import "core:strings" @@ -29,19 +31,38 @@ Symbol :: struct { pkg: string, } -// Function is a test the change adds or touches, whole. +// Function is a test the change adds or touches, whole. skips is the +// line the test skips itself on, or zero, read from the body by shape. Function :: struct { - name: string, - file: string, - line: int, - body: string, + name: string, + file: string, + line: int, + body: string, + skips: int, } -// Located is a comment the change adds. +// Located is a comment the change adds, with the code it sits above, a +// couple of lines of it, so that a claim about behaviour can be read +// beside the behaviour. Located :: struct { - text: string, - file: string, - line: int, + text: string, + file: string, + line: int, + below: string, +} + +// Temporal is what the repository's history counts about the files the +// change touches: how many commits touch each, and which other files +// those commits also touched, nearest first. +Temporal :: struct { + commits: map[string]int, + partners: map[string][]Partner, +} + +// Partner is another file that history shows changing with a changed one. +Partner :: struct { + name: string, + shared: int, } // Declared is one declaration somewhere in the repository, which a new @@ -70,23 +91,39 @@ Diff_Line :: struct { // Change is what is under review. Change :: struct { // diff is the change itself, capped at max_diff. - diff: string, - truncated: bool, + diff: string, + truncated: bool, // files are the paths it touches. - files: []string, + files: []string, // message is the commit message: empty for a staged change. - message: string, - stat: string, + message: string, + stat: string, // added and removed are the diff's two sides, per file. - added: map[string][dynamic]Diff_Line, - removed: map[string][dynamic]string, + added: map[string][dynamic]Diff_Line, + removed: map[string][dynamic]string, // What the sidecars read on the added lines. - symbols: [dynamic]Symbol, - tests: [dynamic]Function, - comments: [dynamic]Located, - imports: map[string][]string, + symbols: [dynamic]Symbol, + tests: [dynamic]Function, + comments: [dynamic]Located, + imports: map[string][]string, // uncovered are the code files no sidecar read, with a reason each. - uncovered: [dynamic]Gap, + uncovered: [dynamic]Gap, + // convention is the recent commit subjects, so a check can read the + // local habit; history is the last thousand, the word frequencies the + // message is measured against. + convention: []string, + history: []string, + // temporal is the counted history of the files the change touches, + // or nothing when the history says nothing. + temporal: Maybe(Temporal), + // changed is how many lines the diff adds and removes, counted by git + // over the whole change rather than the capped diff; whitespace is how + // many of them change nothing but whitespace. + changed: int, + whitespace: int, + // index is every declaration in the repository at the end of the + // change, kept for the checks that judge new work against it. + index: []Declared, } max_diff :: 60000 @@ -115,11 +152,172 @@ gather :: proc(rev, root: string, allocator := context.allocator) -> (c: Change, if rev != "" { c.message, _ = git.run(root, {"log", "-1", "--format=%B", strings.trim_suffix(rev, "^")}) } + // The whitespace-only lines are what the diff loses when git is asked + // to ignore whitespace; the difference is the formatting mixed in. + count := + []string{"diff", "--cached", "--numstat"} if rev == "" else []string{"diff", rev, "--numstat"} + loose := + []string{"diff", "--cached", "-w", "--numstat"} if rev == "" else []string{"diff", rev, "-w", "--numstat"} + if plain, counted := git.run(root, count, context.temp_allocator); counted { + if without, counted_loose := git.run(root, loose, context.temp_allocator); counted_loose { + c.changed = count_numstat(plain) + c.whitespace = c.changed - count_numstat(without) + } + } + c.convention, _ = git.lines(root, {"log", "-12", "--format=%s"}) + c.history, _ = git.lines(root, {"log", "-1000", "--format=%s"}) c.added, c.removed = diff_sides(c.diff) c.imports = make(map[string][]string) + read_temporal(&c, root, rev) return c, true } +// count_numstat sums the lines added and removed over git's --numstat +// output. A binary file's counts are dashes and count nothing. +count_numstat :: proc(out: string) -> int { + n := 0 + rest := out + for line in strings.split_lines_iterator(&rest) { + parts := strings.fields(line, context.temp_allocator) + if len(parts) < 3 { + continue + } + a, _ := strconv.parse_int(parts[0]) + b, _ := strconv.parse_int(parts[1]) + n += a + b + } + return n +} + +// The counting window and width: the coupling is counted over the last +// thousand commits before the change, and a commit listing more than a +// hundred files is left out of the count — a sweep touching everything +// once says nothing about any pair. partner_list is the fewest-nearest +// partners kept per changed file. +temporal_window :: 1000 +temporal_width :: 100 +partner_list :: 20 + +// read_temporal counts, over the last thousand commits before the change, +// how often each changed file is touched by a commit that also touches +// another file. The history starts at the commit the change begins at, so +// the change under review is never counted against itself. +read_temporal :: proc(c: ^Change, root, rev: string) { + if len(c.files) == 0 { + return + } + start := "" + switch { + case rev == "": + case strings.contains(rev, ".."): + start = rev[:strings.index(rev, "..")] + case strings.has_suffix(rev, "^"): + start = rev + case: + start = strings.concatenate({rev, "^"}, context.temp_allocator) + } + args := make([dynamic]string, context.temp_allocator) + append(&args, "log", fmt.tprintf("-%d", temporal_window), "--format=%x00", "--name-only") + if start != "" { + append(&args, start) + } + logs, ok := git.run(root, args[:], context.temp_allocator) + if !ok { + return // No history, or a bare first commit: nothing to count. + } + changed := make(map[string]bool, context.temp_allocator) + for f in c.files { + changed[f] = true + } + commits := make(map[string]int) + pairs := make(map[string]map[string]int, context.temp_allocator) + for chunk in strings.split(logs, "\x00", context.temp_allocator) { + files := make([dynamic]string, context.temp_allocator) + rest := chunk + for line in strings.split_lines_iterator(&rest) { + if trimmed := strings.trim_space(line); trimmed != "" { + append(&files, trimmed) + } + } + if len(files) > temporal_width { + continue + } + for f in files { + commits[strings.clone(f)] += 1 + } + for f in files { + if !changed[f] { + continue + } + if f not_in pairs { + pairs[f] = make(map[string]int, context.temp_allocator) + } + counts := &pairs[f] + for g in files { + if g != f { + counts[g] += 1 + } + } + } + } + t := Temporal { + commits = commits, + partners = make(map[string][]Partner), + } + Named :: struct { + name: string, + j: f64, + } + for f, ps in pairs { + list := make([dynamic]Named, context.temp_allocator) + for name, shared in ps { + joint := commits[f] + commits[name] - shared + if joint <= 0 { + continue + } + append(&list, Named{name, f64(shared) / f64(joint)}) + } + slice.sort_by_cmp(list[:], proc(a, b: Named) -> slice.Ordering { + if a.j != b.j { + return .Less if a.j > b.j else .Greater + } + return .Less if a.name < b.name else (.Greater if a.name > b.name else .Equal) + }) + partners := make([dynamic]Partner) + for e in list { + if len(partners) == partner_list { + break + } + // A partner the tree no longer holds is history's partner, not + // this change's. + if remaining(root, rev, e.name) { + append(&partners, Partner{strings.clone(e.name), ps[e.name]}) + } + } + if len(partners) > 0 { + t.partners[strings.clone(f)] = partners[:] + } + } + if len(t.partners) > 0 { + c.temporal = t + } +} + +// remaining reports whether a partner path is still a file at the end of +// the change. +remaining :: proc(root, rev, name: string) -> bool { + after, ranged := tree.ends(rev) + if !ranged { + return os.is_file(filepath.join({root, name}, context.temp_allocator) or_else name) + } + _, ok := git.run( + root, + {"cat-file", "-e", strings.concatenate({after, ":", name}, context.temp_allocator)}, + context.temp_allocator, + ) + return ok +} + // diff_sides splits a diff into the added and removed lines of each file. // The header lines are read wherever they sit, as they sit between hunks; // content is read only inside a hunk, so that a removed line beginning @@ -211,12 +409,28 @@ read :: proc(c: ^Change, t: tree.Tree, allocator := context.allocator) -> bool { context.allocator = allocator ok := true for name in c.files { + if _, covered := frontend.sidecar_for(name); covered || !tree.exists(t, name) { + continue + } + // A language no parser covers still has its comments read, by + // the prefixes the C-family shares; the jobs that need + // declarations or test bodies are named as unread for it. + if heuristic_covers(name) { + if source, readable := tree.read(t, name, context.temp_allocator); readable { + append(&c.comments, ..comment_prose(source, name, c.added[name])[:]) + } + append( + &c.uncovered, + Gap{name, "no duplication, namer parser"}, + Gap{name, "no tests parser"}, + ) + continue + } // A file no reader could plausibly exist for — prose, data, // configuration — is not a promise the review broke; a code file // with no reader is, because a job that would have read it read // nothing. - if _, covered := frontend.sidecar_for(name); - !covered && is_code_file(name) && tree.exists(t, name) { + if is_code_file(name) { append(&c.uncovered, Gap{name, "no reader"}) } } @@ -253,9 +467,173 @@ read :: proc(c: ^Change, t: tree.Tree, allocator := context.allocator) -> bool { read_file(c, t, name, file) } } + annotate(c, t) return ok } +// annotate adds to what the sidecars read the parts every language +// shares: the code below each comment, and the line a test skips itself +// on. Both are read by shape from the source. +annotate :: proc(c: ^Change, t: tree.Tree) { + lines := make(map[string][]string, context.temp_allocator) + for &comment in c.comments { + if comment.file not_in lines { + source, readable := tree.read(t, comment.file, context.temp_allocator) + lines[comment.file] = + strings.split_lines(string(source), context.temp_allocator) if readable else nil + } + comment.below = code_below(lines[comment.file], comment.line) + } + for &test in c.tests { + test.skips = skip_line(test) + } +} + +// below_lines is how much code a comment is shown beside. +below_lines :: 2 + +// code_below is the code that follows a comment: the first lines after it +// that are neither blank nor comment, up to below_lines of them. +code_below :: proc(lines: []string, comment: int, allocator := context.allocator) -> string { + out := make([dynamic]string, context.temp_allocator) + for i := comment; i < len(lines) && len(out) < below_lines; i += 1 { + trimmed := strings.trim_space(lines[i]) + if trimmed == "" || is_comment_line(trimmed) { + if len(out) > 0 { + break + } + continue + } + append(&out, trimmed) + } + return strings.join(out[:], "\n", allocator) +} + +// is_comment_line is whether a trimmed line is a comment by the shapes +// the tool's languages share. +is_comment_line :: proc(trimmed: string) -> bool { + if strings.has_prefix(trimmed, "//") || + strings.has_prefix(trimmed, "/*") || + strings.has_prefix(trimmed, "*") { + return true + } + return strings.has_prefix(trimmed, "#") && !strings.has_prefix(trimmed, "#!") +} + +// skip_line is the line a test skips itself on, or zero. +skip_line :: proc(t: Function) -> int { + rest := t.body + i := 0 + for line in strings.split_lines_iterator(&rest) { + trimmed := strings.trim_space(line) + for shape in ([]string{"t.Skip(", "t.Skipf(", "t.SkipNow(", "test.skip(", "it.skip(", "describe.skip(", "this.skip(", "testing.skip(", "t.skip(", "pytest.skip(", "pytest.mark.skip", "self.skipTest(", "unittest.skip"}) { + if strings.contains(trimmed, shape) { + return t.line + i + } + } + i += 1 + } + return 0 +} + +// heuristic_covers reports the languages whose comment shape is known +// without a parser. Prose-only formats are left out: their text is not a +// comment. +heuristic_covers :: proc(path: string) -> bool { + for ext in ([]string{".py", ".rb", ".rs", ".c", ".h", ".cc", ".cpp", ".hpp", ".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs", ".lua", ".zig", ".swift", ".kt", ".java", ".php", ".scala", ".cs"}) { + if strings.has_suffix(path, ext) { + return true + } + } + return false +} + +// comment_prose lifts the comment lines among the added ones out of a +// source file by the prefixes the C-family shares: // and # for line +// comments, an open /* kept whole until its close. The shebang is not a +// comment. +comment_prose :: proc( + source: []byte, + name: string, + touched: [dynamic]Diff_Line, + allocator := context.allocator, +) -> [dynamic]Located { + out := make([dynamic]Located, allocator) + if len(touched) == 0 { + return out + } + is_touched := make(map[int]bool, context.temp_allocator) + for l in touched { + is_touched[l.line] = true + } + in_block := false + rest := string(source) + number := 0 + for line in strings.split_lines_iterator(&rest) { + number += 1 + trimmed := strings.trim_space(line) + switch { + case in_block: + if is_touched[number] { + t := strings.trim_suffix(trimmed, "*/") + t = strings.trim_space(strings.trim_prefix(t, "*")) + if t != "" { + append( + &out, + Located{text = strings.clone(t, allocator), file = name, line = number}, + ) + } + } + if strings.contains(trimmed, "*/") { + in_block = false + } + case strings.has_prefix(trimmed, "/*"): + t := trimmed[2:] + if end := strings.index(t, "*/"); end >= 0 { + t = t[:end] + } else { + in_block = true + } + if is_touched[number] && strings.trim_space(t) != "" { + text := strings.trim_space(strings.trim_prefix(strings.trim_space(t), "*")) + append( + &out, + Located{text = strings.clone(text, allocator), file = name, line = number}, + ) + } + case strings.has_prefix(trimmed, "//"): + if is_touched[number] { + append( + &out, + Located { + text = strings.clone( + strings.trim_space(strings.trim_prefix(trimmed, "//")), + allocator, + ), + file = name, + line = number, + }, + ) + } + case strings.has_prefix(trimmed, "#") && !strings.has_prefix(trimmed, "#!"): + if is_touched[number] { + append( + &out, + Located { + text = strings.clone( + strings.trim_space(strings.trim_prefix(trimmed, "#")), + allocator, + ), + file = name, + line = number, + }, + ) + } + } + } + return out +} + // read_file keeps what one file's answer says about the added lines. read_file :: proc(c: ^Change, t: tree.Tree, name: string, file: frontend.File) { source, readable := tree.read(t, name, context.temp_allocator) diff --git a/odin/change/change_test.odin b/odin/change/change_test.odin @@ -140,6 +140,112 @@ gather_reads_a_range_through_the_sidecar :: proc(t: ^testing.T) { } @(test) +comments_are_read_beside_their_code :: proc(t: ^testing.T) { + lines := []string { + "package x", + "", + "// above", + "// and more", + "", + "func f() {", + "\treturn", + "}", + } + testing.expect_value(t, code_below(lines, 3, context.temp_allocator), "func f() {\nreturn") + testing.expect_value(t, code_below(lines, 8, context.temp_allocator), "") + testing.expect_value( + t, + skip_line( + Function{line = 10, body = "func TestX(t *testing.T) {\n\tt.Skip(\"later\")\n}"}, + ), + 11, + ) + testing.expect_value( + t, + skip_line(Function{line = 10, body = "func TestX(t *testing.T) {}"}), + 0, + ) + testing.expect_value(t, count_numstat("3\t1\ta.go\n-\t-\tb.png\n2\t0\tc.go\n"), 6) +} + +@(test) +heuristic_reads_comments_by_shape :: proc(t: ^testing.T) { + src := "#!/usr/bin/env python\n# one\nx = 1 # not a comment line\n\"\"\"\n# two\n/* three\n * four */\n" + touched := make([dynamic]Diff_Line, context.temp_allocator) + for line in ([]int{1, 2, 3, 5, 6, 7}) { + append(&touched, Diff_Line{line, ""}) + } + got := comment_prose(transmute([]byte)src, "a.py", touched, context.temp_allocator) + texts := make([dynamic]string, context.temp_allocator) + for l in got { + append(&texts, l.text) + } + testing.expect_value(t, fmt.tprint(texts[:]), `["one", "two", "three", "four"]`) + testing.expect(t, heuristic_covers("a.zig")) + testing.expect(t, !heuristic_covers("a.md")) +} + +@(test) +temporal_counts_the_pair :: proc(t: ^testing.T) { + root, made := new_repo(t) + testing.expect(t, made, "the fixture repository") + defer os.remove_all(root) + git := proc(root: string, args: ..string) -> bool { + argv := make([dynamic]string, context.temp_allocator) + append( + &argv, + "git", + "-c", + "user.email=t@t", + "-c", + "user.name=t", + "-c", + "commit.gpgsign=false", + ) + append(&argv, ..args) + return sh.exec(argv[:], {dir = root}, context.temp_allocator).ok + } + for i in 0 ..< 8 { + a := filepath.join({root, "pair_a.go"}, context.temp_allocator) or_else "" + b := filepath.join({root, "pair_b.go"}, context.temp_allocator) or_else "" + testing.expect( + t, + os.write_entire_file( + a, + transmute([]byte)fmt.tprintf("package a\n\nvar v%d = %d\n", i, i), + ) == + nil, + ) + testing.expect( + t, + os.write_entire_file( + b, + transmute([]byte)fmt.tprintf("package b\n\nvar w%d = %d\n", i, i), + ) == + nil, + ) + testing.expect(t, git(root, "add", "pair_a.go", "pair_b.go")) + testing.expect(t, git(root, "commit", "-q", "-m", fmt.tprintf("grow: round %d", i))) + } + a := filepath.join({root, "pair_a.go"}, context.temp_allocator) or_else "" + testing.expect( + t, + os.write_entire_file(a, transmute([]byte)string("package a\n\nvar v9 = 9\n")) == nil, + ) + testing.expect(t, git(root, "add", "pair_a.go")) + c, ok := gather("", root, context.temp_allocator) + testing.expect(t, ok) + temporal, counted := c.temporal.? + testing.expect(t, counted, "history counted") + testing.expect_value(t, temporal.commits["pair_a.go"], 8) + testing.expect_value(t, len(temporal.partners["pair_a.go"]), 1) + testing.expect_value(t, temporal.partners["pair_a.go"][0].name, "pair_b.go") + testing.expect_value(t, temporal.partners["pair_a.go"][0].shared, 8) + testing.expect(t, c.changed > 0) + testing.expect(t, len(c.history) >= 10) +} + +@(test) code_files_are_the_ones_a_reader_could_exist_for :: proc(t: ^testing.T) { testing.expect(t, is_code_file("a/b.zig")) testing.expect(t, is_code_file("Justfile")) diff --git a/odin/check/check.odin b/odin/check/check.odin @@ -0,0 +1,366 @@ +/* +Package check is the deterministic readings: what a change can be measured +for without a model, from the commit message to the code it adds — its +names, tests, bodies, comments and leftovers — and the repository around +it. A check that passes says nothing. Each is independent of the others, +and every finding cites a rule the catalogue describes. +*/ +package check + +import "base:runtime" +import "core:fmt" +import "core:slice" +import "core:strings" +import "core:text/regex" +import "core:unicode" + +import "../change" +import "../finding" +import "../tree" + +// Scope is what a check reads: the change, and the repository at its end. +Scope :: struct { + c: ^change.Change, + t: tree.Tree, + files: []string, + sources: map[string][]byte, +} + +// Check measures a change and appends what it noticed. +Check :: #type proc(s: Scope, out: ^[dynamic]finding.Finding) + +// checks are the deterministic readings in the order the catalogue lists +// them: the message, the history, the review's own mechanisms, and then +// the code the change adds. +checks :: proc() -> []Check { + @(static) all := []Check { + check_entropy, + check_compressibility, + check_common, + check_venting, + check_mood, + check_body, + check_formatting, + check_names_unknown, + check_temporal, + check_suppression_added, + check_deleted_tests, + check_names, + check_test_assertions, + check_tautologies, + check_clones, + check_shape, + check_restating, + check_todos, + check_commented_code, + check_debug_leftovers, + check_swallowed_errors, + check_unreferenced, + check_code_without_tests, + } + return all +} + +// scope_of reads the repository once for every check that needs it. +scope_of :: proc(c: ^change.Change, t: tree.Tree, allocator := context.allocator) -> Scope { + s := Scope { + c = c, + t = t, + } + s.files, _ = tree.files(t, allocator) + s.sources, _ = tree.sources(t, allocator) + return s +} + +// run applies every check. Everything a finding holds is allocated from +// allocator. +run :: proc(s: Scope, allocator := context.allocator) -> []finding.Finding { + context.allocator = allocator + sides(s) + out := make([dynamic]finding.Finding) + for check in checks() { + check(s, &out) + } + return out[:] +} + +// collect is one check's findings, for a test of it alone. +collect :: proc(s: Scope, check: Check, allocator := context.allocator) -> []finding.Finding { + context.allocator = allocator + sides(s) + out := make([dynamic]finding.Finding) + check(s, &out) + return out[:] +} + +// sides splits the diff a change was handed without going through gather, +// so that a check reading the added lines reads them. +sides :: proc(s: Scope) { + if len(s.c.added) == 0 && len(s.c.removed) == 0 && s.c.diff != "" { + s.c.added, s.c.removed = change.diff_sides(s.c.diff) + } +} + +// static is a finding a deterministic check made, which verifies itself: +// what it reports was measured, not read once. +static :: proc( + rule: string, + severity: finding.Severity, + message, fix: string, + file := "", + line := 0, + symbol := "", +) -> finding.Finding { + return finding.Finding { + job = "static", + rule = rule, + severity = severity, + message = message, + fix = fix, + file = file, + line = line, + symbol = symbol, + verified = true, + } +} + +// plural is the s a count takes when it is not one. +plural :: proc(n: int) -> string { + return "" if n == 1 else "s" +} + +// first_line is the first line of a text, cut short past 140 characters. +first_line :: proc(s: string, allocator := context.allocator) -> string { + line := s + if i := strings.index_byte(line, '\n'); i >= 0 { + line = line[:i] + } + if len(line) > 140 { + return strings.concatenate({line[:140], "…"}, allocator) + } + return line +} + +// quoted joins names for a message, each in quotes. +quoted :: proc(names: []string, allocator := context.allocator) -> string { + out := make([]string, len(names), context.temp_allocator) + for n, i in names { + out[i] = fmt.tprintf("%q", n) + } + return strings.join(out, ", ", allocator) +} + +// sorted_keys is a map's keys in order, so that a check reports in an +// order a reader can follow. +sorted_keys :: proc(m: map[$K]$V, allocator := context.temp_allocator) -> []K { + keys, _ := slice.map_keys(m, allocator) + slice.sort(keys) + return keys +} + +// set reads a space-separated list of words into a set, allocated to live +// for the program: the lists are the checks' vocabulary. +set :: proc(words: string) -> map[string]bool { + out := make(map[string]bool, runtime.heap_allocator()) + for w in strings.fields(words, context.temp_allocator) { + out[strings.clone(w, runtime.heap_allocator())] = true + } + return out +} + +// split breaks a name into its words: a capital starts a word, an +// underscore ends one, as snake_case languages put the next word after +// it. +split :: proc(name: string, allocator := context.allocator) -> []string { + words := make([dynamic]string, allocator) + start := 0 + for i in 0 ..< len(name) { + c := name[i] + if i > 0 && ((c >= 'A' && c <= 'Z') || c == '_') { + if i > start { + append(&words, name[start:i]) + } + start = i + 1 if c == '_' else i + } + } + if start < len(name) { + append(&words, name[start:]) + } + return words[:] +} + +// fields splits on anything that is not a letter or a digit. +fields :: proc(s: string, allocator := context.allocator) -> []string { + out := make([dynamic]string, allocator) + start := -1 + for r, i in s { + if unicode.is_letter(r) || unicode.is_digit(r) { + if start < 0 { + start = i + } + continue + } + if start >= 0 { + append(&out, s[start:i]) + start = -1 + } + } + if start >= 0 { + append(&out, s[start:]) + } + return out[:] +} + +// has_suffix reports whether a path ends in any of the suffixes. +has_suffix :: proc(path: string, suffixes: []string) -> bool { + for s in suffixes { + if strings.has_suffix(path, s) { + return true + } + } + return false +} + +// is_test_file reports whether a path is one a test runner reads, in any +// of the naming habits the tool's languages have: a marker in the name, +// or a tests directory. A Rust unit test sits in the source file beside +// the code, and that file is not a test file. +is_test_file :: proc(path: string) -> bool { + base := path + if i := strings.last_index_byte(path, '/'); i >= 0 { + base = path[i + 1:] + } + for marker in ([]string{"_test.", ".test.", ".spec."}) { + if strings.contains(base, marker) { + return true + } + } + if strings.has_suffix(base, ".py") && + (strings.has_prefix(base, "test_") || strings.has_suffix(base, "_test.py")) { + return true + } + dir := path[:max(0, len(path) - len(base))] + for part in strings.split(dir, "/", context.temp_allocator) { + if part == "tests" || part == "__tests__" || part == "test" { + return true + } + } + return false +} + +// grammar_of is the ast-grep grammar a TypeScript or JavaScript file is +// read with, or nothing for any other language. +grammar_of :: proc(path: string) -> string { + switch { + case strings.has_suffix(path, ".ts"): + return "ts" + case strings.has_suffix(path, ".tsx"): + return "tsx" + case has_suffix(path, {".js", ".jsx", ".mjs", ".cjs"}): + return "js" + } + return "" +} + +// The patterns the checks match are compiled once and kept for the +// program; a check runs over every line of a change, and a pattern is not +// worth compiling per line. +@(private) +patterns: map[string]regex.Regular_Expression +@(private) +scratch: regex.Capture + +@(init) +init_patterns :: proc "contextless" () { + context = runtime.default_context() + patterns = make(map[string]regex.Regular_Expression, runtime.heap_allocator()) + scratch = regex.preallocate_capture(runtime.heap_allocator()) +} + +// rx is the compiled form of a pattern, in Go's syntax as far as the two +// engines share it. +rx :: proc(pattern: string) -> regex.Regular_Expression { + if re, ok := patterns[pattern]; ok { + return re + } + re, err := regex.create(pattern, {}, runtime.heap_allocator()) + if err != nil { + panic(fmt.tprintf("check: pattern %q: %v", pattern, err)) + } + patterns[strings.clone(pattern, runtime.heap_allocator())] = re + return re +} + +// matches reports whether a pattern matches anywhere in the text. +matches :: proc(pattern, text: string) -> bool { + _, ok := regex.match(rx(pattern), text, &scratch) + return ok +} + +// capture is the groups of the first match: the whole match first, then +// each group, empty where a group did not take part. +capture :: proc( + pattern, text: string, + allocator := context.temp_allocator, +) -> ( + groups: []string, + ok: bool, +) { + cap: regex.Capture + cap, ok = regex.match(rx(pattern), text, allocator) + if !ok { + return nil, false + } + return cap.groups, true +} + +// capture_end is where the first match ends, for a reader that continues +// from there. +capture_end :: proc(pattern, text: string) -> (end: int, ok: bool) { + _, ok = regex.match(rx(pattern), text, &scratch) + if !ok { + return 0, false + } + return scratch.pos[0][1], true +} + +// find_all is every match of a pattern in the text, whole. +find_all :: proc(pattern, text: string, allocator := context.temp_allocator) -> []string { + out := make([dynamic]string, allocator) + it, err := regex.create_iterator(text, pattern, {}, context.temp_allocator) + if err != nil { + return out[:] + } + defer regex.destroy_iterator(it, context.temp_allocator) + for { + cap, _, ok := regex.match_iterator(&it) + if !ok { + break + } + append(&out, cap.groups[0]) + } + return out[:] +} + +// remove_all is the text with every match of a pattern replaced by a +// space. +remove_all :: proc(pattern, text: string, allocator := context.allocator) -> string { + b := strings.builder_make(allocator) + it, err := regex.create_iterator(text, pattern, {}, context.temp_allocator) + if err != nil { + return strings.clone(text, allocator) + } + defer regex.destroy_iterator(it, context.temp_allocator) + last := 0 + for { + cap, _, ok := regex.match_iterator(&it) + if !ok { + break + } + strings.write_string(&b, text[last:cap.pos[0][0]]) + strings.write_string(&b, " ") + last = cap.pos[0][1] + } + strings.write_string(&b, text[last:]) + return strings.to_string(b) +} diff --git a/odin/check/check_test.odin b/odin/check/check_test.odin @@ -0,0 +1,1295 @@ +package check + +import "core:fmt" +import "core:os" +import "core:path/filepath" +import "core:slice" +import "core:strings" +import "core:testing" +import "jfm:sh" + +import "../change" +import "../finding" +import "../frontend" +import "../tree" + + +// dyn is a dynamic array over the items given, for a change built in a +// test. +dyn :: proc(items: []$T) -> [dynamic]T { + out := make([dynamic]T, context.temp_allocator) + append(&out, ..items) + return out +} + +// Pair is one case of a table-driven test. +Pair :: struct($K, $V: typeid) { + key: K, + value: V, +} + +// coupling is a history in which one file changes with one partner. +coupling :: proc( + a: string, + a_commits: int, + b: string, + b_commits: int, + shared: int, +) -> change.Temporal { + t := change.Temporal { + commits = make(map[string]int, context.temp_allocator), + partners = make(map[string][]change.Partner, context.temp_allocator), + } + t.commits[a] = a_commits + t.commits[b] = b_commits + partners := make([]change.Partner, 1, context.temp_allocator) + partners[0] = {b, shared} + t.partners[a] = partners + return t +} + +// only keeps the findings whose rule opens with the prefix. +only :: proc(findings: []finding.Finding, prefix: string) -> []finding.Finding { + out := make([dynamic]finding.Finding, context.temp_allocator) + for f in findings { + if strings.has_prefix(f.rule, prefix) { + append(&out, f) + } + } + return out[:] +} + +rules_of :: proc(findings: []finding.Finding) -> string { + names := make([dynamic]string, context.temp_allocator) + for f in findings { + append(&names, f.rule) + } + return strings.join(names[:], ",", context.temp_allocator) +} + +// over runs every check over a change built in the test, with no +// repository behind it. +over :: proc(c: ^change.Change) -> []finding.Finding { + return run(Scope{c = c}, context.temp_allocator) +} + +// added renders a diff adding the lines to one file, in the shape gather +// produces. +added :: proc(file: string, lines: ..string) -> string { + b := strings.builder_make(context.temp_allocator) + fmt.sbprintf(&b, "--- /dev/null\n+++ b/%s\n@@ -0,0 +1,%d @@\n", file, len(lines)) + for l in lines { + fmt.sbprintf(&b, "+%s\n", l) + } + return strings.to_string(b) +} + +licence_line :: "Copyright 2026 Example Corp. All rights reserved. Licensed under the Apache License, Version 2.0.\n" + +good_message :: `review: say which findings the source dismissed + +A dismissal answered the finding above it and was then thrown away with +the finding, so a reading that met a third of the change reported less +than it read. Kept beside the finding they answered, a dismissal is +visible where a silent one was not, and --verbose names the reason it +gave rather than leaving a gap in the list for nobody to explain. + +The window a dismissal covers stays as written: a few lines either side +of the comment, because narrowing it waits on a case that shows the +narrowing losing a finding it should have kept.` + +@(test) +low_entropy_is_reported :: proc(t: ^testing.T) { + c := change.Change { + message = strings.repeat("asdf asdf ", 6, context.temp_allocator), + } + findings := over(&c) + testing.expect_value(t, rules_of(findings), "message-low-entropy") + if len(findings) == 1 { + f := findings[0] + testing.expect_value(t, f.job, "static") + testing.expect_value(t, f.severity, finding.Severity.Must_Fix) + testing.expect(t, f.verified) + for want in ([]string{"2.3", "Shannon", "3.2", "phrase repeated"}) { + testing.expectf( + t, + strings.contains(f.message, want), + "%q missing from: %s", + want, + f.message, + ) + } + } +} + +@(test) +boilerplate_is_reported :: proc(t: ^testing.T) { + c := change.Change { + message = strings.repeat(licence_line, 9, context.temp_allocator), + } + findings := over(&c) + testing.expect_value(t, rules_of(findings), "message-boilerplate") + if len(findings) == 1 { + for want in ([]string{"compresses", "20%", "pasted"}) { + testing.expectf( + t, + strings.contains(findings[0].message, want), + "%q missing from: %s", + want, + findings[0].message, + ) + } + } + both := change.Change { + message = strings.repeat("asdf asdf asdf asdf asdf asdf\n", 20, context.temp_allocator), + } + testing.expect_value(t, rules_of(over(&both)), "message-low-entropy,message-boilerplate") +} + +@(test) +a_message_that_says_something_is_silent :: proc(t: ^testing.T) { + c := change.Change { + message = good_message, + } + testing.expect_value(t, rules_of(over(&c)), "") + for msg in ([]string{"wip", strings.repeat(licence_line, 4, context.temp_allocator)}) { + short := change.Change { + message = msg, + } + testing.expectf(t, len(over(&short)) == 0, "%q: got %v", msg, over(&short)) + } +} + +// common_history is a history in which fix and build are common and +// nothing else is. +common_history :: proc(extra: ..string) -> []string { + h := make([dynamic]string, context.temp_allocator) + for _ in 0 ..< 60 { + append(&h, "fix the failing build", "build: fix the build", "fix build") + } + for _ in 0 ..< 40 { + append(&h, ..extra) + } + return h[:] +} + +@(test) +common_words_are_reported :: proc(t: ^testing.T) { + c := change.Change { + message = "fix build", + history = common_history(), + files = {"auth_service.go"}, + diff = "+type JWTParser struct{}\n", + } + findings := over(&c) + testing.expect_value(t, rules_of(findings), "message-common-words") + if len(findings) == 1 { + for want in ([]string{"fix", "build", "180", "names nothing"}) { + testing.expectf( + t, + strings.contains(findings[0].message, want), + "%q missing from: %s", + want, + findings[0].message, + ) + } + } +} + +@(test) +common_words_are_spared :: proc(t: ^testing.T) { + // A word the history has not used; the change named; a version + // number; a thin history; git's own subjects. + unused := change.Change { + message = "fix the parser deadlock", + history = common_history(), + files = {"auth_service.go"}, + } + testing.expect_value(t, rules_of(over(&unused)), "") + named := change.Change { + message = "update parser", + history = common_history("update the parser", "parser: update"), + files = {"parser.go"}, + diff = "+func Parse() {}\n", + } + testing.expect_value(t, rules_of(over(&named)), "") + version := change.Change { + message = "bump to 2.0.26", + history = common_history("bump version"), + files = {"version.go"}, + } + testing.expect_value(t, rules_of(over(&version)), "") + thin := change.Change { + message = "fix build", + history = common_history()[:99], + files = {"auth_service.go"}, + } + testing.expect_value(t, rules_of(over(&thin)), "") + for msg in ([]string{"Merge branch 'main' into dev", "Squashed 'vendor/x/' content from branch main"}) { + merge := change.Change { + message = msg, + history = common_history(), + } + testing.expectf(t, len(over(&merge)) == 0, "%q: got %v", msg, over(&merge)) + } +} + +@(test) +venting_is_reported :: proc(t: ^testing.T) { + for msg in ([]string{"whoops", "oops, missed a comma", "damn, ran the formatter with spaces instead of tabs", "WHOOPS, left the debug print in"}) { + c := change.Change { + message = msg, + } + findings := over(&c) + testing.expectf( + t, + rules_of(findings) == "message-frustration", + "%q: got %v", + msg, + findings, + ) + if len(findings) == 1 { + testing.expect(t, strings.contains(findings[0].message, "exclamation")) + } + } + for msg in ([]string{"Change >> behaviour in LLVM to prevent stupid UB", "Respect TERM=dumb in the test runner", "Implement dumb PtrMap", "Temporarily fix the syscall (eventually to be replaced)", "finally fixed the flaky resize test"}) { + c := change.Change { + message = msg, + } + testing.expectf( + t, + len(only(over(&c), "message-frustration")) == 0, + "%q: got %v", + msg, + over(&c), + ) + } +} + +@(test) +mood_is_checked_and_spared :: proc(t: ^testing.T) { + for msg in ([]string{"Added readme.", "Fixing the build", "This fixes the crash", "i can't spell", "web: solarized palette at artifact-page contrast", "deps: upgraded the webp"}) { + c := change.Change { + message = msg, + } + findings := over(&c) + testing.expectf( + t, + rules_of(findings) == "message-not-imperative", + "%q: got %v", + msg, + findings, + ) + if len(findings) == 1 { + testing.expect(t, strings.contains(findings[0].message, "imperative mood")) + } + } + for msg in ([]string{"review: measure the message without a model", "icnsify: read icons out of a Windows binary", "Add caching for responses", "Bump to 2.0.x", "review: an eval set, and the criteria tuned against it", "docs: the vocabulary, the voices section, and the three providers", "Speed up the build", "Merge branch 'main' into dev", "Squashed 'vendor/x/' content from branch main"}) { + c := change.Change { + message = msg, + } + testing.expectf(t, len(over(&c)) == 0, "%q: got %v", msg, over(&c)) + } +} + +varied_words :: proc(n: int) -> string { + words := make([dynamic]string, context.temp_allocator) + for i in 0 ..< n { + append(&words, fmt.tprintf("word%d", i)) + } + return strings.join(words[:], " ", context.temp_allocator) +} + +@(test) +body_is_owed_spared_and_capped :: proc(t: ^testing.T) { + owed := change.Change { + message = "feat: add the thing", + diff = strings.repeat("+line\n", 51, context.temp_allocator), + } + findings := over(&owed) + testing.expect_value(t, rules_of(findings), "message-no-body") + if len(findings) == 1 { + testing.expect(t, strings.contains(findings[0].message, "carries no body")) + } + Case :: struct { + diff, subject, body: string, + } + for c in ([]Case{{strings.repeat("+line\n", 10, context.temp_allocator), "feat: add the thing", ""}, {strings.repeat("+line\n", 500, context.temp_allocator), "feat: add the thing", "one two three four five"}, {strings.repeat("+line\n", 500, context.temp_allocator), "feat: add the thing", varied_words(150)}, {strings.repeat("+line\n", 500, context.temp_allocator), "Merge branch 'main'", ""}, {strings.concatenate({strings.repeat("+x\n", 30, context.temp_allocator), strings.repeat("-x\n", 30, context.temp_allocator)}, context.temp_allocator), "format: run gofmt", ""}}) { + msg := c.subject + if c.body != "" { + msg = strings.concatenate({c.subject, "\n\n", c.body}, context.temp_allocator) + } + spared := change.Change { + message = msg, + diff = c.diff, + } + testing.expectf(t, len(over(&spared)) == 0, "%q: got %v", c.subject, over(&spared)) + } + capped := change.Change { + message = strings.concatenate( + {"feat: add the thing\n\n", varied_words(151)}, + context.temp_allocator, + ), + diff = strings.repeat("+line\n", 10, context.temp_allocator), + } + testing.expect_value(t, rules_of(over(&capped)), "message-long-body") +} + +@(test) +temporal_is_reported_and_spared :: proc(t: ^testing.T) { + coupled := coupling("login.go", 10, "session.go", 9, 9) + c := change.Change { + files = {"login.go"}, + temporal = coupled, + } + findings := over(&c) + testing.expect_value(t, rules_of(findings), "history-coupled-file") + if len(findings) == 1 { + for want in ([]string{"login.go", "session.go", "9 of the 10"}) { + testing.expectf( + t, + strings.contains(findings[0].message, want), + "%q missing from: %s", + want, + findings[0].message, + ) + } + testing.expect_value(t, findings[0].file, "login.go") + } + touched := change.Change { + files = {"login.go", "session.go"}, + temporal = coupled, + } + testing.expect_value(t, rules_of(over(&touched)), "") + weak := change.Change { + files = {"a.go"}, + temporal = coupling("a.go", 10, "b.go", 10, 5), + } + testing.expect_value(t, rules_of(over(&weak)), "") + thin := change.Change { + files = {"a.go"}, + temporal = coupling("a.go", 3, "b.go", 3, 3), + } + testing.expect_value(t, rules_of(over(&thin)), "") +} + +@(test) +brand_shaped_words_are_not_identifiers :: proc(t: ^testing.T) { + for p in ([]Pair(string, bool){{"gRPC", true}, {"iOS", true}, {"macOS", true}, {"eBay", true}, {"getID", false}, {"parseConfig", false}, {"readURL", false}, {"id", false}}) { + testing.expectf(t, brand_shaped(p.key) == p.value, "%s: %v", p.key, brand_shaped(p.key)) + } + names := find_all( + identifier_shaped, + "x: add `load_settings` and parseConfig() for docs/notes.md", + ) + testing.expect_value( + t, + fmt.tprint(names), + "[\"`load_settings`\", \"parseConfig\", \"docs/notes.md\"]", + ) +} + +@(test) +suppression_added_is_reported :: proc(t: ^testing.T) { + // The ignore comment is assembled at runtime so this file does not + // add a dismissal of its own. + diff := strings.concatenate( + { + "--- a/ico.go\n+++ b/ico.go\n@@ -1,3 +1,5 @@\n package ico\n+//review:", + "ignore cannot-fail the test can fail\n+//review:", + "ignore <rule> <why>\n func f() {}\n", + }, + context.temp_allocator, + ) + c := change.Change { + diff = diff, + } + findings := collect(Scope{c = &c}, check_suppression_added, context.temp_allocator) + testing.expect_value(t, len(findings), 1) + if len(findings) == 1 { + f := findings[0] + testing.expect_value(t, f.severity, finding.Severity.Must_Fix) + testing.expect(t, strings.contains(f.message, "ico.go:2 (cannot-fail)")) + testing.expect( + t, + !strings.contains(f.message, "<rule>"), + "counted a documented placeholder", + ) + testing.expect_value(t, f.file, "") + } + prose := change.Change { + diff = strings.concatenate( + { + "--- a/readme.md\n+++ b/readme.md\n@@ -1,3 +1,4 @@\n # review\n+`//review:", + "ignore <rule> <why>`\n done\n", + }, + context.temp_allocator, + ), + } + testing.expect_value( + t, + len(collect(Scope{c = &prose}, check_suppression_added, context.temp_allocator)), + 0, + ) + both := change.Change { + diff = strings.concatenate( + { + "--- a/x_test.go\n+++ b/x_test.go\n@@ -1,2 +1,2 @@\n package x\n+//review:", + "ignore all tidy\n-func TestA(t *testing.T) {}\n", + }, + context.temp_allocator, + ), + } + got := over(&both) + testing.expect_value(t, rules_of(got), "suppression-added,test-deleted") + for f in got { + testing.expect_value(t, f.severity, finding.Severity.Must_Fix) + } +} + +@(test) +deleted_tests_are_reported :: proc(t: ^testing.T) { + c := change.Change { + diff = "--- a/ico_test.go\n+++ b/ico_test.go\n@@ -1,5 +1,4 @@\n package ico\n-func TestAssemble(t *testing.T) {}\n-func TestWrite(t *testing.T) {}\n+func TestAssembleIcons(t *testing.T) {}\n func f() {}\n", + } + findings := collect(Scope{c = &c}, check_deleted_tests, context.temp_allocator) + testing.expect_value(t, len(findings), 1) + if len(findings) == 1 { + f := findings[0] + testing.expect_value(t, f.rule, "test-deleted") + testing.expect_value(t, f.file, "ico_test.go") + testing.expect(t, strings.contains(f.message, "TestWrite")) + testing.expect( + t, + !strings.contains(f.message, "TestAssemble"), + "the rename was counted as a deletion", + ) + testing.expect(t, strings.contains(f.fix, "test-deleted")) + } + js := change.Change { + diff = "--- a/web/app.test.ts\n+++ b/web/app.test.ts\n@@ -1,4 +1 @@\n import { it } from \"testing\";\n-it.skip(\"parses icons\", () => {});\n-describe(\"loads\", () => {});\n+export {};\n", + } + got := collect(Scope{c = &js}, check_deleted_tests, context.temp_allocator) + testing.expect_value(t, len(got), 1) + if len(got) == 1 { + testing.expect(t, strings.contains(got[0].message, "parses icons")) + testing.expect(t, strings.contains(got[0].message, "loads")) + } + outside := change.Change { + diff = "--- a/ico.go\n+++ b/ico.go\n@@ -1,3 +1,2 @@\n package ico\n-func TestWrite(w io.Writer) {}\n-func helper() {}\n+func helper() {}\n", + } + testing.expect_value( + t, + len(collect(Scope{c = &outside}, check_deleted_tests, context.temp_allocator)), + 0, + ) + inside := change.Change { + diff = "--- a/ico_test.go\n+++ b/ico_test.go\n@@ -1,3 +1,2 @@\n package ico\n-func TestWrite(t *testing.T) {}\n-func helper() {}\n+func helper() {}\n", + } + testing.expect_value( + t, + len(collect(Scope{c = &inside}, check_deleted_tests, context.temp_allocator)), + 1, + ) +} + +@(test) +covered_spares_a_rename :: proc(t: ^testing.T) { + testing.expect(t, covered("TestParseIcons", {"TestParseIconsV2", "TestWrite"})) + testing.expect(t, !covered("TestParseIcons", {"TestParse", "TestWrite"})) + testing.expect(t, !covered("TestV2", {"TestV2"})) + for p in ([]Pair(string, string){{`Deno.test("deno string", () => {`, "deno string"}, {`Deno.test.ignore("deno ignored", () => {`, "deno ignored"}, {` test.skip("bun skipped", () => {`, "bun skipped"}, {`it.only("focused", () => {`, "focused"}, {`test('node plain', { timeout: 5 }, () => {`, "node plain"}, {`test.concurrent.only("both", async () => {`, "both"}, {`describe("suite", () => {`, "suite"}, {`Deno.test({ name: "object", fn() {`, ""}, {`const test = 1;`, ""}, {`func TestX(t *testing.T) {`, "TestX"}, {`async def test_it(self):`, "test_it"}}) { + testing.expectf( + t, + removed_test_name(p.key) == p.value, + "%q: got %q, want %q", + p.key, + removed_test_name(p.key), + p.value, + ) + } + for path in ([]string{"ico_test.go", "web/app.test.ts", "web/app.spec.js", "tests/x.py", "test_x.py"}) { + testing.expectf(t, is_test_file(path), "%s is a test file", path) + } + for path in ([]string{"ico.go", "web/app.ts", "src/lib.rs"}) { + testing.expectf(t, !is_test_file(path), "%s is not a test file", path) + } +} + +@(test) +names_are_measured :: proc(t: ^testing.T) { + Stutter :: struct { + s: change.Symbol, + fires: bool, + } + for c in ([]Stutter{{{name = "IcoEntry", pkg = "ico", exported = true, file = "ico/ico.go"}, true}, {{name = "exe_kind", pkg = "exe", exported = true, file = "exe/exe.odin"}, true}, {{name = "Time", pkg = "time", exported = true, file = "time/time.go"}, false}, {{name = "Entry", pkg = "ico", exported = true, file = "ico/ico.go"}, false}, {{name = "icoEntry", pkg = "ico", exported = false, file = "ico/ico.go"}, false}, {{name = "MainLoop", pkg = "main", exported = true, file = "main.go"}, false}, {{name = "Iconic", pkg = "ico", exported = true, file = "ico/ico.go"}, false}}) { + out := make([dynamic]finding.Finding, context.temp_allocator) + stutter(c.s, &out) + testing.expectf(t, (len(out) == 1) == c.fires, "%s in %s: %v", c.s.name, c.s.pkg, out) + } + for c in ([]Stutter{{{name = "len", file = "x.go"}, true}, {{name = "url", file = "x.go"}, true}, {{name = "Promise", file = "x.ts"}, true}, {{name = "render", file = "x.go"}, false}, {{name = "len", file = "x.ts"}, false}, {{name = "Promise", file = "x.go"}, false}}) { + out := make([dynamic]finding.Finding, context.temp_allocator) + shadow(c.s, &out) + testing.expectf(t, (len(out) == 1) == c.fires, "%s in %s: %v", c.s.name, c.s.file, out) + } + for p in ([]Pair(string, bool){{"loadCfg", true}, {"user_mgr", true}, {"BtnLabel", true}, {"msgCount", false}, {"parseURL", false}, {"ctx", false}, {"configure", false}}) { + out := make([dynamic]finding.Finding, context.temp_allocator) + abbreviated(change.Symbol{name = p.key, file = "x.go"}, &out) + testing.expectf(t, (len(out) == 1) == p.value, "%s: %v", p.key, out) + } + c := change.Change { + symbols = dyn( + []change.Symbol { + {name = "IcoEntry", pkg = "ico", exported = true, file = "ico/ico.go", line = 3}, + {name = "cfg", file = "ico/ico.go", line = 9}, + {name = "cfgForTests", file = "ico/ico_test.go", line = 4}, + }, + ), + } + testing.expect_value( + t, + rules_of(collect(Scope{c = &c}, check_names, context.temp_allocator)), + "no-stutter,abbreviation", + ) + testing.expect_value( + t, + fmt.tprint(split("parseHTTPRequest_now", context.temp_allocator)), + `["parse", "H", "T", "T", "P", "Request", "now"]`, + ) +} + +@(test) +leftovers_are_reported :: proc(t: ^testing.T) { + Debug :: struct { + file, line: string, + severity: finding.Severity, + fires: bool, + } + for c in ([]Debug{{"a.ts", " debugger;", .Consider, true}, {"a.tsx", " console.log(x)", .Note, true}, {"a.py", "breakpoint()", .Consider, true}, {"a.rs", "let y = dbg!(x);", .Consider, true}, {"a.go", "spew.Dump(x)", .Consider, true}, {"a.go", `fmt.Println("DEBUG", x)`, .Consider, true}, {"a.go", `fmt.Println("done")`, .Note, false}, {"a.go", "debugger := newDebugger()", .Note, false}, {"a.md", " debugger;", .Note, false}}) { + ch := change.Change { + diff = added(c.file, c.line), + } + got := collect(Scope{c = &ch}, check_debug_leftovers, context.temp_allocator) + testing.expectf(t, (len(got) == 1) == c.fires, "%s %q: %v", c.file, c.line, got) + if len(got) == 1 && c.fires { + testing.expect_value(t, got[0].severity, c.severity) + } + } + todos := change.Change { + comments = dyn( + []change.Located { + {text = "TODO handle the empty case", file = "a.go", line = 1}, + {text = "TODO(jack) handle the empty case", file = "a.go", line = 2}, + {text = "FIXME see #42", file = "a.go", line = 3}, + {text = "HACK until PROJ-12 lands", file = "a.go", line = 4}, + {text = "the todo list is rendered here", file = "a.go", line = 5}, + }, + ), + } + got := collect(Scope{c = &todos}, check_todos, context.temp_allocator) + testing.expect_value(t, len(got), 1) + if len(got) == 1 { + testing.expect_value(t, got[0].line, 1) + } + commented := change.Change { + comments = dyn( + []change.Located { + {text = "x := parse(input);", file = "a.go", line = 1}, + {text = "returns the name (see below)", file = "a.go", line = 5}, + {text = "if err != nil {", file = "a.go", line = 10}, + {text = "return err", file = "a.go", line = 11}, + {text = "}", file = "a.go", line = 12}, + {text = "for the record:", file = "a.go", line = 20}, + {text = "go:generate stringer -type=Kind", file = "a.go", line = 30}, + }, + ), + } + code := collect(Scope{c = &commented}, check_commented_code, context.temp_allocator) + testing.expect_value(t, len(code), 2) + if len(code) == 2 { + testing.expect_value(t, code[0].line, 1) + testing.expect_value(t, code[1].line, 10) + } + Swallowed :: struct { + file: string, + lines: []string, + fires: bool, + } + for c in ([]Swallowed{{"a.go", {"_ = err"}, true}, {"a.go", {"_, err := f()", "if err != nil {", "\treturn err", "}"}, false}, {"a.ts", {"try { f() } catch (e) {}"}, true}, {"a.ts", {"} catch (e) {", "}"}, true}, {"a.ts", {"} catch (e) {", " log(e)", "}"}, false}, {"a.js", {"p.catch(() => {})"}, true}, {"a.py", {"except ValueError:", " pass"}, true}, {"a.py", {"except ValueError: pass"}, true}, {"a.py", {"except ValueError:", " raise"}, false}}) { + ch := change.Change { + diff = added(c.file, ..c.lines), + } + found := collect(Scope{c = &ch}, check_swallowed_errors, context.temp_allocator) + testing.expectf(t, (len(found) == 1) == c.fires, "%s %v: %v", c.file, c.lines, found) + } +} + +// body is a Go function long enough to compare, built from a name and +// the names of the two values it works on. +body :: proc(name, a, b: string) -> string { + return fmt.tprintf( + `func %s(%s []int, %s int) int { + total := 0 + for _, v := range %s { + if v > %s { + total += v + } else if v == %s { + total -= v + } else { + total++ + } + } + if total < 0 { + return -total + } + if total > 1000 { + return 1000 + } + for i := 0; i < len(%s); i++ { + total += i * 2 + } + return total +}`, + name, + a, + b, + a, + b, + b, + a, + ) +} + +// small is a wrapper of the shape every wrapper has, too short for a match +// in shape to mean anything, though long enough for an exact copy to. +small :: proc(name, kind: string) -> string { + return fmt.tprintf( + `func %s(raw string) (%s, error) { + var r %s + if err := json.Unmarshal([]byte(raw), &r); err != nil { + return %s{}, fmt.Errorf("reading the answer: %%w", err) + } + return r, nil +}`, + name, + kind, + kind, + kind, + ) +} + +@(test) +clones_are_found :: proc(t: ^testing.T) { + exact := change.Change { + symbols = dyn( + []change.Symbol { + { + name = "sumAbove", + kind = "func", + file = "b.go", + line = 10, + body = body("sumAbove", "xs", "floor"), + }, + }, + ), + index = { + { + name = "sumAll", + kind = "func", + file = "a.go", + line = 3, + body = body("sumAll", "xs", "floor"), + }, + }, + } + got := collect(Scope{c = &exact}, check_clones, context.temp_allocator) + testing.expect_value(t, len(got), 1) + if len(got) == 1 { + testing.expect_value(t, got[0].rule, "duplicate-body") + testing.expect_value(t, got[0].severity, finding.Severity.Must_Fix) + testing.expect(t, strings.contains(got[0].message, "a.go:3")) + testing.expect_value(t, got[0].symbol, "sumAbove") + } + shape := change.Change { + symbols = dyn( + []change.Symbol { + { + name = "sumAbove", + kind = "func", + file = "b.go", + line = 10, + body = body("sumAbove", "rows", "limit"), + }, + }, + ), + index = { + { + name = "sumAll", + kind = "func", + file = "a.go", + line = 3, + body = body("sumAll", "xs", "floor"), + }, + }, + } + got = collect(Scope{c = &shape}, check_clones, context.temp_allocator) + testing.expect_value(t, len(got), 1) + if len(got) == 1 { + testing.expect_value(t, got[0].severity, finding.Severity.Consider) + } + twice := change.Change { + symbols = dyn( + []change.Symbol { + { + name = "one", + kind = "func", + file = "a.go", + line = 3, + body = body("one", "xs", "floor"), + }, + { + name = "two", + kind = "func", + file = "a.go", + line = 30, + body = body("two", "xs", "floor"), + }, + }, + ), + } + got = collect(Scope{c = &twice}, check_clones, context.temp_allocator) + testing.expect_value(t, len(got), 1) + if len(got) == 1 { + testing.expect_value(t, got[0].symbol, "two") + } + small_or_different := change.Change { + symbols = dyn( + []change.Symbol { + { + name = "Size", + kind = "func", + file = "b.go", + line = 10, + body = "func (e Entry) Size() int { return e.size }", + }, + { + name = "other", + kind = "func", + file = "b.go", + line = 20, + body = strings.concatenate( + {body("other", "xs", "floor"), "\n// and more\nvar _ = 1"}, + context.temp_allocator, + ), + }, + }, + ), + index = { + { + name = "Len", + kind = "func", + file = "a.go", + line = 3, + body = "func (e Entry) Len() int { return e.size }", + }, + { + name = "sumAll", + kind = "func", + file = "a.go", + line = 3, + body = body("sumAll", "xs", "floor"), + }, + }, + } + testing.expect_value( + t, + len(collect(Scope{c = &small_or_different}, check_clones, context.temp_allocator)), + 0, + ) + wrapper := change.Change { + symbols = dyn( + []change.Symbol { + { + name = "decodeB", + kind = "func", + file = "b.go", + line = 10, + body = small("decodeB", "verdicts"), + }, + }, + ), + index = { + { + name = "decodeA", + kind = "func", + file = "a.go", + line = 3, + body = small("decodeA", "reported"), + }, + }, + } + testing.expect_value( + t, + len(collect(Scope{c = &wrapper}, check_clones, context.temp_allocator)), + 0, + ) + wrapper.symbols[0].body = small("decodeB", "reported") + copied := collect(Scope{c = &wrapper}, check_clones, context.temp_allocator) + testing.expect_value(t, len(copied), 1) + if len(copied) == 1 { + testing.expect_value(t, copied[0].severity, finding.Severity.Must_Fix) + } +} + +@(test) +normalise_reads_through_comments_and_space :: proc(t: ^testing.T) { + a := normalise(body("f", "xs", "n"), "f", "go", context.temp_allocator) + with_comment, _ := strings.replace_all( + body("f", "xs", "n"), + "total := 0", + "total := 0 // start\n\n", + context.temp_allocator, + ) + b := normalise(with_comment, "f", "go", context.temp_allocator) + testing.expect_value(t, a.exact, b.exact) + testing.expect(t, strings.contains(a.exact, "NAME")) + testing.expect(t, !strings.contains(a.exact, " f ")) + testing.expect(t, strings.contains(a.structural, "for ID , ID := range ID")) + toks := lexemes(`x := a.b(0x1F, 2.5, "s\"t", 'c') // c`, context.temp_allocator) + testing.expect_value( + t, + fmt.tprint(toks), + `["x", ":=", "a", ".", "b", "(", "0x1F", ",", "2.5", ",", "\"s\\\"t\"", ",", "'c'", ")", "/", "/", "c"]`, + ) + testing.expect_value( + t, + strip_comments("a // b\n # c\nd /* e */ f \"//x\"", context.temp_allocator), + "a \n \nd f \"//x\"", + ) +} + +@(test) +shape_prose_and_formatting_are_measured :: proc(t: ^testing.T) { + deep := "func f() {\n\tif a {\n\t\tfor b {\n\t\t\tif c {\n\t\t\t\tswitch d {\n\t\t\t\tcase 1:\n\t\t\t\t\tif e {\n\t\t\t\t\t\tif g { x := \"{\" }\n\t\t\t\t\t}\n\t\t\t\t}\n\t\t\t}\n\t\t}\n\t}\n}" + testing.expect_value(t, nesting(deep), 6) + testing.expect_value(t, nesting("func f() { return 1 }"), 0) + testing.expect_value(t, line_count("a\nb\nc"), 3) + c := change.Change { + symbols = dyn( + []change.Symbol{{name = "f", kind = "func", file = "a.go", line = 1, body = deep}}, + ), + } + testing.expect_value( + t, + rules_of(collect(Scope{c = &c}, check_shape, context.temp_allocator)), + "nesting-too-deep", + ) + long := change.Change { + symbols = dyn( + []change.Symbol { + { + name = "g", + kind = "func", + file = "a.go", + line = 1, + body = strings.repeat("x\n", 151, context.temp_allocator), + }, + }, + ), + } + testing.expect_value( + t, + rules_of(collect(Scope{c = &long}, check_shape, context.temp_allocator)), + "function-too-long", + ) + + testing.expect( + t, + restates(change.Located{text = "parse the input", below = "func parseInput(x) {\nreturn"}), + ) + testing.expect( + t, + !restates( + change.Located{text = "parse the input carefully", below = "func parseInput(x) {"}, + ), + ) + testing.expect(t, !restates(change.Located{text = "parse", below = "func parse() {"})) + testing.expect( + t, + !restates(change.Located{text = "TODO parse the input", below = "func parseInput() {"}), + ) + testing.expect( + t, + directive("go:generate x") && directive("see https://x") && !directive("plain prose"), + ) + prose := change.Change { + comments = dyn( + []change.Located { + { + text = "parse the input", + file = "a.go", + line = 3, + below = "func parseInput(x) {", + }, + }, + ), + } + testing.expect_value( + t, + rules_of(collect(Scope{c = &prose}, check_restating, context.temp_allocator)), + "comment-restates-code", + ) + + mixed := change.Change { + changed = 100, + whitespace = 60, + } + testing.expect_value( + t, + rules_of(collect(Scope{c = &mixed}, check_formatting, context.temp_allocator)), + "formatting-mixed-in", + ) + pure := change.Change { + changed = 100, + whitespace = 95, + } + testing.expect_value( + t, + rules_of(collect(Scope{c = &pure}, check_formatting, context.temp_allocator)), + "", + ) + little := change.Change { + changed = 30, + whitespace = 10, + } + testing.expect_value( + t, + rules_of(collect(Scope{c = &little}, check_formatting, context.temp_allocator)), + "", + ) +} + +@(test) +tests_are_measured :: proc(t: ^testing.T) { + testing.expect( + t, + assertless( + change.Function { + name = "TestX", + file = "a_test.go", + body = "func TestX(t *testing.T) {\n\tf()\n}", + }, + ), + ) + testing.expect( + t, + !assertless( + change.Function { + name = "TestX", + file = "a_test.go", + body = "func TestX(tc *testing.T) {\n\ttc.Fatal(1)\n}", + }, + ), + ) + testing.expect( + t, + !assertless( + change.Function { + name = "TestX", + file = "a_test.go", + body = "func TestX(t *testing.T) {\n\thelper(t, 1)\n}", + }, + ), + ) + testing.expect( + t, + !assertless( + change.Function { + name = "BenchmarkX", + file = "a_test.go", + body = "func BenchmarkX(b *testing.B) {}", + }, + ), + ) + testing.expect( + t, + assertless( + change.Function { + name = "reads", + file = "a.test.ts", + body = "test('reads', () => { f() })", + }, + ), + ) + testing.expect( + t, + !assertless( + change.Function { + name = "reads", + file = "a.test.ts", + body = "test('reads', () => { expect(f()).toBe(1) })", + }, + ), + ) + testing.expect( + t, + assertless( + change.Function { + name = "x", + file = "a_test.odin", + body = "x :: proc(t: ^testing.T) { f() }", + }, + ), + ) + testing.expect( + t, + !assertless( + change.Function { + name = "x", + file = "a_test.odin", + body = "x :: proc(t: ^testing.T) { testing.expect(t, f()) }", + }, + ), + ) + for p in ([]Pair(string, string){{"assert.True(t, true)", "a constant"}, {"require.NoError(t, nil)", "a constant"}, {"if got != got {", "a value against itself"}, {"assert 1 == 1", "a constant"}, {"self.assertTrue(True)", "a constant"}, {"expect(true).toBe(true)", "a constant"}, {"expect(1).toEqual(1)", "a constant"}, {"expect(x.name).toBe(x.name)", "a value against itself"}, {"assert!(true);", "a constant"}, {"assert_eq!(2, 2);", "a constant"}, {"testing.expect(t, true)", "a constant"}, {"assert_eq!(a, a);", "a value against itself"}, {"assert.Equal(t, x, x)", "a value against itself"}, {"assert.Equal(t, got, want)", ""}, {"if got != want {", ""}, {"// assert.True(t, true) is what not to write", ""}, {"expect(x).toBe(y)", ""}}) { + testing.expectf( + t, + tautological(p.key) == p.value, + "%q: got %q, want %q", + p.key, + tautological(p.key), + p.value, + ) + } + c := change.Change { + tests = dyn( + []change.Function { + { + name = "TestX", + file = "x_test.go", + line = 10, + body = "func TestX(t *testing.T) {\n\tassert.True(t, true)\n}", + }, + { + name = "TestY", + file = "x_test.go", + line = 20, + body = "func TestY(t *testing.T) {\n\tf()\n}", + }, + }, + ), + } + got := over(&c) + testing.expect_value(t, rules_of(got), "test-no-assertion,assertion-always-true") + if len(got) == 2 { + testing.expect_value(t, got[1].line, 11) + testing.expect_value(t, got[1].symbol, "TestX") + } +} + +@(test) +references_are_searched_as_whole_words :: proc(t: ^testing.T) { + sources := make(map[string][]byte, context.temp_allocator) + sources["a.go"] = transmute([]byte)string("package x\n\nfunc Waiting() int { return 2 }\n") + sources["b.go"] = transmute([]byte)string("var _ = WaitingRoom\n") + testing.expect( + t, + !referenced(change.Symbol{name = "Waiting", file = "a.go", line = 3}, sources), + "a prefix of another word counted", + ) + sources["c.go"] = transmute([]byte)string("var _ = Waiting()\n") + testing.expect( + t, + referenced(change.Symbol{name = "Waiting", file = "a.go", line = 3}, sources), + "a call was not counted", + ) + sources["c.go"] = transmute([]byte)string("// Waiting is documented here\n") + testing.expect( + t, + !referenced(change.Symbol{name = "Waiting", file = "a.go", line = 3}, sources), + "a comment counted", + ) + + testing.expect( + t, + called_by_the_runtime(change.Symbol{name = "MarshalJSON", kind = "func", file = "a.go"}), + ) + testing.expect( + t, + called_by_the_runtime( + change.Symbol { + name = "DllGetClassObject", + kind = "func", + file = "a.go", + doc = "DllGetClassObject answers COM.\n\nexport DllGetClassObject", + }, + ), + ) + testing.expect( + t, + !called_by_the_runtime( + change.Symbol { + name = "DllInstall", + kind = "func", + file = "a.go", + doc = "export DllGetClassObject", + }, + ), + ) + testing.expect( + t, + !called_by_the_runtime(change.Symbol{name = "String", kind = "value", file = "a.go"}), + ) + desc, ok := describe("go-vet/nilness") + testing.expect(t, ok && strings.contains(desc, "go vet")) + _, ok = describe("no-such-rule") + testing.expect(t, !ok) + testing.expect(t, strings.contains(catalogue(context.temp_allocator), "`test-deleted`")) +} + +// new_repo makes a repository with one commit holding go.mod, x.go and +// x_test.go, for the checks that read the tree. +new_repo :: proc(t: ^testing.T) -> (root: string, ok: bool) { + temp := os.temp_directory(context.temp_allocator) or_else "" + scratch, err := os.make_directory_temp(temp, "review-check-*", context.temp_allocator) + if err != nil { + testing.fail_now(t, "no scratch directory") + } + root = scratch + ok = git(root, "init", "-q") + ok &&= write(root, "go.mod", "module x\n\ngo 1.27.0\n") + ok &&= write(root, "x.go", "package x\n") + ok &&= write(root, "x_test.go", "package x\n") + ok &&= git(root, "add", "go.mod", "x.go", "x_test.go") + ok &&= git(root, "commit", "-q", "-m", "first") + return root, ok +} + +git :: proc(root: string, args: ..string) -> bool { + argv := make([dynamic]string, context.temp_allocator) + append(&argv, "git", "-c", "user.email=t@t", "-c", "user.name=t", "-c", "commit.gpgsign=false") + append(&argv, ..args) + return sh.exec(argv[:], {dir = root}, context.temp_allocator).ok +} + +write :: proc(root, name, src: string) -> bool { + path := filepath.join({root, name}, context.temp_allocator) or_else name + return os.write_entire_file(path, transmute([]byte)src) == nil +} + +// staged gathers the staged change of a repository and reads it, for a +// check that needs the whole tree. +staged :: proc(t: ^testing.T, root: string) -> (c: ^change.Change, s: Scope) { + c = new(change.Change, context.temp_allocator) + gathered: bool + c^, gathered = change.gather("", root, context.temp_allocator) + testing.expect(t, gathered, "gather") + tr, at_ok := tree.at(root, "", context.temp_allocator) + testing.expect(t, at_ok) + change.read(c, tr, context.temp_allocator) + c.index, _ = change.index(tr, context.temp_allocator) + return c, scope_of(c, tr, context.temp_allocator) +} + +@(test) +unreferenced_reads_the_whole_tree :: proc(t: ^testing.T) { + if !frontend.installed(.Go) { + testing.fail_now(t, "review-go is not on the path") + } + root, made := new_repo(t) + testing.expect(t, made) + defer os.remove_all(root) + testing.expect( + t, + write( + root, + "x.go", + "package x\n\n// Used is called from the template below.\nfunc Used() int { return 1 }\n\n// Waiting is called from nowhere.\nfunc Waiting() int { return 2 }\n\nfunc lonely() int { return 3 }\n", + ), + ) + testing.expect(t, write(root, "page.tmpl", "{{ Used }}\n")) + testing.expect(t, git(root, "add", "x.go", "page.tmpl")) + _, s := staged(t, root) + got := collect(s, check_unreferenced, context.temp_allocator) + testing.expect_value(t, len(got), 1) + if len(got) == 1 { + testing.expect_value(t, got[0].rule, "new-symbol-unreferenced") + testing.expect_value(t, got[0].file, "x.go") + testing.expect(t, strings.contains(got[0].message, "Waiting")) + testing.expect(t, strings.contains(got[0].message, "lonely")) + testing.expect( + t, + !strings.contains(got[0].message, "Used"), + "a name the template uses was reported", + ) + } +} + +@(test) +code_without_tests_is_reported_where_tests_are_kept :: proc(t: ^testing.T) { + if !frontend.installed(.Go) { + testing.fail_now(t, "review-go is not on the path") + } + root, made := new_repo(t) + testing.expect(t, made) + defer os.remove_all(root) + parts := make([dynamic]string, context.temp_allocator) + append(&parts, "package x\n\n") + for i in 0 ..< 60 { + append(&parts, fmt.tprintf("var v%d = %d\n", i, i)) + } + testing.expect(t, write(root, "x.go", strings.concatenate(parts[:], context.temp_allocator))) + testing.expect(t, git(root, "add", "x.go")) + c, s := staged(t, root) + c.message = "x: add sixty variables" + got := over_scope(s) + testing.expect_value(t, rules_of(only(got, "code-without")), "code-without-tests") + testing.expect_value(t, rules_of(only(got, "message-")), "message-no-body") + names := only(got, "message-names") + testing.expect_value(t, len(names), 0) + + testing.expect(t, write(root, "x_test.go", "package x\n\n// touched\n")) + testing.expect(t, git(root, "add", "x_test.go")) + _, again := staged(t, root) + testing.expect_value( + t, + len(collect(again, check_code_without_tests, context.temp_allocator)), + 0, + ) +} + +over_scope :: proc(s: Scope) -> []finding.Finding { + return run(s, context.temp_allocator) +} + +@(test) +names_unknown_reads_the_tree :: proc(t: ^testing.T) { + if !frontend.installed(.Go) { + testing.fail_now(t, "review-go is not on the path") + } + root, made := new_repo(t) + testing.expect(t, made) + defer os.remove_all(root) + testing.expect( + t, + write(root, "x.go", "package x\n\nfunc readConfig() {}\n\nfunc parseFlags() {}\n"), + ) + testing.expect( + t, + os.make_directory_all(filepath.join({root, "docs"}, context.temp_allocator) or_else "") == + nil, + ) + testing.expect(t, write(root, "docs/notes.md", "notes\n")) + testing.expect(t, git(root, "add", "x.go", "docs/notes.md")) + c, s := staged(t, root) + Case :: struct { + message: string, + fires: bool, + } + for k in ([]Case{{"x: add parseFlags beside readConfig", false}, {"x: add parseConfig()", true}, {"x: add `load_settings` for docs/notes.md", true}, {"x: touch docs/notes.md", false}, {"x: make the reader faster", false}, {"x: see https://example.com/parseConfig", false}}) { + c.message = k.message + got := collect(s, check_names_unknown, context.temp_allocator) + testing.expectf(t, (len(got) == 1) == k.fires, "%q: got %v", k.message, got) + } + _ = slice.contains([]int{1}, 1) +} diff --git a/odin/check/clones.odin b/odin/check/clones.odin @@ -0,0 +1,381 @@ +package check + +// Two functions with the same body are one fact stated twice, and telling +// that two bodies are the same needs no judgement: the tokens either +// match or they do not. Whether two different bodies mean the same thing +// is the duplication job's. + +import "base:runtime" +import "core:fmt" +import "core:strings" + +import "../finding" + +// The floors under which two bodies are not compared. Two one-line +// accessors are alike because accessors are alike; a match in shape alone +// needs a body twice as long, because two small wrappers share a shape +// because wrappers do. +min_clone_tokens :: 40 +min_shape_tokens :: 80 + +@(private = "file") +keywords: map[string]map[string]bool + +@(init) +init_keywords :: proc "contextless" () { + context = runtime.default_context() + // The words a language reserves, per grammar, which a structural + // comparison keeps while it replaces every other identifier. + keywords = make(map[string]map[string]bool, runtime.heap_allocator()) + keywords["go"] = set( + `break case chan const continue default defer else fallthrough for func go goto if import + interface map package range return select struct switch type var nil true false iota + len cap append make new panic recover error string int int8 int16 int32 int64 uint uint8 uint16 + uint32 uint64 byte rune float32 float64 bool any`, + ) + keywords["js"] = set( + `break case catch class const continue debugger default delete do else enum export extends + false finally for function if import in instanceof new null return super switch this throw true + try typeof var void while with yield let static async await of undefined interface type + implements private public protected readonly declare namespace abstract as is keyof never + unknown string number boolean object symbol bigint`, + ) + keywords["py"] = set( + `False None True and as assert async await break class continue def del elif else except + finally for from global if import in is lambda nonlocal not or pass raise return try while with + yield self cls print len range str int float list dict set tuple bool`, + ) + keywords["rs"] = set( + `as async await break const continue crate dyn else enum extern false fn for if impl in let + loop match mod move mut pub ref return self Self static struct super trait true type unsafe use + where while Some None Ok Err Vec String Option Result Box i8 i16 i32 i64 u8 u16 u32 u64 usize + isize f32 f64 bool char str`, + ) + keywords["odin"] = set( + `package import foreign proc struct union enum bit_set map dynamic using if else when for + switch case in not_in defer return break continue fallthrough cast transmute auto_cast distinct + matrix or_else or_return or_break or_continue where do context true false nil int uint bool + string rune byte f32 f64 i8 i16 i32 i64 u8 u16 u32 u64 uintptr rawptr any typeid`, + ) +} + +// grammar_for is which keyword set a file's tokens are read with. +grammar_for :: proc(path: string) -> string { + switch { + case strings.has_suffix(path, ".go"): + return "go" + case strings.has_suffix(path, ".odin"): + return "odin" + case strings.has_suffix(path, ".py"): + return "py" + case strings.has_suffix(path, ".rs"): + return "rs" + case grammar_of(path) != "": + return "js" + } + if dot := strings.last_index_byte(path, '.'); dot >= 0 { + return path[dot:] + } + return "" +} + +// Shapes are a body's two normalisations: exact, with only the function's +// own name replaced, and structural, with every identifier and literal +// replaced. Empty when the body is too small to compare. +Shapes :: struct { + exact: string, + structural: string, + tokens: int, +} + +// normalise reads a body into its shapes. Whitespace and comments are +// gone in both; the exact shape keeps every name but the function's own, +// the structural shape keeps only the grammar's keywords and punctuation. +normalise :: proc(body, name, grammar: string, allocator := context.allocator) -> Shapes { + toks := lexemes(strip_comments(body, context.temp_allocator), context.temp_allocator) + if len(toks) < min_clone_tokens { + return Shapes{} + } + reserved := keywords[grammar] + exact := make([]string, len(toks), context.temp_allocator) + structural := make([]string, len(toks), context.temp_allocator) + for t, i in toks { + exact[i] = "NAME" if t == name else t + c := t[0] + switch { + case reserved[t]: + structural[i] = t + case c == '_' || (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z'): + structural[i] = "ID" + case c == '"' || c == '\'' || c == '`' || (c >= '0' && c <= '9'): + structural[i] = "LIT" + case: + structural[i] = t + } + } + return Shapes { + exact = strings.join(exact, " ", allocator), + structural = strings.join(structural, " ", allocator), + tokens = len(toks), + } +} + +// strip_comments blanks what a tokeniser drops: line comments, block +// comments, and the hash comments of the languages that write them at a +// line's start. +strip_comments :: proc(body: string, allocator := context.allocator) -> string { + b := strings.builder_make(allocator) + i := 0 + line_start := true + for i < len(body) { + c := body[i] + switch { + case c == '/' && i + 1 < len(body) && body[i + 1] == '/': + for i < len(body) && body[i] != '\n' { + i += 1 + } + strings.write_byte(&b, ' ') + continue + case c == '/' && i + 1 < len(body) && body[i + 1] == '*': + end := strings.index(body[i + 2:], "*/") + i = len(body) if end < 0 else i + 2 + end + 2 + strings.write_byte(&b, ' ') + continue + case c == '#' && line_start: + for i < len(body) && body[i] != '\n' { + i += 1 + } + strings.write_byte(&b, ' ') + continue + case c == '"' || c == '\'' || c == '`': + // A string is copied whole, so a comment marker inside it is + // not a comment. + j := i + 1 + for j < len(body) && body[j] != c { + if body[j] == '\\' && c != '`' { + j += 1 + } + if body[j] == '\n' && c != '`' { + break + } + j += 1 + } + j = min(j + 1, len(body)) + strings.write_string(&b, body[i:j]) + i = j + line_start = false + continue + } + strings.write_byte(&b, c) + if c == '\n' { + line_start = true + } else if c != ' ' && c != '\t' { + line_start = false + } + i += 1 + } + return strings.to_string(b) +} + +// lexemes reads the lexical tokens of the C-family languages: an +// identifier, a number, a string in any of three quotings, a +// two-character operator, or one character of punctuation. +lexemes :: proc(text: string, allocator := context.allocator) -> []string { + out := make([dynamic]string, allocator) + i := 0 + for i < len(text) { + c := text[i] + switch { + case c == ' ' || c == '\t' || c == '\n' || c == '\r': + i += 1 + case c == '_' || (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z'): + j := i + 1 + for j < len(text) && + (text[j] == '_' || + (text[j] >= 'a' && text[j] <= 'z') || + (text[j] >= 'A' && text[j] <= 'Z') || + (text[j] >= '0' && text[j] <= '9')) { + j += 1 + } + append(&out, text[i:j]) + i = j + case c >= '0' && c <= '9': + j := i + 1 + if c == '0' && j < len(text) && (text[j] == 'x' || text[j] == 'X') { + j += 1 + for j < len(text) && is_hex(text[j]) { + j += 1 + } + } else { + for j < len(text) && text[j] >= '0' && text[j] <= '9' { + j += 1 + } + if j + 1 < len(text) && + text[j] == '.' && + text[j + 1] >= '0' && + text[j + 1] <= '9' { + j += 1 + for j < len(text) && text[j] >= '0' && text[j] <= '9' { + j += 1 + } + } + } + append(&out, text[i:j]) + i = j + case c == '"' || c == '\'' || c == '`': + j := i + 1 + for j < len(text) && text[j] != c { + if c != '`' && text[j] == '\\' { + j += 1 + } else if c != '`' && text[j] == '\n' { + break + } + j += 1 + } + if j < len(text) && text[j] == c { + append(&out, text[i:j + 1]) + i = j + 1 + } else { + append(&out, text[i:i + 1]) + i += 1 + } + case: + if i + 1 < len(text) && is_double_operator(text[i:i + 2]) { + append(&out, text[i:i + 2]) + i += 2 + } else { + append(&out, text[i:i + 1]) + i += 1 + } + } + } + return out[:] +} + +is_hex :: proc(c: byte) -> bool { + return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') +} + +is_double_operator :: proc(s: string) -> bool { + switch s { + case ":=", + "::", + "==", + "!=", + "<=", + ">=", + "&&", + "||", + "++", + "--", + "+=", + "-=", + "*=", + "/=", + "->", + "=>", + "<<", + ">>": + return true + } + return false +} + +// Owned is one function with its shapes, and where it is declared. +Owned :: struct { + name: string, + file: string, + line: int, + shape: Shapes, +} + +// check_clones reports a new function whose body already exists: in the +// repository's index, or in another function the same change adds. An +// exact match is one function written twice and is must-fix; a match in +// shape alone, every name changed, is the same procedure over other +// names, and is worth considering. +check_clones :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + fresh := make([dynamic]Owned, context.temp_allocator) + for sym in s.c.symbols { + if sym.kind != "func" || sym.body == "" || is_test_file(sym.file) { + continue + } + shape := normalise(sym.body, sym.name, grammar_for(sym.file), context.temp_allocator) + if shape.tokens == 0 { + continue + } + append(&fresh, Owned{sym.name, sym.file, sym.line, shape}) + } + if len(fresh) == 0 { + return + } + existing := make([dynamic]Owned, context.temp_allocator) + for d in s.c.index { + if d.kind != "func" || d.body == "" { + continue + } + shape := normalise(d.body, d.name, grammar_for(d.file), context.temp_allocator) + if shape.tokens == 0 { + continue + } + append(&existing, Owned{d.name, d.file, d.line, shape}) + } + reported := make(map[string]bool, context.temp_allocator) + for a in fresh { + for b in existing { + if a.file == b.file && a.line == b.line { + continue + } + alike(a, b, &reported, out) + } + } + // Two new functions alike are reported once, the later against the + // earlier, where the index did not already hold the earlier. + for a, i in fresh { + for b in fresh[:i] { + alike(a, b, &reported, out) + } + } +} + +// alike reports a against b when their bodies match: exactly at any size +// compared, or in shape when both are long enough for a shape to mean +// something. A pair is reported once. +alike :: proc(a, b: Owned, reported: ^map[string]bool, out: ^[dynamic]finding.Finding) { + exact := false + switch { + case a.shape.exact == b.shape.exact: + exact = true + case a.shape.tokens >= min_shape_tokens && + b.shape.tokens >= min_shape_tokens && + a.shape.structural == b.shape.structural: + case: + return + } + key := fmt.tprintf("%s:%d|%s:%d", a.file, a.line, b.file, b.line) + if reported[key] { + return + } + reported[strings.clone(key, context.temp_allocator)] = true + how := + "the same body, token for token," if exact else "the same shape of body, every name changed," + append( + out, + static( + "duplicate-body", + .Must_Fix if exact else .Consider, + fmt.aprintf( + "%s has %s as %s at %s:%d; one procedure written twice drifts into two", + a.name, + how, + b.name, + b.file, + b.line, + ), + fmt.aprintf("call %s, or lift what they share into one function both call", b.name), + file = a.file, + line = a.line, + symbol = a.name, + ), + ) +} diff --git a/odin/check/coverage.odin b/odin/check/coverage.odin @@ -0,0 +1,218 @@ +package check + +// Two things about a change are visible only against the whole +// repository: whether anything refers to what it adds, and whether it +// added tests where the repository keeps them. Both are counted over the +// tree in memory. + +import "base:runtime" +import "core:fmt" +import "core:strings" + +import "../change" +import "../finding" + +@(private = "file") +unsearchable: map[string]bool +@(private = "file") +runtime_methods: map[string]bool + +@(init) +init_coverage_lists :: proc "contextless" () { + context = runtime.default_context() + // The names the language calls rather than the code, and names too + // short to search for. + unsearchable = set(`main init TestMain _ default`) + // The methods the standard library and its encoders call through an + // interface or by reflection, so that nothing in the repository names + // them and they are referenced all the same. + runtime_methods = set( + `String Error Format GoString MarshalJSON UnmarshalJSON MarshalText UnmarshalText + MarshalBinary UnmarshalBinary MarshalYAML UnmarshalYAML GobEncode GobDecode Len Less Swap Read Write + Close Seek ReadFrom WriteTo ServeHTTP Scan Value Is As Unwrap Compare Equal Hash`, + ) +} + +// check_unreferenced reports a new declaration nothing in the repository +// refers to: not the change, not the rest of the tree. It is written and +// waiting, and what waits drifts. The name is searched as a whole word +// over every text file, so a use from a template or a script counts. One +// finding per file, naming what it declares and nothing refers to. +check_unreferenced :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + unused := make(map[string][dynamic]change.Symbol, context.temp_allocator) + for sym in s.c.symbols { + if is_test_file(sym.file) || + sym.kind == "field" || + unsearchable[sym.name] || + called_by_the_runtime(sym) { + continue + } + if referenced(sym, s.sources) { + continue + } + list := unused[sym.file] + if list.allocator.procedure == nil { + list = make([dynamic]change.Symbol, context.temp_allocator) + } + append(&list, sym) + unused[sym.file] = list + } + for file in sorted_keys(unused) { + symbols := unused[file] + names := make([dynamic]string, context.temp_allocator) + for sym, i in symbols { + if i == 8 { + append(&names, fmt.tprintf("and %d more", len(symbols) - 8)) + break + } + append(&names, sym.name) + } + append( + out, + static( + "new-symbol-unreferenced", + .Consider, + fmt.aprintf( + "nothing in the repository refers to %s but the declaration%s in %s; code written for a caller that does not exist yet is a guess about what the caller will need", + strings.join(names[:], ", ", context.temp_allocator), + plural(len(symbols)), + file, + ), + "use it, or leave it out until something does", + file = file, + line = symbols[0].line, + symbol = symbols[0].name, + ), + ) + } +} + +// called_by_the_runtime is whether a symbol is called by something +// outside the repository's text: a method an encoder or interface reaches +// for, or a function cgo exports to the host under //export. +called_by_the_runtime :: proc(sym: change.Symbol) -> bool { + if sym.kind == "func" && runtime_methods[sym.name] { + return true + } + if strings.has_suffix(sym.file, ".go") { + rest := sym.doc + for line in strings.split_lines_iterator(&rest) { + if strings.trim_space(line) == + strings.concatenate({"export ", sym.name}, context.temp_allocator) { + return true + } + } + } + return false +} + +// referenced is whether the name appears as a whole word anywhere but on +// its own declaration line or in a comment. A Go doc comment opens with +// the name it documents, and a comment is not a caller. +referenced :: proc(sym: change.Symbol, sources: map[string][]byte) -> bool { + if len(sym.name) < 2 { + return true // Too short to search for honestly. + } + for file, data in sources { + text := string(data) + offset := 0 + for { + i := strings.index(text[offset:], sym.name) + if i < 0 { + break + } + at := offset + i + offset = at + len(sym.name) + if !word_boundary(text, at, len(sym.name)) { + continue + } + if file == sym.file && line_of(text, at) == sym.line { + continue + } + if in_comment(text, at) { + continue + } + return true + } + } + return false +} + +// word_boundary is whether the match at i of length n is bounded by +// non-identifier characters on both sides. +word_boundary :: proc(text: string, i, n: int) -> bool { + before := i == 0 || !ident_char(text[i - 1]) + after := i + n >= len(text) || !ident_char(text[i + n]) + return before && after +} + +ident_char :: proc(b: byte) -> bool { + return b == '_' || (b >= '0' && b <= '9') || (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') +} + +// in_comment is whether the offset sits on a line that is a comment by +// the shapes the tool's languages share, or after a line comment's +// opening. +in_comment :: proc(text: string, offset: int) -> bool { + start := strings.last_index_byte(text[:offset], '\n') + 1 + line := text[start:offset] + head := strings.concatenate( + {line, text[offset:min(offset + 1, len(text))]}, + context.temp_allocator, + ) + if change.is_comment_line(strings.trim_space(head)) { + return true + } + return strings.contains(line, "//") || strings.contains(line, "/*") +} + +// line_of is the 1-based line the offset falls on. +line_of :: proc(text: string, offset: int) -> int { + return 1 + strings.count(text[:offset], "\n") +} + +// test_floor_lines is the size of change, in lines added to code that is +// not a test, from which a change owes a test where the repository keeps +// them. +test_floor_lines :: 50 + +// check_code_without_tests reports a change that adds a body of code to a +// repository that has tests, and touches none of them. +check_code_without_tests :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + lines := 0 + for file, l in s.c.added { + if change.is_code_file(file) && !is_test_file(file) { + lines += len(l) + } + } + if lines < test_floor_lines { + return + } + for f in s.c.files { + if is_test_file(f) { + return + } + } + tested := 0 + for f in s.files { + if is_test_file(f) { + tested += 1 + } + } + if tested == 0 { + return // A repository without tests is not asked to start here. + } + append( + out, + static( + "code-without-tests", + .Consider, + fmt.aprintf( + "the change adds %d lines of code and touches no test, in a repository that keeps %d test files; what the change does is asserted nowhere", + lines, + tested, + ), + "add or extend the test that would fail without this change", + ), + ) +} diff --git a/odin/check/formatting.odin b/odin/check/formatting.odin @@ -0,0 +1,47 @@ +package check + +// A commit that reformats and changes logic in one diff hides the logic +// among the reformatting, and neither can be reverted alone. Git can count +// which changed lines change only whitespace, and the count is the check. + +import "core:fmt" + +import "../finding" + +// formatting_share is the share of a change's lines that change only +// whitespace at which the change is a reformatting with logic mixed in; +// formatting_floor the fewest whitespace-only lines worth a word; +// logic_floor the fewest lines that change something else. +formatting_share :: 0.5 +formatting_floor :: 20 +logic_floor :: 10 + +// check_formatting reports a change whose diff is mostly whitespace and +// yet carries logic too: two changes that should be two commits. +check_formatting :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + c := s.c + if c.changed == 0 || c.whitespace < formatting_floor { + return + } + logic := c.changed - c.whitespace + if logic < logic_floor { + return + } + if f64(c.whitespace) / f64(c.changed) < formatting_share { + return + } + append( + out, + static( + "formatting-mixed-in", + .Consider, + fmt.aprintf( + "%d of the change's %d lines change only whitespace, and %d change something else; a reformatting with logic in it hides the logic, and neither half can be reverted alone", + c.whitespace, + c.changed, + logic, + ), + "commit the reformatting on its own, then the change", + ), + ) +} diff --git a/odin/check/gaming.odin b/odin/check/gaming.odin @@ -0,0 +1,242 @@ +package check + +// The anti-gaming checks hold the review to itself: a change that +// dismisses what the reading would find, or deletes the tests that would +// have failed, is measured rather than waved through. + +import "core:fmt" +import "core:strings" + +import "../change" +import "../finding" + +// rule_id is what a dismissal's rule id has to look like. The prose that +// documents the mechanism writes placeholders (<rule>) and quoted +// examples, which match the ignore pattern but dismiss nothing. +rule_id :: `^[a-z][a-z0-9-]*$` + +// check_suppression_added reports a change that adds its own dismissal, +// before the readers have run. The finding names no file, so it cannot +// be dismissed in turn. +check_suppression_added :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + spots := make([dynamic]string, context.temp_allocator) + for file in sorted_keys(s.c.added) { + if !change.is_code_file(file) { + continue + } + lines := s.c.added[file] + for l in lines { + rule, _, found := finding.dismissal(l.text) + if !found || !matches(rule_id, rule) { + continue + } + append(&spots, fmt.tprintf("%s:%d (%s)", file, l.line, rule)) + } + } + if len(spots) == 0 { + return + } + append( + out, + static( + "suppression-added", + .Must_Fix, + fmt.aprintf( + "the change adds its own dismissal%s: %s; a dismissal is a decision for the person reviewing the change, and a review that dismisses itself cannot be failed", + plural(len(spots)), + strings.join(spots[:], "; ", context.temp_allocator), + ), + "correct what the dismissal covers; a dismissal belongs to the person reviewing, who accepts it deliberately", + ), + ) +} + +// The shapes a test's declaration takes on a diff line, per runner. +go_test :: `^func ((?:Test|Benchmark|Fuzz)[A-Za-z0-9_]+)\(` +py_test :: `^(?:async\s+)?def (test_\w+)\(` +js_test_call :: `^(?:Deno\.test|test|describe|it)(?:\.(?:skip|only|todo|fails|failing|ignore|concurrent|serial))*\(` +rs_test_attribute :: `^\#\[[\w:]*test(\(|\])` +rs_fn :: `^(?:pub(?:\([^)]*\))?\s+)?(?:async\s+)?fn (\w+)\(` + +// check_deleted_tests reports the tests a change deletes. A test is never +// heard from again once deleted, so deletion is where a test the reading +// would have failed goes to pass the review. A test the change also adds +// under a name made of the same words is a rename, and is spared. +check_deleted_tests :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + spoken := added_test_names(s.c.added, context.temp_allocator) + for file in sorted_keys(s.c.removed) { + if !is_test_file(file) && !strings.has_suffix(file, ".rs") { + continue + } + removed := s.c.removed[file] + names := make([dynamic]string, context.temp_allocator) + seen := make(map[string]bool, context.temp_allocator) + for text, i in removed { + name := removed_test_name(text) + if strings.has_suffix(file, ".rs") { + // A Rust test is the function after the attribute, and + // only that; a deleted function without one is not a test. + name = "" + if matches(rs_test_attribute, strings.trim_space(text)) && i + 1 < len(removed) { + if m, ok := capture(rs_fn, strings.trim_space(removed[i + 1])); ok { + name = m[1] + } + } + } + if name == "" || seen[name] { + continue + } + seen[name] = true + if !covered(name, spoken) { + append(&names, name) + } + } + if len(names) == 0 { + continue + } + shown := make([dynamic]string, context.temp_allocator) + for name, i in names { + if i == 8 { + append(&shown, fmt.tprintf("and %d more", len(names) - 8)) + break + } + append(&shown, fmt.tprintf("%q", name)) + } + append( + out, + static( + "test-deleted", + .Must_Fix, + fmt.aprintf( + "the change deletes the test%s %s from %s; a deleted test cannot fail again, so deletion is where a failing test goes to pass the review", + plural(len(names)), + strings.join(shown[:], ", ", context.temp_allocator), + file, + ), + fmt.aprintf( + "restore the test, or dismiss it where a reader can read why: a review:ignore comment naming test-deleted, with the reason, in %s", + file, + ), + file = file, + symbol = strings.clone(names[0]), + ), + ) + } +} + +// removed_test_name reads the name of a deleted test out of its removed +// line, in the shapes the tool's languages write tests in. +removed_test_name :: proc(text: string) -> string { + trimmed := strings.trim_left(text, " \t") + if m, ok := capture(go_test, trimmed); ok { + return m[1] + } + if m, ok := capture(py_test, trimmed); ok { + return m[1] + } + if end, ok := capture_end(js_test_call, trimmed); ok { + return read_quoted(strings.trim_left(trimmed[end:], " \t")) + } + return "" +} + +// read_quoted reads the string literal a JS test registration is named +// by, from the text that follows its opening parenthesis. +read_quoted :: proc(text: string) -> string { + if len(text) == 0 { + return "" + } + q := text[0] + if q != '"' && q != '\'' && q != '`' { + return "" + } + if end := strings.index_byte(text[1:], q); end >= 0 { + return text[1:1 + end] + } + return "" +} + +// added_test_names collects the names of the tests a change adds, read +// with the same shapes the removed side is read with, so that a rename is +// not read as a deletion. +added_test_names :: proc( + added: map[string][dynamic]change.Diff_Line, + allocator := context.allocator, +) -> []string { + seen := make(map[string]bool, context.temp_allocator) + for _, lines in added { + for l in lines { + if name := added_test_name(strings.trim_left(l.text, " \t")); name != "" { + seen[name] = true + } + } + } + return sorted_keys(seen, allocator) +} + +// added_test_name reads a test's name off an added line, in any runner's +// shape, or nothing. +added_test_name :: proc(trimmed: string) -> string { + for pattern in ([]string{go_test, py_test, rs_fn}) { + if m, ok := capture(pattern, trimmed); ok { + return m[1] + } + } + if end, ok := capture_end(js_test_call, trimmed); ok { + return read_quoted(strings.trim_left(trimmed[end:], " \t")) + } + return "" +} + +// covered reports whether a deleted test's name is spoken for by a test +// the change adds: every word of the old name is in a new one, which is +// what a rename or a re-anchored test looks like. +covered :: proc(deleted: string, spoken: []string) -> bool { + want := test_words(deleted, context.temp_allocator) + if len(want) == 0 { + return false + } + for name in spoken { + have := test_words(name, context.temp_allocator) + says_all := true + for w in want { + found := false + for h in have { + if h == w { + found = true + break + } + } + if !found { + says_all = false + break + } + } + if says_all { + return true + } + } + return false +} + +// test_words breaks a test's name into its lowercased words: camel humps +// and underscores for the Go shapes, spaces for the strings the +// JavaScript shapes are named by, with the runner's own prefixes and +// short leftovers left out. +test_words :: proc(name: string, allocator := context.allocator) -> []string { + out := make([dynamic]string, allocator) + for part in split(name, context.temp_allocator) { + for piece in strings.fields(part, context.temp_allocator) { + word := strings.to_lower(piece, allocator) + if len(word) < 3 { + continue + } + switch word { + case "test", "benchmark", "fuzz", "skip", "only", "todo", "fails": + continue + } + append(&out, word) + } + } + return out[:] +} diff --git a/odin/check/leftovers.odin b/odin/check/leftovers.odin @@ -0,0 +1,228 @@ +package check + +// What a change leaves behind by accident has a shape: a debugger +// statement, a task marker nobody is named on, code kept as a comment, an +// error caught and dropped. Each is a pattern over the lines the change +// adds. + +import "core:fmt" +import "core:slice" +import "core:strings" + +import "../change" +import "../finding" + +// Debug_Marker is a call or statement that exists to be removed before a +// change is done, by language. Ordinary printing is not here: a command's +// output and a debug print share a function. +Debug_Marker :: struct { + suffixes: []string, + pattern: string, + severity: finding.Severity, +} + +@(private = "file") +debug_markers := []Debug_Marker { + {{".js", ".jsx", ".mjs", ".cjs", ".ts", ".tsx"}, `^\s*debugger\s*;?\s*$`, .Consider}, + {{".js", ".jsx", ".mjs", ".cjs", ".ts", ".tsx"}, `\bconsole\.(log|debug|trace)\(`, .Note}, + {{".py"}, `\b(breakpoint\(\)|pdb\.set_trace\(\)|ipdb\.set_trace\(\))`, .Consider}, + {{".rs"}, `\bdbg!\(`, .Consider}, + {{".go"}, `\b(spew\.Dump|litter\.Dump|pp\.Print)\(`, .Consider}, + {{".rb"}, `\b(binding\.pry|byebug|debugger)\b`, .Consider}, + {nil, `\b(Printf|Println|Print|log|print|debug)\(\s*["'](DEBUG|XXX|HERE|>>>)`, .Consider}, +} + +// check_debug_leftovers reports the debugging a change adds and did not +// remove. +check_debug_leftovers :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for file in sorted_keys(s.c.added) { + if !change.is_code_file(file) { + continue + } + lines := s.c.added[file] + for l in lines { + for m in debug_markers { + if m.suffixes != nil && !has_suffix(file, m.suffixes) { + continue + } + if !matches(m.pattern, l.text) { + continue + } + append( + out, + static( + "debug-leftover", + m.severity, + fmt.aprintf( + "the change adds debugging output: %s", + strings.trim_space(l.text), + ), + "remove it before the change is done", + file = file, + line = l.line, + ), + ) + break + } + } + } +} + +// task_marker is a comment that names work left undone; task_reference +// what makes it answerable: an issue number, a ticket key, a link, or a +// name in parentheses. +task_marker :: `\b(TODO|FIXME|XXX|HACK)\b` +task_reference :: `\#\d+|\b[A-Z][A-Z0-9]+-\d+\b|https?://|\(\w+\)` + +// check_todos reports a task marker the change adds with nothing to find +// it by again. Unreferenced, it is a promise the log will not keep. +check_todos :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for comment in s.c.comments { + if !matches(task_marker, comment.text) || matches(task_reference, comment.text) { + continue + } + append( + out, + static( + "todo-without-reference", + .Note, + fmt.aprintf( + "the change adds a task marker nothing refers to: %q", + first_line(comment.text, context.temp_allocator), + ), + "name the issue or the person, or do the work now", + file = comment.file, + line = comment.line, + ), + ) + } +} + +// strong_code is a comment line that is code beyond doubt: an assignment +// operator, a call closed and terminated, a closing brace terminated, an +// arrow function. weak_code is a line shaped like a statement, which two +// in a row make into commented-out code. +strong_code :: `:=|\);\s*$|\};\s*$|=>|^\s*\}\s*else\s*\{` +weak_code :: `^(if|for|while|return|func|fn|def|import|const|let|var|switch|case|else|try|catch|package|use|pub|proc|struct|type|class|await|export)\b.*[({=:;]\s*$|^[\w.]+\(.*\)\s*;?\s*$|[;{}]\s*$` + +// code_like is whether a comment line reads as code, and how surely. +code_like :: proc(text: string) -> (strong, weak: bool) { + trimmed := strings.trim_space(text) + if trimmed == "" || directive(trimmed) { + return false, false + } + return matches(strong_code, trimmed), matches(weak_code, trimmed) +} + +// check_commented_code reports code the change keeps as comments. A run +// of consecutive comment lines is one candidate; it is reported when two +// of its lines are shaped like statements, or one is code beyond doubt. +check_commented_code :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + by_file := make(map[string][dynamic]change.Located, context.temp_allocator) + for comment in s.c.comments { + list := by_file[comment.file] + if list.allocator.procedure == nil { + list = make([dynamic]change.Located, context.temp_allocator) + } + append(&list, comment) + by_file[comment.file] = list + } + for file in sorted_keys(by_file) { + comments := by_file[file][:] + slice.sort_by_cmp(comments, proc(a, b: change.Located) -> slice.Ordering { + return .Less if a.line < b.line else (.Greater if a.line > b.line else .Equal) + }) + i := 0 + for i < len(comments) { + j := i + strong, weak := 0, 0 + for j < len(comments) && (j == i || comments[j].line == comments[j - 1].line + 1) { + st, wk := code_like(comments[j].text) + if st { + strong += 1 + } + if wk || st { + weak += 1 + } + j += 1 + } + if strong > 0 || weak >= 2 { + append( + out, + static( + "commented-out-code", + .Consider, + fmt.aprintf( + "the change adds code as a comment, %d line(s) from %s:%d; the version control has the old code, and a reader cannot tell a comment that was code from one that is meant", + j - i, + file, + comments[i].line, + ), + "delete it; git remembers it", + file = file, + line = comments[i].line, + ), + ) + } + i = j + } + } +} + +go_dropped_error :: `^\s*_\s*=\s*err\b` +empty_catch :: `\bcatch\s*(\([^)]*\))?\s*\{\s*\}` +open_catch :: `\bcatch\s*(\([^)]*\))?\s*\{\s*$` +promise_catch :: `\.catch\(\s*(\(\s*\w*\s*\)|\w+)?\s*=>\s*\{\s*\}\s*\)` +except_pass :: `^\s*except\b[^:]*:\s*pass\s*$` +except_open :: `^\s*except\b[^:]*:\s*$` +closing_brace :: `^\s*\}` +pass_line :: `^\s*pass\s*$` + +// check_swallowed_errors reports an error the change catches and drops: +// a Go error assigned to the blank identifier, an empty catch in +// JavaScript or TypeScript, an except that passes in Python. +check_swallowed_errors :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for file in sorted_keys(s.c.added) { + if !change.is_code_file(file) { + continue + } + lines := s.c.added[file] + for l, i in lines { + next := "" + if i + 1 < len(lines) && lines[i + 1].line == l.line + 1 { + next = lines[i + 1].text + } + hit := false + switch { + case strings.has_suffix(file, ".go"): + hit = matches(go_dropped_error, l.text) + case grammar_of(file) != "": + hit = + matches(empty_catch, l.text) || + matches(promise_catch, l.text) || + (matches(open_catch, l.text) && matches(closing_brace, next)) + case strings.has_suffix(file, ".py"): + hit = + matches(except_pass, l.text) || + (matches(except_open, l.text) && matches(pass_line, next)) + } + if !hit { + continue + } + append( + out, + static( + "error-swallowed", + .Consider, + fmt.aprintf( + "the change catches an error and drops it: %s; a dropped error is a failure the program has decided not to know about", + strings.trim_space(l.text), + ), + "handle it, return it, or write beside it why it cannot matter", + file = file, + line = l.line, + ), + ) + } + } +} diff --git a/odin/check/message.odin b/odin/check/message.odin @@ -0,0 +1,746 @@ +package check + +// The checks here need no model. They measure the commit message and +// report what fails, before anything is asked of a provider and whether or +// not one can answer. + +import "base:runtime" +import "core:fmt" +import "core:math" +import "core:slice" +import "core:strings" +import "vendor:zlib" + +import "../change" +import "../finding" + +// min_entropy is the Shannon entropy, in bits per byte, under which a +// message is one short phrase said over rather than a description of a +// change. Ordinary commit messages measure between 3.7 and 4.9; +// entropy_floor is the length under which entropy says nothing. +min_entropy :: 3.2 +entropy_floor :: 40 + +// max_compression is the share of its length a message keeps after zlib, +// under which the message is one block of text pasted or repeated whole. +// No written commit message measured keeps less than 0.26; +// compression_floor is the length under which zlib cannot beat its own +// framing. +max_compression :: 0.20 +compression_floor :: 400 + +// common_idf is the natural log under which a word counts as one of the +// repository's commonest: it appears in at least a fifth of the subjects. +// history_floor is the number of subjects under which the frequencies are +// too thin to judge a message by. +common_idf :: 1.6 +history_floor :: 100 + +// digits are the characters whose presence spares a message: a version or +// an issue number is information however few words carry it. +digits :: "0123456789" + +// body_floor_lines is the size of diff under which a body is optional; +// max_body_words the length over which a body is listing what the diff +// already shows. +body_floor_lines :: 50 +max_body_words :: 150 + +// temporal_coupling is the Jaccard over which history says two files +// change together; temporal_support the fewest commits they must share +// before it is a pattern; temporal_findings caps how many pairs one +// change is asked about. +temporal_coupling :: 0.7 +temporal_support :: 5 +temporal_findings :: 3 + +@(private = "file") +venting_words: map[string]bool +@(private = "file") +stop_words: map[string]bool +@(private = "file") +openers: map[string]bool +@(private = "file") +invariant_verbs: map[string]bool +@(private = "file") +irregular_past: map[string]bool +@(private = "file") +non_verb_ing: map[string]bool +@(private = "file") +brands: map[string]bool + +@(init) +init_message_words :: proc "contextless" () { + context = runtime.default_context() + // The exclamations and profanities of a message written in the moment + // of the mistake. Every word here is unambiguous: stupid, dumb, + // finally, eventually and annoying all appear in measured history + // describing the code legitimately. + venting_words = set( + `fuck fucking fucked shit bullshit wtf damn dammit damnit oops oopsie whoops + ugh argh grr sigh fml yolo idk`, + ) + // The function words, which name nothing. + stop_words = set( + `a an the and or but if then than that this these those + to of in on at by for with from into up out it its is are was were be been being am + as so not no nor do does did done doing can could will would shall should may might + must have has had having i we you they he she him her them his hers their ours your + my me us what which who whom when where why how all any both each few more most + other some such only own same too very just also now here there over under again + further once about between through during before after above below because while + until against`, + ) + // The words whose presence alone is narration. + openers = set(`i we my this these those`) + // The verbs whose past and imperative share a form. + invariant_verbs = set( + `read cut set put let hit cost split shut cast hurt quit burst spread slit`, + ) + // The past forms no suffix rule could catch. + irregular_past = set( + `wrote made kept went got ran brought built bought caught drove found held left met + paid sent spent took won sold freed`, + ) + // The words that end in ing without being a verb's gerund. + non_verb_ing = set( + `during nothing something anything everything morning evening offing outing bring + king ring sing spring string swing thing wing cling sting fling`, + ) + // The product names written with an inner capital. + brands = set( + `gRPC iOS macOS iPadOS watchOS tvOS iPhone iPad iCloud eBay jQuery + PayPal YouTube GitHub GitLab OpenAI WebAssembly LaTeX TeX`, + ) +} + +// measured is the message the checks read. It is empty for a staged +// change without a supplied message, and every message check says nothing +// then. +measured :: proc(c: ^change.Change) -> string { + return strings.trim_space(c.message) +} + +// check_entropy reports a message whose characters carry too little +// entropy: the shape of a placeholder, a keyboard mash, or one phrase +// repeated. +check_entropy :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + msg := measured(s.c) + if msg == "" || len(msg) < entropy_floor { + return + } + h := shannon_entropy(msg) + if h >= min_entropy { + return + } + append( + out, + static( + "message-low-entropy", + .Must_Fix, + fmt.aprintf( + "the commit message measures %.1f bits of Shannon entropy per byte, under the %.1f beneath which a message is a phrase repeated rather than a description; ordinary messages measure 3.7 to 4.9", + h, + min_entropy, + ), + "write a message that says what the change does and why", + ), + ) +} + +// check_compressibility reports a message zlib keeps only a fraction of: +// the shape of text pasted or repeated wholesale, such as a licence notice +// or a log, rather than prose written for this change. +check_compressibility :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + msg := measured(s.c) + if msg == "" || len(msg) < compression_floor { + return + } + ratio := compression_ratio(msg) + if ratio >= max_compression { + return + } + append( + out, + static( + "message-boilerplate", + .Must_Fix, + fmt.aprintf( + "the commit message compresses to %.0f%% of its length, under the %.0f%% beneath which it is one block of text pasted or repeated rather than prose about the change; no written message measured keeps less than 26%%", + 100 * ratio, + 100 * max_compression, + ), + "keep only what the reader needs of the quoted text, and write the rest", + ), + ) +} + +// check_common reports a message made entirely of the words this +// repository's own history uses most, which names nothing the change +// touches. A word the history has never used is the one thing a message +// like this cannot have, which is why a message holding any rarer word is +// left to the reader. +check_common :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + msg := measured(s.c) + if msg == "" || len(s.c.history) < history_floor { + return + } + words := content_words(msg, context.temp_allocator) + if len(words) == 0 { + return + } + if strings.has_prefix(msg, "Merge ") || + strings.has_prefix(msg, "Squashed ") || + strings.contains_any(msg, digits) { + return + } + freq := frequencies(s.c.history, context.temp_allocator) + n := len(s.c.history) + for w in words { + if math.ln(f64(n) / f64(1 + freq[w])) > common_idf { + return + } + } + ground := diff_words(s.c, context.temp_allocator) + for w in words { + if ground[w] { + return + } + } + listed := sorted_keys(words) + append( + out, + static( + "message-common-words", + .Must_Fix, + fmt.aprintf( + "the commit message is made of the repository's commonest commit words — %s — with nothing rarer than a fifth of its %d commit subjects, and it names nothing the change touches; a word the history has not used is the one thing a message like this cannot have", + strings.join(listed, ", ", context.temp_allocator), + n, + ), + "name the part and the fault, in words the change itself uses", + ), + ) +} + +// identifier_shaped matches the words of a message that name code rather +// than describe it: a camel-cased or snake-cased word, a dotted or slashed +// path, a call, or anything in backticks. +identifier_shaped :: "`[^`]+`|\\b[a-z][a-z0-9]*[A-Z][A-Za-z0-9]*\\b|\\b[A-Za-z][A-Za-z0-9]*_[A-Za-z0-9_]+\\b|\\b[A-Za-z][A-Za-z0-9_]*\\(\\)|\\b[A-Za-z][A-Za-z0-9_-]*(?:[./][A-Za-z0-9_-]+)+\\.[a-z]{1,5}\\b" + +// links matches a URL in a message. +links :: `\bhttps?://\S+` + +// check_names_unknown reports a message that names an identifier the +// repository does not hold: not in the diff, not in any file at the end +// of the change, not a path in the tree. A message naming code that is +// not there describes work the diff does not contain. +check_names_unknown :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + msg := measured(s.c) + if msg == "" { + return + } + msg = remove_all(links, msg, context.temp_allocator) + names := make([dynamic]string, context.temp_allocator) + seen := make(map[string]bool, context.temp_allocator) + for m in find_all(identifier_shaped, msg) { + name := strings.trim_suffix(strings.trim_space(strings.trim(m, "`")), "()") + if name == "" || + seen[name] || + strings.contains_any(name, " \t") || + strings.contains(name, "://") || + brand_shaped(name) { + continue + } + seen[name] = true + append(&names, name) + } + if len(names) == 0 { + return + } + missing := make([dynamic]string, context.temp_allocator) + for name in names { + if strings.contains(s.c.diff, name) || strings.contains(s.c.stat, name) { + continue + } + found := false + for f in s.files { + if strings.contains(f, name) { + found = true + break + } + } + if !found { + for _, data in s.sources { + if strings.contains(string(data), name) { + found = true + break + } + } + } + if !found { + append(&missing, name) + } + } + if len(missing) == 0 { + return + } + append( + out, + static( + "message-names-unknown", + .Consider, + fmt.aprintf( + "the commit message names %s, and nothing by that name is in the diff or anywhere in the repository at the end of the change; a message naming code that is not there describes work the diff does not contain", + quoted(missing[:], context.temp_allocator), + ), + "name what the change actually touches, as the code spells it", + ), + ) +} + +// brand_shaped is whether a word is a product name rather than a name +// from the code: on the list, or a short lowercase prefix before a run of +// capitals, which is how gRPC and iOS are spelled and how no identifier +// is. +brand_shaped :: proc(word: string) -> bool { + if brands[word] { + return true + } + i := 0 + for i < len(word) && word[i] >= 'a' && word[i] <= 'z' { + i += 1 + } + if i == 0 || i > 2 || i == len(word) { + return false + } + for j in i ..< len(word) { + if word[j] < 'A' || word[j] > 'Z' { + return false + } + } + return true +} + +// check_venting reports a message whose words are the author's reaction +// rather than the change's description: oops, whoops, damn, profanity. +check_venting :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + msg := measured(s.c) + if msg == "" { + return + } + hit := make(map[string]bool, context.temp_allocator) + for piece in fields(msg, context.temp_allocator) { + w := strings.to_lower(piece, context.temp_allocator) + if venting_words[w] { + hit[w] = true + } + } + if len(hit) == 0 { + return + } + append( + out, + static( + "message-frustration", + .Must_Fix, + fmt.aprintf( + "the commit message is an exclamation — %s — where a description should be; the log then records the author's feeling, and the change goes undescribed", + strings.join(sorted_keys(hit), ", ", context.temp_allocator), + ), + "describe the change, not the moment", + ), + ) +} + +// check_mood reports a subject that does not open as a command. The +// discipline is package: explainer, with the explainer in the imperative +// mood: a subject that opens in the past tense or on a gerund records +// that a thing was done, and one that opens on the author narrates the +// author. Verbs that wear one form for every mood are spared, as are +// articles: an explainer may be a noun phrase on purpose. +check_mood :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + msg := measured(s.c) + if msg == "" { + return + } + subject := subject_of(msg) + if strings.has_prefix(subject, "Merge ") || strings.has_prefix(subject, "Squashed ") { + return + } + words := fields(explainer(subject), context.temp_allocator) + if len(words) == 0 { + return + } + first := strings.to_lower(words[0], context.temp_allocator) + what := "" + switch { + case openers[first]: + what = "narration" + case invariant_verbs[first]: + return + case irregular_past[first] || + (len(first) >= 4 && strings.has_suffix(first, "ed") && !strings.has_suffix(first, "eed")): + what = "the past tense" + case len(first) >= 5 && strings.has_suffix(first, "ing") && !non_verb_ing[first]: + what = "a gerund" + case: + return + } + append( + out, + static( + "message-not-imperative", + .Must_Fix, + fmt.aprintf( + "the commit message opens on %s — %q — where the discipline is a command: package: explainer, with the explainer in the imperative mood; a subject that opens in the past tense, on a gerund, or on the author records what was done rather than saying what to do", + what, + words[0], + ), + "open the subject on its verb, in the imperative", + ), + ) +} + +// subject_of is a message's first line. +subject_of :: proc(msg: string) -> string { + if i := strings.index_byte(msg, '\n'); i >= 0 { + return msg[:i] + } + return msg +} + +// explainer is the part of a subject after its package prefix: the part +// after "review:" in "review: measure it". A subject without a lowercase +// prefix is all explainer. +explainer :: proc(subject: string) -> string { + i := strings.index(subject, ": ") + if i < 2 || i > 23 { + return subject + } + for j in 0 ..< i { + r := subject[j] + if !((r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' || r == '_') { + return subject + } + } + return subject[i + 2:] +} + +// check_body asks a large change to say something in its body: a diff of +// more than body_floor_lines owes a body, however short, and no body may +// exceed max_body_words. The diff records what moved; the body is the +// only place the change's why is recorded. A change that only moves text +// around, whose subject says what it did, owes nothing. +check_body :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + msg := measured(s.c) + if msg == "" { + return + } + subject := subject_of(msg) + if strings.has_prefix(subject, "Merge ") || strings.has_prefix(subject, "Squashed ") { + return + } + n := body_words(msg) + if n > max_body_words { + append( + out, + static( + "message-long-body", + .Must_Fix, + fmt.aprintf( + "the commit message's body holds %d words, over the %d the discipline allows; past that a body lists what the diff already shows, and the reader of the log stops before the why", + n, + max_body_words, + ), + "cut the body to the change's why", + ), + ) + return + } + if n > 0 { + return + } + changed := changed_lines(s.c.diff) + if changed <= body_floor_lines || moved(s.c.diff) { + return + } + append( + out, + static( + "message-no-body", + .Must_Fix, + fmt.aprintf( + "the change is %d lines of diff, and the commit message carries no body; the diff records what moved, and the body is the only place the change's why is recorded", + changed, + ), + "write the body, saying why the change is what it is", + ), + ) +} + +// check_temporal reports a changed file whose history names a partner the +// change does not touch: over the last thousand commits before the +// change, at least temporal_coupling of the commits touching either file +// have touched both, at least temporal_support times. Files that change +// together this reliably usually fail together. +check_temporal :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + temporal, counted := s.c.temporal.? + if !counted { + return + } + changed := make(map[string]bool, context.temp_allocator) + for f in s.c.files { + changed[f] = true + } + Pair :: struct { + file: string, + partner: change.Partner, + joint: int, + j: f64, + } + pairs := make([dynamic]Pair, context.temp_allocator) + for file, partners in temporal.partners { + for p in partners { + if changed[p.name] { + continue + } + joint := temporal.commits[file] + temporal.commits[p.name] - p.shared + if joint <= 0 { + continue + } + j := f64(p.shared) / f64(joint) + if j < temporal_coupling { + // The list is nearest first, so the rest of this file's + // partners are further away still. + break + } + if p.shared < temporal_support { + continue + } + append(&pairs, Pair{file, p, joint, j}) + } + } + slice.sort_by_cmp(pairs[:], proc(a, b: Pair) -> slice.Ordering { + if a.partner.shared != b.partner.shared { + return .Less if a.partner.shared > b.partner.shared else .Greater + } + if a.j != b.j { + return .Less if a.j > b.j else .Greater + } + if a.file != b.file { + return .Less if a.file < b.file else .Greater + } + if a.partner.name != b.partner.name { + return .Less if a.partner.name < b.partner.name else .Greater + } + return .Equal + }) + for p, i in pairs { + if i == temporal_findings { + break + } + append( + out, + static( + "history-coupled-file", + .Must_Fix, + fmt.aprintf( + "history ties %s to %s: %d of the %d commits touching either file have touched both, and this change touches %s without %s; files that change together this reliably usually fail together, and the half of the pair left out is where a forgotten change usually is", + p.file, + p.partner.name, + p.partner.shared, + p.joint, + p.file, + p.partner.name, + ), + fmt.aprintf( + "touch %s too, or be sure it stands without this change", + p.partner.name, + ), + file = p.file, + ), + ) + } +} + +// moved is whether a diff's added and removed sides hold the same lines: +// the change rearranged text rather than changing it. +moved :: proc(diff: string) -> bool { + counts := make(map[string]int, context.temp_allocator) + n := 0 + rest := diff + for line in strings.split_lines_iterator(&rest) { + if !strings.has_prefix(line, "+") && !strings.has_prefix(line, "-") { + continue + } + if strings.has_prefix(line, "+++") || strings.has_prefix(line, "---") { + continue + } + s := strings.trim_space(line[1:]) + if s == "" { + continue + } + n += 1 + counts[s] += 1 if line[0] == '+' else -1 + } + for _, v in counts { + if v != 0 { + return false + } + } + return n > 0 +} + +// body_words counts the words below the subject line. +body_words :: proc(msg: string) -> int { + i := strings.index_byte(msg, '\n') + if i < 0 { + return 0 + } + return len(fields(msg[i + 1:], context.temp_allocator)) +} + +// changed_lines counts the lines a diff adds or removes, without the +// diff's own framing. +changed_lines :: proc(diff: string) -> int { + n := 0 + rest := diff + for line in strings.split_lines_iterator(&rest) { + if !strings.has_prefix(line, "+") && !strings.has_prefix(line, "-") { + continue + } + if strings.has_prefix(line, "+++") || strings.has_prefix(line, "---") { + continue + } + n += 1 + } + return n +} + +// content_words is the vocabulary of a text: lowercased, split on +// anything that is not a letter or digit, split again at camel humps, +// depluralised, and stripped of the words that name nothing. +content_words :: proc(text: string, allocator := context.allocator) -> map[string]bool { + out := make(map[string]bool, allocator) + for piece in fields(text, context.temp_allocator) { + for part in humps(piece, context.temp_allocator) { + w := depluralise(strings.to_lower(part, allocator)) + if len(w) > 2 && !stop_words[w] { + out[w] = true + } + } + } + return out +} + +// humps splits containerSniff into container and sniff, so that a message +// naming a thing meets the identifier for it in the diff. +humps :: proc(s: string, allocator := context.allocator) -> []string { + out := make([dynamic]string, allocator) + start := 0 + prev_lower := false + for r, i in s { + upper := r >= 'A' && r <= 'Z' + if i > 0 && upper && prev_lower { + append(&out, s[start:i]) + start = i + } + prev_lower = r >= 'a' && r <= 'z' + } + if start < len(s) { + append(&out, s[start:]) + } + return out[:] +} + +// depluralise drops a trailing s, except where dropping it would leave +// another: class stays class. +depluralise :: proc(w: string) -> string { + if len(w) > 3 && strings.has_suffix(w, "s") && w[len(w) - 2] != 's' { + return w[:len(w) - 1] + } + return w +} + +// frequencies counts, over the repository's subjects, in how many of them +// each content word appears. +frequencies :: proc(history: []string, allocator := context.allocator) -> map[string]int { + out := make(map[string]int, allocator) + for s in history { + for w in content_words(s, allocator) { + out[w] += 1 + } + } + return out +} + +// diff_words is the vocabulary of what a change touches: the file paths, +// and the words of every line it adds or removes. +diff_words :: proc(c: ^change.Change, allocator := context.allocator) -> map[string]bool { + out := make(map[string]bool, allocator) + for path in c.files { + stem := path + if dot := strings.last_index_byte(path, '.'); dot > strings.last_index_byte(path, '/') { + stem = path[:dot] + } + for w in content_words(stem, allocator) { + out[w] = true + } + } + rest := c.diff + for line in strings.split_lines_iterator(&rest) { + if strings.has_prefix(line, "+++") || + strings.has_prefix(line, "---") || + strings.has_prefix(line, "@@") || + strings.has_prefix(line, "diff ") || + strings.has_prefix(line, "index ") || + strings.has_prefix(line, "\\ ") { + continue + } + if strings.has_prefix(line, "+") || strings.has_prefix(line, "-") { + for w in content_words(line[1:], allocator) { + out[w] = true + } + } + } + return out +} + +// shannon_entropy is the entropy of s in bits per byte, over its bytes. +shannon_entropy :: proc(s: string) -> f64 { + n := len(s) + if n == 0 { + return 0 + } + counts: [256]int + for i in 0 ..< n { + counts[s[i]] += 1 + } + h: f64 + for c in counts { + if c == 0 { + continue + } + p := f64(c) / f64(n) + h -= p * math.log2(p) + } + return h +} + +// compression_ratio is the share of its length s keeps after zlib, at +// the library's default level, which is the ratio the threshold was +// measured with. +compression_ratio :: proc(s: string) -> f64 { + if len(s) == 0 { + return 1 + } + bound := zlib.compressBound(zlib.uLong(len(s))) + dest := make([]byte, int(bound), context.temp_allocator) + dest_len := zlib.uLongf(bound) + source := transmute([]byte)s + if zlib.compress2(raw_data(dest), &dest_len, raw_data(source), zlib.uLong(len(s)), -1) != 0 { + return 1 + } + return f64(dest_len) / f64(len(s)) +} diff --git a/odin/check/names.odin b/odin/check/names.odin @@ -0,0 +1,188 @@ +package check + +// Three of the naming rules need no judgement. Whether a name repeats its +// package, shadows something the language already names, or carries an +// invented abbreviation is a comparison against a list. + +import "base:runtime" +import "core:fmt" +import "core:strings" + +import "../change" +import "../finding" + +@(private = "file") +predeclared: map[string]bool +@(private = "file") +stdlib: map[string]bool +@(private = "file") +globals: map[string]bool +@(private = "file") +builtins: map[string]bool +@(private = "file") +abbreviations: map[string]bool + +@(init) +init_name_lists :: proc "contextless" () { + context = runtime.default_context() + // Go's universe-block identifiers. A package-level name that takes + // one compiles, and then the builtin is gone for the whole package. + predeclared = set( + `append bool byte cap clear close complex complex64 complex128 copy delete error false + float32 float64 imag int int8 int16 int32 int64 iota len make max min new nil panic print println + real recover rune string true uint uint8 uint16 uint32 uint64 uintptr any comparable`, + ) + // The standard library packages a Go file is likeliest to import. + stdlib = set( + `bufio bytes cmp context errors fmt io log maps math os path reflect regexp slices sort + strconv strings sync testing time unicode url http json exec filepath rand hash flag template`, + ) + // The names a browser or Node runtime already binds. + globals = set( + `Promise Map Set Array Object Error JSON Math Date Symbol String Number Boolean console + window document process require module exports fetch event location history navigator`, + ) + // Python's builtins, the ones a module-level name is likeliest to take + // by accident. + builtins = set( + `abs all any bin bool bytes callable chr dict dir divmod enumerate eval exec filter + float format frozenset getattr hasattr hash help hex id input int isinstance issubclass iter len list + locals map max min next object oct open ord pow print property range repr reversed round set setattr + slice sorted str sum super tuple type vars zip`, + ) + // The shortenings the discipline rejects. Established ones — id, url, + // ctx, msg, err, buf, cmd, tmp — are words in their own right. + abbreviations = set( + `cfg mgr mgmt hdlr hndlr hndl svc ctrl ctlr btn cnt amt qty calc tbl usr pwd dflt nbr mdl + srvr clnt rslt chk upd`, + ) +} + +// check_names reports the naming faults a comparison can settle: stutter +// against the package, shadowing of a predeclared or well-known name, +// and abbreviations the discipline rejects. +check_names :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for sym in s.c.symbols { + if is_test_file(sym.file) { + continue + } + stutter(sym, out) + shadow(sym, out) + abbreviated(sym, out) + } +} + +// stutter reports a name whose first word is its package. The package's +// name is already said wherever the name is used, so the word is said +// twice: ico.IcoEntry. A type named exactly for its package is the +// language's own idiom — time.Time — and is spared, as is anything in +// package main, which nothing qualifies. +stutter :: proc(sym: change.Symbol, out: ^[dynamic]finding.Finding) { + if sym.pkg == "" || sym.pkg == "main" || !sym.exported { + return + } + words := split(sym.name, context.temp_allocator) + if len(words) < 2 { + return + } + pkg, _ := strings.replace_all(sym.pkg, "_", "", context.temp_allocator) + if strings.to_lower(words[0], context.temp_allocator) != + strings.to_lower(pkg, context.temp_allocator) { + return + } + append( + out, + static( + "no-stutter", + .Consider, + fmt.aprintf( + "%s repeats its package: %s.%s says %s twice", + sym.name, + sym.pkg, + sym.name, + words[0], + ), + fmt.aprintf( + "drop the package's word: %s.%s", + sym.pkg, + strings.join(words[1:], "", context.temp_allocator), + ), + file = sym.file, + line = sym.line, + symbol = sym.name, + ), + ) +} + +// shadow reports a name the language or its runtime already means +// something by: Go's predeclared identifiers and standard library +// packages, the runtime's globals for TypeScript and JavaScript, the +// builtins for Python. +shadow :: proc(sym: change.Symbol, out: ^[dynamic]finding.Finding) { + what, why: string + switch { + case strings.has_suffix(sym.file, ".go"): + switch { + case predeclared[sym.name]: + what, why = "a predeclared identifier", "the builtin is gone for the whole package" + case stdlib[sym.name]: + what, why = + "a standard library package", + "no file in the package can import it beside this name" + } + case grammar_of(sym.file) != "": + if globals[sym.name] { + what, why = + "a runtime global", "the runtime's is shadowed for every reader of the module" + } + case strings.has_suffix(sym.file, ".py"): + if builtins[sym.name] { + what, why = "a builtin", "the builtin is gone for the whole module" + } + } + if what == "" { + return + } + append( + out, + static( + "no-shadow", + .Consider, + fmt.aprintf("%s is %s, and %s", sym.name, what, why), + "name it for what it is here, in a word the language does not already use", + file = sym.file, + line = sym.line, + symbol = sym.name, + ), + ) +} + +// abbreviated reports a name carrying an invented abbreviation: a word +// the reader has to expand rather than read. +abbreviated :: proc(sym: change.Symbol, out: ^[dynamic]finding.Finding) { + hit := make([dynamic]string, context.temp_allocator) + for word in split(sym.name, context.temp_allocator) { + if abbreviations[strings.to_lower(word, context.temp_allocator)] { + append(&hit, word) + } + } + if len(hit) == 0 { + return + } + append( + out, + static( + "abbreviation", + .Consider, + fmt.aprintf( + "%s abbreviates %s; an invented abbreviation is a word the reader expands rather than reads", + sym.name, + strings.join(hit[:], ", ", context.temp_allocator), + ), + "write the word out", + file = sym.file, + line = sym.line, + symbol = sym.name, + ), + ) +} diff --git a/odin/check/prose.odin b/odin/check/prose.odin @@ -0,0 +1,78 @@ +package check + +// A comment whose words are the code's own words says nothing the code +// does not. It is never a claim, so the claims job should not be paying to +// read it, and the habit of narrating each line is the habit this +// catches. + +import "core:fmt" +import "core:strings" + +import "../change" +import "../finding" + +// min_restated_words is the fewest content words a comment needs before +// it can be said to restate anything: one word is a label. +min_restated_words :: 2 + +// restates is whether a comment's content words all appear in the code +// it sits above. A directive, a dismissal, a task marker and a link are +// not prose about the code and are never said to restate it. +restates :: proc(comment: change.Located) -> bool { + if comment.below == "" || directive(comment.text) { + return false + } + words := content_words(comment.text, context.temp_allocator) + if len(words) < min_restated_words { + return false + } + code := content_words(subject_of(comment.below), context.temp_allocator) + for w in words { + if !code[w] { + return false + } + } + return true +} + +// directive is whether a comment is one the tools read rather than a +// person: a compiler or linter instruction, a dismissal, a task marker, a +// link. +directive :: proc(text: string) -> bool { + trimmed := strings.trim_space(text) + for prefix in ([]string{"go:", "nolint", "eslint", "@ts-", "prettier", "review:ignore", "#!", "+build", "lint:"}) { + if strings.has_prefix(trimmed, prefix) { + return true + } + } + for marker in ([]string{"TODO", "FIXME", "XXX", "HACK", "http://", "https://"}) { + if strings.contains(trimmed, marker) { + return true + } + } + return false +} + +// check_restating reports the comments the change adds whose every word +// the code below already says. +check_restating :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for comment in s.c.comments { + if !restates(comment) { + continue + } + append( + out, + static( + "comment-restates-code", + .Note, + fmt.aprintf( + "the comment %q says only what the line below it says; a comment that narrates the code is read twice and informs once", + first_line(comment.text, context.temp_allocator), + ), + "say why, or say nothing", + file = comment.file, + line = comment.line, + ), + ) + } +} diff --git a/odin/check/rules.odin b/odin/check/rules.odin @@ -0,0 +1,183 @@ +package check + +// The rules are what an agent is handed. A finding cites one by id, and +// the id has to lead somewhere: the description of the deterministic check +// that measured it. Printing them from the binary is what makes the rules +// travel with it. + +import "core:fmt" +import "core:strings" + +// Rule describes one deterministic check, for the reader who met its id +// in a finding. +Rule :: struct { + id: string, + description: string, +} + +// rules are the deterministic checks, in the order the readme lists +// them. Each id here is one a finding can carry with job "static". +rules := []Rule { + { + "go-build", + "the Go compiler cannot build a package the change touched; must-fix wherever the error lands", + }, + { + "go-vet/<analyzer>", + "go vet's finding, on a line the change adds — with review-vet's extra analysers where it is installed; nilness and vet's own set are must-fix, shadow and unusedwrite consider, modernize note", + }, + { + "staticcheck/<code>", + "the Go analyser's finding, on a line the change adds; SA is must-fix, S and U consider, the rest note", + }, + { + "odin-check", + "odin check -vet -strict-style cannot check a package the change touched: a type error is must-fix wherever it lands, a vet failure consider, a style failure note", + }, + { + "tsc/<code>", + "the TypeScript compiler's error under the project's own tsconfig; must-fix wherever it lands; working tree only", + }, + { + "ruff/<code>", + "ruff's finding on a line the change adds; pyflakes codes consider, style codes note", + }, + { + "mypy/<code>", + "mypy's type error on a line the change adds, missing imports ignored; working tree only", + }, + { + "cargo/<code>", + "cargo check's — or clippy's — diagnostic: an error is must-fix wherever it lands, a warning consider on a line the change adds", + }, + { + "semgrep/<rule>", + "a semgrep match on a line the change adds, under the repository's own .semgrep.yml or the registry's p/default pack; the rule's severity is kept: ERROR must-fix, WARNING consider, INFO note", + }, + { + "message-low-entropy", + "the commit message's Shannon entropy is under 3.2 bits per byte: a phrase repeated rather than a description", + }, + { + "message-boilerplate", + "the commit message keeps under 20% of its length after zlib: a block of text pasted or repeated whole", + }, + { + "message-common-words", + "the commit message is made only of the repository's commonest commit words and names nothing the change touches", + }, + { + "message-frustration", + "the commit message is an exclamation — oops, whoops, damn — where a description should be", + }, + { + "message-not-imperative", + "the subject's first word after its package prefix is past tense, a gerund, or the author", + }, + { + "message-no-body", + "a change over 50 lines carries no body, and the diff does not only move text around", + }, + {"message-long-body", "the body is over 150 words, listing what the diff already shows"}, + { + "history-coupled-file", + "history ties a changed file to a partner the change does not touch: Jaccard at least 0.7 over at least 5 shared commits", + }, + { + "message-names-unknown", + "the commit message names an identifier, path or call that is in neither the diff nor the repository at the end of the change", + }, + { + "formatting-mixed-in", + "half or more of the change's lines change only whitespace, and at least 10 change something else: a reformatting with logic in it", + }, + { + "suppression-added", + "the change adds its own review:ignore dismissal, before the readers have run", + }, + {"test-deleted", "the change deletes a test function that no added test renames"}, + {"no-stutter", "an exported name's first word is its package: ico.IcoEntry says ico twice"}, + { + "no-shadow", + "a new name is a predeclared identifier, a standard library package, or a runtime global", + }, + {"abbreviation", "a new name carries an invented abbreviation: cfg, mgr, hdlr, svc, btn, cnt"}, + { + "test-no-assertion", + "an added or altered test has no call that could fail it; it can only fail by crashing", + }, + { + "assertion-always-true", + "an assertion in an added or altered test holds whatever the code does: a literal true, two literals, or a value against itself", + }, + { + "duplicate-body", + "a new function's body already exists, token for token (must-fix) or in shape with every name changed (consider)", + }, + { + "function-too-long", + "a new function is over 150 lines; 95% of measured functions fit in 109", + }, + { + "nesting-too-deep", + "a new function nests blocks more than 5 deep; 99% of measured functions stay within 6", + }, + { + "comment-restates-code", + "every content word of an added comment is in the line of code below it", + }, + { + "todo-without-reference", + "an added TODO, FIXME, XXX or HACK names no issue, ticket, link or person", + }, + { + "commented-out-code", + "an added comment run holds code: two statement-shaped lines, or one beyond doubt", + }, + { + "debug-leftover", + "an added line is a debugger statement, a breakpoint, dbg!, spew.Dump, console.log, or a DEBUG print", + }, + { + "error-swallowed", + "an added line drops an error: _ = err, an empty catch, an except that passes", + }, + { + "new-symbol-unreferenced", + "nothing in the repository refers to a new declaration but its own line", + }, + { + "code-without-tests", + "the change adds 50 or more lines of code, touches no test, and the repository keeps tests", + }, +} + +// catalogue is every rule, as review rules prints them. +catalogue :: proc(allocator := context.allocator) -> string { + b := strings.builder_make(allocator) + strings.write_string(&b, "# Deterministic checks\n\n") + for r in rules { + fmt.sbprintf(&b, "- `%s` — %s\n", r.id, r.description) + } + return strings.to_string(b) +} + +// describe is one rule's description, by its id or by the family a +// tool's rules share: go-vet/anything is go-vet/<analyzer>. +describe :: proc(id: string) -> (description: string, ok: bool) { + family := id + if i := strings.index_byte(id, '/'); i >= 0 { + family = id[:i] + } + for r in rules { + if r.id == id || + (strings.contains(id, "/") && + strings.has_prefix( + r.id, + strings.concatenate({family, "/"}, context.temp_allocator), + )) { + return r.description, true + } + } + return "", false +} diff --git a/odin/check/shape.odin b/odin/check/shape.odin @@ -0,0 +1,121 @@ +package check + +// A function's size and depth are measured, not judged. The thresholds +// were read off 8,255 functions: length sits between the 95th percentile +// (109 lines) and the 99th (257); depth at the 99th (6). + +import "core:fmt" + +import "../finding" + +// max_function_lines is the length past which a function is several; +// max_nesting the block depth past which a reader is holding more +// context than the function's name gave them. +max_function_lines :: 150 +max_nesting :: 5 + +// check_shape reports a new function that is too long or too deeply +// nested to read as one thing. +check_shape :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for sym in s.c.symbols { + if sym.kind != "func" || sym.body == "" { + continue + } + if n := line_count(sym.body); n > max_function_lines { + append( + out, + static( + "function-too-long", + .Consider, + fmt.aprintf( + "%s is %d lines, over the %d past which a function is several; 95%% of measured functions fit in 109", + sym.name, + n, + max_function_lines, + ), + "split it at the point where the reader has to remember what came before", + file = sym.file, + line = sym.line, + symbol = sym.name, + ), + ) + } + if d := nesting(sym.body); d > max_nesting { + append( + out, + static( + "nesting-too-deep", + .Consider, + fmt.aprintf( + "%s nests %d blocks deep, over the %d past which a reader is holding more than the name told them; 99%% of measured functions stay within 6", + sym.name, + d, + max_nesting, + ), + "return early, or lift the inner blocks into functions of their own", + file = sym.file, + line = sym.line, + symbol = sym.name, + ), + ) + } + } +} + +line_count :: proc(body: string) -> int { + n := 1 + for i in 0 ..< len(body) { + if body[i] == '\n' { + n += 1 + } + } + return n +} + +// nesting is the deepest block within a body, counted by braces with +// strings and comments read through, less the body's own pair. +nesting :: proc(body: string) -> int { + depth, deepest := 0, 0 + in_string, in_raw, in_line, in_block: bool + quote: byte + i := 0 + for i < len(body) { + c := body[i] + switch { + case in_line: + if c == '\n' { + in_line = false + } + case in_block: + if c == '*' && i + 1 < len(body) && body[i + 1] == '/' { + in_block = false + i += 1 + } + case in_raw: + if c == '`' { + in_raw = false + } + case in_string: + if c == '\\' { + i += 1 + } else if c == quote || c == '\n' { + in_string = false + } + case c == '/' && i + 1 < len(body) && body[i + 1] == '/': + in_line = true + case c == '/' && i + 1 < len(body) && body[i + 1] == '*': + in_block = true + case c == '`': + in_raw = true + case c == '"' || c == '\'': + in_string, quote = true, c + case c == '{': + depth += 1 + deepest = max(deepest, depth) + case c == '}': + depth -= 1 + } + i += 1 + } + return max(deepest - 1, 0) +} diff --git a/odin/check/tests.odin b/odin/check/tests.odin @@ -0,0 +1,185 @@ +package check + +// A test with no assertion in it is the plainest shape of a test that +// cannot fail, and the shape is visible without reading what the test +// means. Whether an assertion that is there asserts anything is the tests +// job's, except for the tautologies a pattern can see. + +import "core:fmt" +import "core:strings" + +import "../change" +import "../finding" + +// go_test_param reads the name a Go test gives its testing.T, so that a +// test naming it tc or tt is read by the name it uses. +go_test_param :: `^func \w+\((\w+) \*testing\.T\)` + +// check_test_assertions reports an added or altered test whose body +// asserts nothing: no failing call, no subtest, no helper handed the test. +check_test_assertions :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for t in s.c.tests { + if !assertless(t) { + continue + } + append( + out, + static( + "test-no-assertion", + .Consider, + fmt.aprintf( + "%s asserts nothing: no call in its body can fail it, so it passes whatever the code does, and can only fail by crashing", + t.name, + ), + "assert the value the test exists to check, or remove the test", + file = t.file, + line = t.line, + symbol = t.name, + ), + ) + } +} + +// assertless is whether a test body holds nothing that could fail it, in +// the shapes the tool's languages assert in. +assertless :: proc(t: change.Function) -> bool { + body := t.body + switch { + case strings.has_suffix(t.file, ".go"): + if strings.has_prefix(t.name, "Benchmark") || + strings.has_prefix(t.name, "Fuzz") || + strings.has_prefix(t.name, "Example") || + t.name == "TestMain" { + return false + } + param := "t" + if m, ok := capture(go_test_param, subject_of(body)); ok { + param = m[1] + } + for shape in ([]string{".Error", ".Fatal", ".Fail", ".Run(", ".Skip"}) { + if strings.contains( + body, + strings.concatenate({param, shape}, context.temp_allocator), + ) { + return false + } + } + for shape in ([]string{"panic(", "require.", "assert.", "is."}) { + if strings.contains(body, shape) { + return false + } + } + // A test that hands its testing.T to a helper may assert through + // it, and the helper is not here to read. + return !matches(fmt.tprintf(`[(,]\s*%s\s*[,)]`, param), body) + case grammar_of(t.file) != "": + for shape in ([]string{"expect(", "expect.", "assert", "should", "toThrow", "fail(", ".rejects", ".resolves", "throw "}) { + if strings.contains(body, shape) { + return false + } + } + return true + case strings.has_suffix(t.file, ".odin"): + for shape in ([]string{"testing.expect", "testing.fail", "expect(", "expectf(", "expect_value(", "assert(", "panic("}) { + if strings.contains(body, shape) { + return false + } + } + return true + case strings.has_suffix(t.file, ".py"): + for shape in ([]string{"assert ", "assert(", "self.assert", "pytest.raises", "pytest.fail", "raise ", ".assert_"}) { + if strings.contains(body, shape) { + return false + } + } + return true + case strings.has_suffix(t.file, ".rs"): + for shape in ([]string{"assert!", "assert_eq!", "assert_ne!", "panic!", "unwrap()", "expect(", "?;", "should_panic"}) { + if strings.contains(body, shape) { + return false + } + } + return true + } + return false +} + +// tautologies are the assertion shapes that hold whatever the code does: +// a literal true asserted, two literals compared. They are what a test +// reaches for once a rule says a test must assert. +@(private = "file") +tautologies := []string { + `\b(?:assert|require)\.(?:True|NoError|Nil|Empty)\(\s*\w+\s*,\s*(?:true|nil)\s*\)`, + `\bassert\s+(?:True|1|"[^"]+"|'[^']+')\s*(?:,|$)`, + `\bassert(?:True|Is)\(\s*True\s*[,)]`, + `\bexpect\(\s*true\s*\)\.(?:toBe\(\s*true\s*\)|toBeTruthy\(\))`, + `\bassert!\(\s*true\s*\)`, + `\b(?:testing\.)?expect\(\s*\w+\s*,\s*true\s*\)`, + `\bexpect\(\s*(-?\d+|"[^"]*"|'[^']*')\s*\)\.(?:toBe|toEqual|toStrictEqual)\(\s*(-?\d+|"[^"]*"|'[^']*')\s*\)`, + `\bassert_eq!\(\s*(-?\d+|"[^"]*")\s*,\s*(-?\d+|"[^"]*")\s*\)`, + `\b(?:assert|require)\.Equal\(\s*\w+\s*,\s*(-?\d+|"[^"]*")\s*,\s*(-?\d+|"[^"]*")\s*\)`, + `\bassert\s+(-?\d+|"[^"]*"|'[^']*')\s*==\s*(-?\d+|"[^"]*"|'[^']*')`, +} + +// self_compare matches an expression compared with itself, in the shapes +// the languages assert with: x == x, expect(x).toBe(x), assert_eq!(x, x), +// assert.Equal(t, x, x). +@(private = "file") +self_compare := []string { + `\b([\w.]+(?:\([^()]*\))?)\s*(?:==|!=)\s*([\w.]+(?:\([^()]*\))?)`, + `\bexpect\(\s*([^()]+)\s*\)\.(?:toBe|toEqual|toStrictEqual)\(\s*([^()]+)\s*\)`, + `\bassert_(?:eq|ne)!\(\s*(.+?)\s*,\s*(.+?)\s*\);?\s*$`, + `\b(?:assert|require)\.(?:Equal|NotEqual)\(\s*\w+\s*,\s*(.+?)\s*,\s*(.+?)\s*\)\s*$`, +} + +// check_tautologies reports an assertion in an added or altered test that +// holds whatever the code does. +check_tautologies :: proc(s: Scope, out: ^[dynamic]finding.Finding) { + for t in s.c.tests { + rest := t.body + i := 0 + for line in strings.split_lines_iterator(&rest) { + if why := tautological(line); why != "" { + append( + out, + static( + "assertion-always-true", + .Must_Fix, + fmt.aprintf( + "%s asserts %s: %s; the assertion holds whatever the code does, so the test cannot fail on it", + t.name, + why, + strings.trim_space(line), + ), + "assert the value the code produced against the value it should have", + file = t.file, + line = t.line + i, + symbol = t.name, + ), + ) + } + i += 1 + } + } +} + +// tautological says what is tautological about an assertion line, or +// nothing. +tautological :: proc(line: string) -> string { + trimmed := strings.trim_space(line) + if trimmed == "" || strings.has_prefix(trimmed, "//") || strings.has_prefix(trimmed, "#") { + return "" + } + for pattern in tautologies { + if matches(pattern, trimmed) { + return "a constant" + } + } + for pattern in self_compare { + if m, ok := capture(pattern, trimmed); + ok && len(m) > 2 && strings.trim_space(m[1]) == strings.trim_space(m[2]) { + return "a value against itself" + } + } + return "" +} diff --git a/odin/review/main.odin b/odin/review/main.odin @@ -3,12 +3,14 @@ // revision range given. // // review [--json] [--verbose] [--show] [rev] +// review rules [<rule>] package main import "core:fmt" import "core:os" import "../change" +import "../check" import "../finding" import "../git" import "../report" @@ -17,6 +19,10 @@ import "../tree" main :: proc() { as_json, verbose, show: bool rev := "" + if len(os.args) > 1 && os.args[1] == "rules" { + rules(os.args[2:]) + return + } for arg in os.args[1:] { switch arg { case "--json": @@ -64,12 +70,15 @@ main :: proc() { defer tree.close(t) change.read(&c, t) + c.index, _ = change.index(t) if show { list(c, t) } - findings: [dynamic]finding.Finding - finding.sort(findings[:]) - kept, dismissed := report.filter(t.dir, findings[:]) + // The deterministic checks ask nothing of a provider, so they run + // before one is built and survive a model that cannot answer. + findings := check.run(check.scope_of(&c, t)) + finding.sort(findings) + kept, dismissed := report.filter(t.dir, findings) env := report.Contract { status = report.status_of(c, 0), findings = kept, @@ -110,6 +119,21 @@ list :: proc(c: change.Change, t: tree.Tree) { for l in c.comments { fmt.printfln("comment %s:%d %s", l.file, l.line, l.text) } - declared, _ := change.index(t) - fmt.printfln("=== index: %d declarations ===", len(declared)) + fmt.printfln("=== index: %d declarations ===", len(c.index)) +} + +// rules prints the catalogue, or one rule's description, so that an +// agent given a finding can read what it was judged against without +// leaving the terminal. +rules :: proc(args: []string) { + if len(args) == 0 { + fmt.print(check.catalogue()) + return + } + description, ok := check.describe(args[0]) + if !ok { + fmt.eprintfln("review: no rule called %q", args[0]) + os.exit(1) + } + fmt.printfln("%s, a deterministic check:\n\n%s", args[0], description) } diff --git a/odin/tree/tree.odin b/odin/tree/tree.odin @@ -10,6 +10,7 @@ package tree import "core:fmt" import "core:os" import "core:path/filepath" +import "core:slice" import "core:strings" import "jfm:sh" @@ -93,6 +94,28 @@ read :: proc(t: Tree, name: string, allocator := context.allocator) -> (data: [] return contents, err == nil } +// sources reads every tracked file that is text, so that a search over +// the repository is a loop over memory rather than a program run. Binary +// files are left out by the cheapest test there is: a NUL byte in the +// first kilobyte. +sources :: proc(t: Tree, allocator := context.allocator) -> (out: map[string][]byte, ok: bool) { + tracked := files(t, context.temp_allocator) or_return + out = make(map[string][]byte, allocator) + for name in tracked { + data, readable := read(t, name, allocator) + if !readable { + continue // A tracked path the tree cannot read is a link or gone. + } + head := data[:min(len(data), 1024)] + if slice.contains(head, 0) { + delete(data, allocator) + continue + } + out[strings.clone(name, allocator)] = data + } + return out, true +} + // exists reports whether the tree holds the path as a file. exists :: proc(t: Tree, name: string) -> bool { return os.is_file(path(t, name, context.temp_allocator))