[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 <olivernewman@google.com>
Reviewed-by: Anthony Fandrianto <atyfto@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
diff --git a/internal/engine/fix.go b/internal/engine/fix.go
index e513c7c..425c929 100644
--- a/internal/engine/fix.go
+++ b/internal/engine/fix.go
@@ -30,7 +30,10 @@
// 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 @@
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{
diff --git a/internal/engine/fix_test.go b/internal/engine/fix_test.go
index 1efae69..169f8a2 100644
--- a/internal/engine/fix_test.go
+++ b/internal/engine/fix_test.go
@@ -35,45 +35,46 @@
// 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",
@@ -81,14 +82,23 @@
},
},
{
- "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 {
diff --git a/internal/engine/run.go b/internal/engine/run.go
index dbb6941..8665d39 100644
--- a/internal/engine/run.go
+++ b/internal/engine/run.go
@@ -35,6 +35,7 @@
"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"
@@ -42,6 +43,7 @@
"google.golang.org/protobuf/encoding/prototext"
)
+// TODO(olivernewman): Foo
func init() {
// Enable not-yet-standard Starlark features.
resolve.AllowRecursion = true
@@ -95,6 +97,8 @@
// level "error".
type Level string
+var _ flag.Value = (*Level)(nil)
+
// Valid Level values.
const (
Notice Level = "notice"
@@ -103,6 +107,35 @@
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 @@
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 @@
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 @@
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 @@
}
}
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
diff --git a/internal/engine/runtime_ctx_emit.go b/internal/engine/runtime_ctx_emit.go
index 3897490..5639a03 100644
--- a/internal/engine/runtime_ctx_emit.go
+++ b/internal/engine/runtime_ctx_emit.go
@@ -116,7 +116,7 @@
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
diff --git a/internal/engine/testdata/fix/ignored_findings.star b/internal/engine/testdata/fix/ignored_findings.star
index 950eef6..2ea8045 100644
--- a/internal/engine/testdata/fix/ignored_findings.star
+++ b/internal/engine/testdata/fix/ignored_findings.star
@@ -14,11 +14,6 @@
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",
message="Error, but multiple options",
diff --git a/internal/engine/testdata/fix/various_level_findings.star b/internal/engine/testdata/fix/various_level_findings.star
new file mode 100644
index 0000000..fda62e8
--- /dev/null
+++ b/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)
