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 @@ 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{ 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 @@ 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{ "", "the contents", "", }, }, { - "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 @@ func TestFix(t *testing.T) { }, }, { - "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 @@ 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" @@ -42,6 +43,7 @@ import ( "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 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 @@ 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 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 @@ -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", 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)