[engine] Apply fixes for checks of all levels

Previously `shac fix` and `shac fmt` would only apply fixes for findings
with a level or "error", which meant there was no automatic way to apply
fixes for non-error findings.

I considered making it configurable whether or not non-error findings'
replacements are applied (e.g. via a `--level` flag that specifies the
minimum level of findings to automatically fix) but I couldn't think of
a nice interface, so for now it's simplest to not filter findings based
on level.

Change-Id: I12e49839163a0e0ae4f86b52cdc28fb7584e1bae
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/912740
Fuchsia-Auto-Submit: Oliver Newman <[email protected]>
Reviewed-by: Anthony Fandrianto <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>

internal/engine/fix.go

@@ -30,7 +30,10 @@ import (
 // Fix loads a main shac.star file from a root directory and runs checks defined
 // in it, then applies suggested fixes to files on disk.
 func Fix(ctx context.Context, o *Options, quiet bool) error {
-	fc := findingCollector{countsByCheck: map[string]int{}, quiet: quiet}
+	fc := findingCollector{
+		countsByCheck: map[string]int{},
+		quiet:         quiet,
+	}
 	if o.Report != nil {
 		return fmt.Errorf("cannot overwrite reporter")
 	}
@@ -176,11 +179,13 @@ type findingCollector struct {
 var _ Report = (*findingCollector)(nil)
 
 func (c *findingCollector) EmitFinding(ctx context.Context, check string, level Level, message, root, file string, s Span, replacements []string) error {
-	// Only findings with "error" level and a single replacement will be
-	// automatically fixed. Non-error findings may not be necessary to fix, and
-	// findings with more than one replacement do not have a single fix that can
+	// Only findings with a single replacement will be automatically fixed.
+	// Findings with more than one replacement do not have a single fix that can
 	// be automatically chosen.
-	if level == Error && len(replacements) == 1 {
+	// TODO(olivernewman): Add level-based filtering; it should be possible to
+	// only apply fixes for findings with level=error, as others are not
+	// necessary to fix for `shac check` to pass.
+	if len(replacements) == 1 {
 		c.mu.Lock()
 		defer c.mu.Unlock()
 		c.findings = append(c.findings, findingToFix{

internal/engine/fix_test.go

@@ -35,60 +35,70 @@ func TestFix(t *testing.T) {
 
 	// TODO(olivernewman): Checks that emit findings in different files.
 	data := []struct {
-		name string
-		want []string
+		name  string
+		want  []string
+		level Level
 	}{
 		{
-			"delete_lines.star",
-			[]string{
+			name: "delete_lines.star",
+			want: []string{
 				"These are",
 				"that may be modified",
 			},
 		},
 		{
-			"ignored_findings.star",
-			originalLines,
+			name: "ignored_findings.star",
+			want: originalLines,
 		},
 		{
-			"multiple_replacements_one_file.star",
-			[]string{
+			name: "multiple_replacements_one_file.star",
+			want: []string{
 				"<REPL1>",
 				"the contents",
 				"<REPL2>",
 			},
 		},
 		{
-			"replace_entire_file.star",
-			[]string{
+			name: "replace_entire_file.star",
+			want: []string{
 				"this text is a replacement",
 				"for the entire file",
 			},
 		},
 		{
-			"replace_entire_file_others_ignored.star",
-			[]string{
+			name: "replace_entire_file_others_ignored.star",
+			want: []string{
 				"this text is a replacement",
 				"for the entire file",
 			},
 		},
 		{
-			"replace_one_full_line.star",
-			[]string{
+			name: "replace_one_full_line.star",
+			want: []string{
 				"These are",
 				"the contents",
 				"UPDATED",
 				"that may be modified",
 			},
 		},
 		{
-			"replace_partial_line.star",
-			[]string{
+			name: "replace_partial_line.star",
+			want: []string{
 				"These are",
 				"the contents",
 				"of UPDATED file",
 				"that may be modified",
 			},
 		},
+		{
+			name: "various_level_findings.star",
+			want: []string{
+				"NOTICE",
+				"WARNING",
+				"ERROR",
+				"that may be modified",
+			},
+		},
 	}
 	want := make([]string, len(data))
 	for i := range data {

internal/engine/run.go

@@ -35,13 +35,15 @@ import (
 	"time"
 
 	"github.com/go-git/go-git/plumbing/format/gitignore"
+	flag "github.com/spf13/pflag"
 	"go.fuchsia.dev/shac-project/shac/internal/sandbox"
 	"go.starlark.net/resolve"
 	"go.starlark.net/starlark"
 	"golang.org/x/sync/errgroup"
 	"google.golang.org/protobuf/encoding/prototext"
 )
 
+// TODO(olivernewman): Foo
 func init() {
 	// Enable not-yet-standard Starlark features.
 	resolve.AllowRecursion = true
@@ -95,6 +97,8 @@ func OnlyNonFormatters(c registeredCheck) bool {
 // level "error".
 type Level string
 
+var _ flag.Value = (*Level)(nil)
+
 // Valid Level values.
 const (
 	Notice  Level = "notice"
@@ -103,6 +107,35 @@ const (
 	Nothing Level = ""
 )
 
+func (l *Level) Set(value string) error {
+	*l = Level(value)
+	if !l.isValid() {
+		return fmt.Errorf("invalid level value %q", l)
+	}
+	return nil
+}
+
+func (l *Level) String() string {
+	return string(*l)
+}
+
+func (l *Level) Type() string {
+	return "level"
+}
+
+// Meets returns whether `l` is semantically more severe than, or as severe as,
+// `other`.
+func (l Level) Meets(other Level) bool {
+	// Any Level object that gets passed from user code should have already been
+	// validated at this point, so no need to check `isValid`.
+	ordered := []Level{
+		Notice,
+		Warning,
+		Error,
+	}
+	return slices.Index(ordered, l) >= slices.Index(ordered, other)
+}
+
 func (l Level) isValid() bool {
 	switch l {
 	case Notice, Warning, Error:
@@ -233,7 +266,7 @@ func Run(ctx context.Context, o *Options) error {
 	if err != nil {
 		return err
 	}
-	err = runInner(ctx, root, tmpdir, main, o.Report, doc.AllowNetwork, doc.WritableRoot, o.Recurse, o.Filter, scm, packages)
+	err = runInner(ctx, root, tmpdir, main, doc.AllowNetwork, doc.WritableRoot, o, scm, packages)
 	if err2 := os.RemoveAll(tmpdir); err == nil {
 		err = err2
 	}
@@ -295,7 +328,7 @@ func normalizeFiles(files []string, root string) ([]file, error) {
 	return res, nil
 }
 
-func runInner(ctx context.Context, root, tmpdir, main string, r Report, allowNetwork, writableRoot, recurse bool, filter CheckFilter, scm scmCheckout, packages map[string]fs.FS) error {
+func runInner(ctx context.Context, root, tmpdir, main string, allowNetwork, writableRoot bool, o *Options, scm scmCheckout, packages map[string]fs.FS) error {
 	sb, err := sandbox.New(tmpdir)
 	if err != nil {
 		return err
@@ -318,9 +351,9 @@ func runInner(ctx context.Context, root, tmpdir, main string, r Report, allowNet
 		return &shacState{
 			allowNetwork: allowNetwork,
 			env:          &env,
-			filter:       filter,
+			filter:       o.Filter,
 			main:         main,
-			r:            r,
+			r:            o.Report,
 			root:         root,
 			sandbox:      sb,
 			scm:          scm,
@@ -330,7 +363,7 @@ func runInner(ctx context.Context, root, tmpdir, main string, r Report, allowNet
 		}
 	}
 	var shacStates []*shacState
-	if recurse {
+	if o.Recurse {
 		// Each found shac.star is run in its own interpreter for maximum
 		// parallelism.
 		// Discover all the main files via the SCM. This enables us to not walk

internal/engine/runtime_ctx_emit.go

@@ -116,7 +116,7 @@ func ctxEmitFinding(ctx context.Context, s *shacState, name string, args starlar
 	c := ctxCheck(ctx)
 	message := string(argmessage)
 	if len(message) == 0 {
-		if c.formatter && file != "" && level == Error && len(replacements) == 1 {
+		if c.formatter && file != "" && len(replacements) == 1 {
 			// If the check is a formatter, and the finding would be fixed by
 			// `shac fmt`, let users omit `message` as long as `file` is
 			// specified, since `message` will always look something like the

internal/engine/testdata/fix/ignored_findings.star

@@ -13,11 +13,6 @@
 # limitations under the License.
 
 def cb(ctx):
-  ctx.emit.finding(
-      level="warning",  # Warnings do not trigger fixes.
-      filepath="file.txt",
-      message="Just a warning",
-      replacements=["IGNORED"])
   ctx.emit.finding(
       level="error",
       filepath="file.txt",

internal/engine/testdata/fix/various_level_findings.star

@@ -0,0 +1,35 @@
+# Copyright 2023 The Shac Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+def cb(ctx):
+  ctx.emit.finding(
+      level="notice",
+      filepath="file.txt",
+      message="Just a notice",
+      line=1,
+      replacements=["NOTICE\n"])
+  ctx.emit.finding(
+      level="warning",
+      filepath="file.txt",
+      message="A warning",
+      line=2,
+      replacements=["WARNING\n"])
+  ctx.emit.finding(
+      level="error",
+      filepath="file.txt",
+      message="An error!!!",
+      line=3,
+      replacements=["ERROR\n"])
+
+shac.register_check(cb)