diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 7897c63..a2fcc4f 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -362,7 +362,10 @@ subcommand the page itself introduces as linting, checking, or validating for pr as in "The `recheck` command lints Markdown files for structure and prose problems", is check mode for that document. The inference is by subcommand, never by binary, since a linter run over a bad configuration exits nonzero too and blessing the binary would turn -that into a hollow pass. A nonzero exit from either is the tool working. A crash stays a +that into a hollow pass. A flag the page says implies one of the check-mode flags, as in +"`--format sarif` implies `--fix=false`", is a check-mode flag for that document, read with +its value as written, and an explicit `--fix` beside it turns fixing back on. A nonzero +exit from either is the tool working. A crash stays a failure, and the classifier now enforces it: a check-mode line whose output holds a traceback or a panic falls through to the crash rules. Output the document shows under a command as a comment, which the tool then printed, is documented behavior as well: a diff --git a/internal/plan/plan.go b/internal/plan/plan.go index b4ec0f8..ec2ed5c 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -224,6 +224,10 @@ func BuildPlan(repo, dir, markdown string, binaries []string, installs []PlanIns for _, sub := range describedAsLinter(markdown) { pl.findingSubs[sub] = true } + pl.findingFlags = maps.Clone(findingFlags) + for _, f := range impliedFindingFlags(markdown) { + pl.findingFlags[f] = true + } for b := range pl.binaries { if describedAsWatcher(markdown, b) { pl.watcher = true @@ -258,7 +262,7 @@ func (pl *planner) flagNonzeroOK() { for li := range pl.plan.Steps[si].Lines { l := &pl.plan.Steps[si].Lines[li] flat := docblock.Flatten(l.Cmd) - if bin, _ := InvokedBinary(flat, pl.binaries); bin != "" && findingFlagged(flat) { + if bin, _ := InvokedBinary(flat, pl.binaries); bin != "" && pl.findingFlagged(flat) { l.NonzeroOK = true } } @@ -328,6 +332,9 @@ type planner struct { // this document: the package-level ones and those its prose introduces as // linting. findingSubs map[string]bool + // findingFlags holds the flags that put a line in check mode for this + // document: the fixed set plus any the document says imply one of them. + findingFlags map[string]bool // staged marks a document that has run git add on a line that is not // skipped. It is the only repository state the planner tracks, because a // commit is the only allowed git form that needs any. diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 070d6d0..c110b7f 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -1427,3 +1427,52 @@ func TestLostDirectorySpansBlocks(t *testing.T) { }) } } + +// TestImpliedFindingFlag checks that a flag the document says implies a +// check-mode flag puts a line in check mode for that document only. +func TestImpliedFindingFlag(t *testing.T) { + t.Parallel() + sarif := "# SARIF\n\n`--format sarif` implies `--fix=false`, so files are not modified.\n\n" + tests := []struct { + Markdown string + WantFlags []string + WantNonzero []bool + }{{ // Test 0: The implied flag, with its value as a separate word or joined. + Markdown: sarif + "```sh\ntool run --format sarif\n```\n\n" + + "```sh\ntool run --format=sarif > out.json\n```\n", + WantFlags: []string{"--format sarif"}, WantNonzero: []bool{true, true}, + }, { // Test 1: The implied flag must be one of the fixed set. + Markdown: "# Text\n\n`--fix` implies `--format text`.\n\n```sh\ntool run --fix\n```\n", + WantNonzero: []bool{false}, + }, { // Test 2: Another value of the same flag is not check mode. + Markdown: sarif + "```sh\ntool run --format json\n```\n", + WantFlags: []string{"--format sarif"}, + WantNonzero: []bool{false}, + }, { // Test 3: The sentence on another page does not carry over. + Markdown: "# Run\n\n```sh\ntool run --format sarif\n```\n", + WantNonzero: []bool{false}, + }, { // Test 4: An explicit --fix beside the implied flag turns fixing back on. + Markdown: sarif + "```sh\ntool run --format sarif --fix\n```\n", + WantFlags: []string{"--format sarif"}, + WantNonzero: []bool{false}, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + if diff := cmp.Diff(test.WantFlags, impliedFindingFlags(test.Markdown), + cmpopts.EquateEmpty()); diff != "" { + t.Errorf("impliedFindingFlags mismatch (-want +got):\n%s", diff) + } + plan := BuildPlan("repo", "", test.Markdown, []string{"tool"}, nil, nil, testRecognizer()) + var got []bool + for _, l := range projectPlan(plan) { + for _, pl := range l { + got = append(got, pl.NonzeroOK) + } + } + if diff := cmp.Diff(test.WantNonzero, got); diff != "" { + t.Errorf("NonzeroOK mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/internal/plan/planskip.go b/internal/plan/planskip.go index d62691d..e0fd8fb 100644 --- a/internal/plan/planskip.go +++ b/internal/plan/planskip.go @@ -431,14 +431,52 @@ var findingFlags = map[string]bool{ "--dry-run": true, "--diff": true, } -// findingFlagged reports whether a line carries a check-mode flag. -func findingFlagged(flat string) bool { - for _, f := range shell.WordsOrFields(docblock.StripComment(flat)) { +// findingFlagged reports whether a line carries a check-mode flag, one of the +// fixed set or one the document says implies a member of it. A flag written +// with a separate value, such as --format sarif, is read as that pair, and an +// explicit --fix beside an implied flag turns fixing back on. +func (pl *planner) findingFlagged(flat string) bool { + words := shell.WordsOrFields(docblock.StripComment(flat)) + implied := false + for i, f := range words { if findingFlags[f] { return true } + if fixFlags[f] { + return false + } + switch { + case pl.findingFlags[strings.Replace(f, "=", " ", 1)]: + implied = true + case i+1 < len(words) && pl.findingFlags[f+" "+words[i+1]]: + implied = true + } } - return false + return implied +} + +// fixFlags are flags that turn fixing back on, so a line carrying one next to +// a flag the document says implies check mode is not check mode after all. +var fixFlags = map[string]bool{"--fix": true, "--fix=true": true} + +// reImpliesFinding matches a sentence that says one flag implies a check-mode +// flag, as in "`--format sarif` implies `--fix=false`, so files are not +// modified". The implied flag must be one of the fixed set, so the document +// extends check mode only by naming a flag kibble already reads that way. +var reImpliesFinding = regexp.MustCompile( + "`(--[a-z][\\w-]*(?:[= ][\\w.-]+)?)`[^.\\n]{0,40}\\bimplies\\b[^.\\n]{0,40}" + + "`(--fix=false|--no-fix|--check|--dry-run|--diff)`") + +// impliedFindingFlags returns the flags a document says imply a check-mode +// flag, so a line carrying one is check mode for that document. +func impliedFindingFlags(markdown string) []string { + var flags []string + for _, m := range reImpliesFinding.FindAllStringSubmatch(docblock.ProseOnly(markdown), -1) { + if f := strings.ReplaceAll(m[1], "=", " "); !slices.Contains(flags, f) { + flags = append(flags, f) + } + } + return flags } // interactiveFlag reports whether a line asks a binary to run interactively.