[engine] Disallow *args and **kwargs in check impls
I can't think of a good use case for allowing check functions to have
arbitrary **kwargs and especially *args, so disallow them for now,
especially since it's not clear how they should interact with the
`check.with_args()` function.
Change-Id: I2ee786e325ec72f4788ae687ab712431194c274d
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/920972
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
Commit-Queue: Oliver Newman <olivernewman@google.com>
Reviewed-by: Anthony Fandrianto <atyfto@google.com>
Reviewed-by: Marc-Antoine Ruel <maruel@google.com>
diff --git a/internal/engine/run_test.go b/internal/engine/run_test.go
index 32b1ca1..5c09186 100644
--- a/internal/engine/run_test.go
+++ b/internal/engine/run_test.go
@@ -1549,6 +1549,16 @@
" //load-recurse.star:15:1: in <toplevel>\n",
},
{
+ "shac-check-star_args.star",
+ "shac.check: \"impl\" must not accept *args",
+ " //shac-check-star_args.star:18:31: in <toplevel>\n",
+ },
+ {
+ "shac-check-star_kwargs.star",
+ "shac.check: \"impl\" must not accept **kwargs",
+ " //shac-check-star_kwargs.star:18:31: in <toplevel>\n",
+ },
+ {
"shac-check-with_args-ctx.star",
"with_args: \"ctx\" argument cannot be overridden",
" //shac-check-with_args-ctx.star:18:45: in <toplevel>\n",
@@ -1605,7 +1615,7 @@
},
{
"shac-register_check-multiple_required_args.star",
- "shac.register_check: \"impl\" can only have one required argument",
+ "shac.register_check: \"impl\" cannot have required arguments besides \"ctx\"",
" //shac-register_check-multiple_required_args.star:18:20: in <toplevel>\n",
},
{
diff --git a/internal/engine/runtime_shac_check.go b/internal/engine/runtime_shac_check.go
index e6ec24f..cf2cb5c 100644
--- a/internal/engine/runtime_shac_check.go
+++ b/internal/engine/runtime_shac_check.go
@@ -55,11 +55,27 @@
if fun.ParamDefault(0) != nil {
return nil, errors.New("\"impl\" must not have a default value for the \"ctx\" parameter")
}
- for i := 1; i < fun.NumParams(); i++ {
- if fun.ParamDefault(i) == nil {
- return nil, errors.New("\"impl\" can only have one required argument")
- }
+
+ // Checks should not accept arbitrary positional or keyword arguments. This
+ // restriction can be reconsidered if there turns out to be a valid use
+ // case.
+ if fun.HasVarargs() {
+ return nil, errors.New("\"impl\" must not accept *args")
}
+ if fun.HasKwargs() {
+ return nil, errors.New("\"impl\" must not accept **kwargs")
+ }
+
+ // Check impl functions are called internally by shac without any arguments
+ // by default, so they will fail if they have any required arguments.
+ //
+ // It's only necessary to check the first parameter after "ctx" because it's
+ // illegal for any required parameters to come after optional ones, so if
+ // the first parameter is optional then the rest are as well.
+ if fun.NumParams() > 1 && fun.ParamDefault(1) == nil {
+ return nil, errors.New("\"impl\" cannot have required arguments besides \"ctx\"")
+ }
+
if name == "" {
if fun.Name() == "lambda" {
return nil, errors.New("\"name\" must be set when \"impl\" is a lambda")
diff --git a/internal/engine/testdata/fail_or_throw/shac-check-star_args.star b/internal/engine/testdata/fail_or_throw/shac-check-star_args.star
new file mode 100644
index 0000000..6d05756
--- /dev/null
+++ b/internal/engine/testdata/fail_or_throw/shac-check-star_args.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, *args):
+ pass
+
+shac.register_check(shac.check(cb))
diff --git a/internal/engine/testdata/fail_or_throw/shac-check-star_kwargs.star b/internal/engine/testdata/fail_or_throw/shac-check-star_kwargs.star
new file mode 100644
index 0000000..855479e
--- /dev/null
+++ b/internal/engine/testdata/fail_or_throw/shac-check-star_kwargs.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, **kwargs):
+ pass
+
+shac.register_check(shac.check(cb))
diff --git a/internal/engine/version.go b/internal/engine/version.go
index 24ed15a..9433863 100644
--- a/internal/engine/version.go
+++ b/internal/engine/version.go
@@ -18,5 +18,5 @@
// Version is the current tool version.
//
// TODO(maruel): Add proper version, preferably from git tag.
- Version = [...]int{0, 1, 6}
+ Version = [...]int{0, 1, 7}
)
diff --git a/shac.textproto b/shac.textproto
index bc4b74c..eec752b 100644
--- a/shac.textproto
+++ b/shac.textproto
@@ -15,7 +15,7 @@
# TODO(olivernewman): Build github.com/protocolbuffers/txtpbfmt into shac and
# enforce formatting of shac.textproto files in all repos that use shac.
-min_shac_version: "0.1.6"
+min_shac_version: "0.1.7"
allow_network: False
ignore: "/vendor/"
# Vendored code for test data only.
