Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion internal/plan/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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.
Expand Down
49 changes: 49 additions & 0 deletions internal/plan/plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
}
46 changes: 42 additions & 4 deletions internal/plan/planskip.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading