feat: Surface option parsing warnings via findings.

cmd/cmd.go

@@ -26,6 +26,7 @@ import (
 	"strings"
 
 	"github.com/google/keep-sorted/keepsorted"
+	"github.com/rs/zerolog/log"
 	flag "github.com/spf13/pflag"
 )
 
@@ -218,10 +219,22 @@ func fix(fixer *keepsorted.Fixer, filenames []string, modifiedLines []keepsorted
 		if err != nil {
 			return false, err
 		}
-		if want, alreadyFixed := fixer.Fix(contents, modifiedLines); fn == stdin || !alreadyFixed {
+		if want, alreadyFixed, warnings := fixer.Fix(fn, contents, modifiedLines); fn == stdin || !alreadyFixed {
 			if err := write(fn, want); err != nil {
 				return false, err
 			}
+			for _, warn := range warnings {
+				log := log.Warn()
+				if warn.Path != stdin {
+					log = log.Str("file", warn.Path)
+				}
+				if warn.Lines.Start == warn.Lines.End {
+					log = log.Int("line", warn.Lines.Start)
+				} else {
+					log = log.Int("start", warn.Lines.Start).Int("end", warn.Lines.End)
+				}
+				log.Msg(warn.Message)
+			}
 		}
 	}
 	return true, nil

goldens/generate-goldens.sh

@@ -18,13 +18,15 @@
 set -euo pipefail
 [[ -n "${DEBUG:-}" ]] && set -x
 
-files=()
-for i in "$@"; do
-  o="${i%%in}out"
-  cp "${i}" "${i%%in}out"
-  files+=( "$o" )
-done
-
 dir="$(dirname "$(realpath "${BASH_SOURCE[0]}")")"
 git_dir="$(git -C "${dir}" rev-parse --show-toplevel)"
-go run "${git_dir}" --id=keep-sorted-test -- "${files[@]}"
+
+for i in "$@"; do
+  out="${i%%in}out"
+  err="${i%%in}err"
+
+  go run "${git_dir}" --id=keep-sorted-test --omit-timestamps - <"${i}" >"${out}" 2>"${err}"
+  if (( $(wc -l < "${err}") == 0 )); then
+    rm "${err}"
+  fi
+done

goldens/golden_test.go

@@ -15,11 +15,14 @@
 package golden_test
 
 import (
+	"errors"
 	"io"
+	"maps"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"runtime"
+	"slices"
 	"strings"
 	"testing"
 
@@ -49,7 +52,7 @@ func TestGoldens(t *testing.T) {
 		t.Fatalf("Did not find any golden files.")
 	}
 
-	needsRegen := make(chan string, len(tcs))
+	needsRegen := make(chan string, 2*len(tcs))
 	t.Run("group", func(t *testing.T) {
 		for _, tc := range tcs {
 			tc := tc
@@ -61,16 +64,18 @@ func TestGoldens(t *testing.T) {
 					t.Fatalf("Could not open .in file: %v", err)
 				}
 
-				out, err := os.Open(filepath.Join(dir, tc+".out"))
+				wantOut, err := os.ReadFile(filepath.Join(dir, tc+".out"))
 				if err != nil {
-					t.Fatalf("Could not open .out file: %v", err)
+					t.Fatalf("Could not read .out file: %v", err)
 				}
-				want, err := io.ReadAll(out)
+				wantErr, err := os.ReadFile(filepath.Join(dir, tc+".err"))
 				if err != nil {
-					t.Fatalf("Could not read .out file: %v", err)
+					if !errors.Is(err, os.ErrNotExist) {
+						t.Fatalf("Could not read .err file: %v", err)
+					}
 				}
 
-				cmd := exec.Command("go", "run", gitDir, "--id=keep-sorted-test", "-")
+				cmd := exec.Command("go", "run", gitDir, "--id=keep-sorted-test", "--omit-timestamps", "-")
 				cmd.Stdin = in
 				stdout, err := cmd.StdoutPipe()
 				if err != nil {
@@ -84,15 +89,16 @@ func TestGoldens(t *testing.T) {
 					t.Errorf("could not start keep-sorted: %v", err)
 				}
 
-				if stderr, err := io.ReadAll(stderr); err != nil {
+				if gotErr, err := io.ReadAll(stderr); err != nil {
 					t.Errorf("could not read keep-sorted stderr: %v", err)
-				} else if len(stderr) != 0 {
-					t.Errorf("keep-sorted stderr: %s", string(stderr))
+				} else if diff := cmp.Diff(strings.Split(string(wantErr), "\n"), strings.Split(string(gotErr), "\n")); diff != "" {
+					t.Errorf("keep-sorted stderr:\n%s", diff)
+					needsRegen <- inFile
 				}
 
-				if got, err := io.ReadAll(stdout); err != nil {
+				if gotOut, err := io.ReadAll(stdout); err != nil {
 					t.Errorf("could not read keep-sorted stdout: %v", err)
-				} else if diff := cmp.Diff(strings.Split(string(want), "\n"), strings.Split(string(got), "\n")); diff != "" {
+				} else if diff := cmp.Diff(strings.Split(string(wantOut), "\n"), strings.Split(string(gotOut), "\n")); diff != "" {
 					t.Errorf("keep-sorted stdout diff (-want +got):\n%s", diff)
 					needsRegen <- inFile
 				}
@@ -105,13 +111,13 @@ func TestGoldens(t *testing.T) {
 	})
 
 	close(needsRegen)
-	var files []string
+	files := make(map[string]bool)
 	for f := range needsRegen {
-		files = append(files, f)
+		files[f] = true
 	}
 
 	if len(files) != 0 {
-		t.Logf("Run the following to fix: %s %s", filepath.Join(gitDir, "goldens/generate-goldens.sh"), strings.Join(files, " "))
+		t.Logf("Run the following to fix: %s %s", filepath.Join(gitDir, "goldens/generate-goldens.sh"), strings.Join(slices.Sorted(maps.Keys(files)), " "))
 	}
 }
 

goldens/simple.err

@@ -0,0 +1,2 @@
+WRN skip_lines has invalid value: -1 line=105
+WRN unrecognized option "foo" line=105

goldens/simple.in

@@ -100,3 +100,10 @@ A
 
 B
 // keep-sorted-test end
+
+Invalid option
+  keep-sorted-test start group=yes skip_lines=-1 foo=bar
+  2
+  1
+  3
+  keep-sorted-test end

goldens/simple.out

@@ -99,3 +99,10 @@ B
 
 C
 // keep-sorted-test end
+
+Invalid option
+  keep-sorted-test start group=yes skip_lines=-1 foo=bar
+  1
+  2
+  3
+  keep-sorted-test end

keepsorted/block.go

@@ -16,7 +16,6 @@ package keepsorted
 
 import (
 	"cmp"
-	"fmt"
 	"slices"
 	"strings"
 
@@ -68,7 +67,7 @@ const (
 // include is a function that lets the caller determine if a particular block
 // should be included in the result. Mostly useful for filtering keep-sorted
 // blocks to just the ones that were modified by the currently CL.
-func (f *Fixer) newBlocks(lines []string, offset int, include func(start, end int) bool) ([]block, []incompleteBlock) {
+func (f *Fixer) newBlocks(filename string, lines []string, offset int, include func(start, end int) bool) (_ []block, _ []incompleteBlock, warnings []*Finding) {
 	var blocks []block
 	var incompleteBlocks []incompleteBlock
 
@@ -110,10 +109,9 @@ func (f *Fixer) newBlocks(lines []string, offset int, include func(start, end in
 			}
 
 			commentMarker, options, _ := strings.Cut(start.line, f.startDirective)
-			opts, err := parseBlockOptions(commentMarker, options, f.defaultOptions)
-			if err != nil {
-				// TODO(b/250608236): Is there a better way to surface this error?
-				log.Err(fmt.Errorf("keep-sorted block at index %d had bad start directive: %w", start.index+offset, err)).Msg("")
+			opts, optionWarnings := parseBlockOptions(commentMarker, options, f.defaultOptions)
+			for _, warn := range optionWarnings {
+				warnings = append(warnings, finding(filename, start.index+offset, start.index+offset, warn.Error()))
 			}
 
 			start.index += opts.SkipLines
@@ -172,7 +170,7 @@ func (f *Fixer) newBlocks(lines []string, offset int, include func(start, end in
 		}
 	}
 
-	return blocks, incompleteBlocks
+	return blocks, incompleteBlocks, warnings
 }
 
 // sorted returns a slice which represents the correct sorting of b.lines.

keepsorted/keep_sorted.go

@@ -56,16 +56,21 @@ func (f *Fixer) errorMissingEnd() string {
 }
 
 // Fix all of the findings on contents to make keep-sorted happy.
-func (f *Fixer) Fix(contents string, modifiedLines []LineRange) (fixed string, alreadyCorrect bool) {
+func (f *Fixer) Fix(filename, contents string, modifiedLines []LineRange) (fixed string, alreadyCorrect bool, warnings []*Finding) {
 	lines := strings.Split(contents, "\n")
-	fs := f.findings("unused-filename", lines, modifiedLines, false)
+	fs := f.findings(filename, lines, modifiedLines, false)
 	if len(fs) == 0 {
-		return contents, true
+		return contents, true, nil
 	}
 
 	var s strings.Builder
 	startLine := 1
 	for _, f := range fs {
+		if len(f.Fixes) == 0 {
+			warnings = append(warnings, f)
+			continue
+		}
+
 		repl := f.Fixes[0].Replacements[0]
 		endLine := repl.Lines.Start
 
@@ -77,7 +82,7 @@ func (f *Fixer) Fix(contents string, modifiedLines []LineRange) (fixed string, a
 	}
 	s.WriteString(strings.Join(lines[startLine-1:], "\n"))
 
-	return s.String(), false
+	return s.String(), false, warnings
 }
 
 // Findings returns a slice of things that need to be addressed in the file to
@@ -126,15 +131,16 @@ type Replacement struct {
 }
 
 func (f *Fixer) findings(filename string, contents []string, modifiedLines []LineRange, considerLintOption bool) []*Finding {
-	blocks, incompleteBlocks := f.newBlocks(contents, 1, includeModifiedLines(modifiedLines))
+	blocks, incompleteBlocks, warns := f.newBlocks(filename, contents, 1, includeModifiedLines(modifiedLines))
 
 	var fs []*Finding
+	fs = append(fs, warns...)
 	for _, b := range blocks {
 		if considerLintOption && !b.metadata.opts.Lint {
 			continue
 		}
 		if s, alreadySorted := b.sorted(); !alreadySorted {
-			fs = append(fs, finding(filename, b.start+1, b.end-1, errorUnordered, linesToString(s)))
+			fs = append(fs, finding(filename, b.start+1, b.end-1, errorUnordered, replacement(b.start+1, b.end-1, linesToString(s))))
 		}
 	}
 	for _, ib := range incompleteBlocks {
@@ -147,7 +153,7 @@ func (f *Fixer) findings(filename string, contents []string, modifiedLines []Lin
 		default:
 			panic(fmt.Errorf("unknown directive type: %v", ib.dir))
 		}
-		fs = append(fs, finding(filename, ib.line, ib.line, msg, ""))
+		fs = append(fs, finding(filename, ib.line, ib.line, msg, replacement(ib.line, ib.line, "")))
 	}
 
 	slices.SortFunc(fs, func(a, b *Finding) int {
@@ -178,30 +184,35 @@ func linesToString(lines []string) string {
 	return strings.Join(lines, "\n") + "\n"
 }
 
-func finding(filename string, start, end int, msg, replace string) *Finding {
-	lr := LineRange{
-		Start: start,
-		End:   end,
-	}
+func finding(filename string, start, end int, msg string, fixes ...Fix) *Finding {
 	return &Finding{
 		Path:    filename,
-		Lines:   lr,
+		Lines:   lineRange(start, end),
 		Message: msg,
-		Fixes: []Fix{
+		Fixes:   fixes,
+	}
+}
+
+func replacement(start, end int, s string) Fix {
+	return Fix{
+		Replacements: []Replacement{
 			{
-				Replacements: []Replacement{
-					{
-						Lines:      lr,
-						NewContent: replace,
-					},
-				},
+				Lines:      lineRange(start, end),
+				NewContent: s,
 			},
 		},
 	}
 }
 
+func lineRange(start, end int) LineRange {
+	return LineRange{
+		Start: start,
+		End:   end,
+	}
+}
+
 func startLine(f *Finding) int {
-	return f.Fixes[0].Replacements[0].Lines.Start
+	return f.Lines.Start
 }
 
 var _ augmentedtree.Interval = LineRange{}