[engine] Make `filepath` a requirement for `replacements` or `line`
It doesn't make sense to specify a line range in a finding without
specifying a file. Likewise, it doesn't make sense to suggest
replacements without specifying a file in which to apply the
replacements.
Change-Id: Iabfe0e34e1af3797b573b49e079207d5f270340a
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/887574
Reviewed-by: Marc-Antoine Ruel <maruel@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
diff --git a/internal/engine/run_test.go b/internal/engine/run_test.go
index f1d9424..2717825 100644
--- a/internal/engine/run_test.go
+++ b/internal/engine/run_test.go
@@ -934,6 +934,11 @@
" //ctx-emit-finding-level.star:16:19: in cb\n",
},
{
+ "ctx-emit-finding-line-no_file.star",
+ "ctx.emit.finding: for parameter \"line\": \"filepath\" must be specified",
+ " //ctx-emit-finding-line-no_file.star:16:19: in cb\n",
+ },
+ {
"ctx-emit-finding-line.star",
"ctx.emit.finding: for parameter \"line\": got -1, line are 1 based",
" //ctx-emit-finding-line.star:16:19: in cb\n",
@@ -954,6 +959,11 @@
" //ctx-emit-finding-replacements-list.star:16:19: in cb\n",
},
{
+ "ctx-emit-finding-replacements-no_file.star",
+ "ctx.emit.finding: for parameter \"replacements\": \"filepath\" must be specified",
+ " //ctx-emit-finding-replacements-no_file.star:16:19: in cb\n",
+ },
+ {
"ctx-emit-finding-replacements-str.star",
"ctx.emit.finding: for parameter \"replacements\": got string, want starlark.Sequence",
" //ctx-emit-finding-replacements-str.star:16:19: in cb\n",
@@ -1450,11 +1460,15 @@
Check: "cb",
Level: Notice,
Message: "great code",
+ Root: root,
+ File: "file.txt",
Span: Span{Start: Cursor{Line: 100, Col: 2}},
}, {
Check: "cb",
Level: "notice",
Message: "nice",
+ Root: root,
+ File: "file.txt",
Span: Span{Start: Cursor{Line: 100, Col: 2}, End: Cursor{Line: 100, Col: 3}},
},
{
@@ -1470,6 +1484,8 @@
Check: "cb",
Level: "warning",
Message: "weird",
+ Root: root,
+ File: "file.txt",
Span: Span{Start: Cursor{Line: 1}, End: Cursor{Line: 10}},
Replacements: []string{"a", "dict"},
},
@@ -1477,6 +1493,8 @@
Check: "cb",
Level: "warning",
Message: "no span, full file",
+ Root: root,
+ File: "file.txt",
Replacements: []string{"this text is a replacement\nfor the entire file\n"},
},
},
diff --git a/internal/engine/runtime_ctx_emit.go b/internal/engine/runtime_ctx_emit.go
index dc3d89b..9aa44d2 100644
--- a/internal/engine/runtime_ctx_emit.go
+++ b/internal/engine/runtime_ctx_emit.go
@@ -57,8 +57,6 @@
if len(message) == 0 {
return fmt.Errorf("for parameter \"message\": got %s, want string", argmessage)
}
- // TODO(olivernewman): Require filepath to be set if span and/or
- // replacements are specified.
file := string(argfilepath)
span := Span{
Start: Cursor{
@@ -83,6 +81,9 @@
return errors.New("for parameter \"end_col\": \"col\" must be specified")
}
if span.Start.Line > 0 {
+ if file == "" {
+ return errors.New("for parameter \"line\": \"filepath\" must be specified")
+ }
if span.End.Line > 0 {
if span.End.Line < span.Start.Line {
return errors.New("for parameter \"end_line\": must be greater than or equal to \"line\"")
@@ -104,6 +105,9 @@
}
var replacements []string
if argreplacements != nil {
+ if file == "" {
+ return errors.New("for parameter \"replacements\": \"filepath\" must be specified")
+ }
if replacements = sequenceToStrings(argreplacements); replacements == nil {
return fmt.Errorf("for parameter \"replacements\": got %s, want sequence of str", argreplacements.Type())
}
diff --git a/internal/engine/testdata/emit/ctx-emit-finding.star b/internal/engine/testdata/emit/ctx-emit-finding.star
index f3c6bff..511bfe3 100644
--- a/internal/engine/testdata/emit/ctx-emit-finding.star
+++ b/internal/engine/testdata/emit/ctx-emit-finding.star
@@ -22,8 +22,19 @@
end_line=10,
end_col=1,
replacements=("a", "tuple"))
- ctx.emit.finding(level="notice", line=100, col=2, message="great code")
- ctx.emit.finding(level="notice", line=100, col=2, end_col=3, message="nice")
+ ctx.emit.finding(
+ level="notice",
+ filepath="file.txt",
+ line=100,
+ col=2,
+ message="great code")
+ ctx.emit.finding(
+ level="notice",
+ filepath="file.txt",
+ line=100,
+ col=2,
+ end_col=3,
+ message="nice")
ctx.emit.finding(
level="warning",
message="please fix",
@@ -36,12 +47,14 @@
ctx.emit.finding(
level="warning",
message="weird",
+ filepath="file.txt",
line=1,
end_line=10,
replacements={"a": True, "dict": 42})
ctx.emit.finding(
level="warning",
message="no span, full file",
+ filepath="file.txt",
replacements=["this text is a replacement\nfor the entire file\n"])
shac.register_check(cb)
diff --git a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_col-col-equal.star b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_col-col-equal.star
index 12edc46..c70ff29 100644
--- a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_col-col-equal.star
+++ b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_col-col-equal.star
@@ -16,6 +16,7 @@
ctx.emit.finding(
level="notice",
message="fix it",
+ filepath="foo.txt",
line=1,
col=2,
end_line=1,
diff --git a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_line-line-reverse.star b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_line-line-reverse.star
index fa06120..e2d2339 100644
--- a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_line-line-reverse.star
+++ b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-end_line-line-reverse.star
@@ -13,6 +13,11 @@
# limitations under the License.
def cb(ctx):
- ctx.emit.finding(level="notice", message="fix it", line=2, end_line=1)
+ ctx.emit.finding(
+ level="notice",
+ message="fix it",
+ filepath="foo.txt",
+ line=2,
+ end_line=1)
shac.register_check(cb)
diff --git a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-line-no_file.star b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-line-no_file.star
new file mode 100644
index 0000000..0947171
--- /dev/null
+++ b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-line-no_file.star
@@ -0,0 +1,18 @@
+# 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", message="fix it", line=1)
+
+shac.register_check(cb)
diff --git a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-limit.star b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-limit.star
index 1eb42c0..5abddf5 100644
--- a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-limit.star
+++ b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-limit.star
@@ -17,6 +17,7 @@
ctx.emit.finding(
level="notice",
message="fix it",
+ filepath="foo.txt",
replacements=too_many)
shac.register_check(cb)
diff --git a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-list.star b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-list.star
index 60e4777..83048c4 100644
--- a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-list.star
+++ b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-list.star
@@ -16,6 +16,7 @@
ctx.emit.finding(
level="notice",
message="fix it",
+ filepath="foo.txt",
replacements=["nothing", 42])
shac.register_check(cb)
diff --git a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-no_file.star b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-no_file.star
new file mode 100644
index 0000000..580cfbb
--- /dev/null
+++ b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-no_file.star
@@ -0,0 +1,21 @@
+# 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",
+ message="fix it",
+ replacements=["foo"])
+
+shac.register_check(cb)
diff --git a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-tuple.star b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-tuple.star
index 96c07a8..fbb89c7 100644
--- a/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-tuple.star
+++ b/internal/engine/testdata/fail_or_throw/ctx-emit-finding-replacements-tuple.star
@@ -16,6 +16,7 @@
ctx.emit.finding(
level="notice",
message="fix it",
+ filepath="foo.txt",
replacements=("nothing", 42))
shac.register_check(cb)
