Better error message on missing parameter

Summary: Marginally more helpful.

Reviewed By: IanChilds

Differential Revision: D63680370

fbshipit-source-id: 4346f078388efefe9cfb42d358594d3cde4bbcc9

starlark/src/eval/runtime/arguments.rs

@@ -45,8 +45,6 @@ use crate::values::ValueLike;
 
 #[derive(Debug, Clone, Error)]
 pub(crate) enum FunctionError {
-    #[error("Missing parameter `{name}` for call to {function}")]
-    MissingParameter { name: String, function: String },
     #[error("Found {count} extra positional argument(s) for call to {function}")]
     ExtraPositionalArg { count: usize, function: String },
     #[error("Found `{}` extra named parameter(s) for call to {function}", .names.join("` `"))]

starlark/src/eval/runtime/params/spec.rs

@@ -26,6 +26,7 @@ use starlark_derive::Freeze;
 use starlark_derive::Trace;
 use starlark_map::small_map::SmallMap;
 use starlark_map::Hashed;
+use starlark_syntax::function_error;
 use starlark_syntax::other_error;
 use starlark_syntax::syntax::def::DefParamIndices;
 
@@ -739,11 +740,21 @@ impl<'v> ParametersSpec<Value<'v>> {
             }
             match def {
                 ParameterKind::Required => {
-                    return Err(FunctionError::MissingParameter {
-                        name: self.param_names[index].clone(),
-                        function: self.signature(),
+                    let function_name = &self.function_name;
+                    let param_name = &self.param_names[index];
+                    if index < self.indices.num_positional_only as usize {
+                        return Err(function_error!(
+                            "Missing positional-only parameter `{param_name}` for call to `{function_name}`",
+                        ));
+                    } else if index >= self.indices.num_positional as usize {
+                        return Err(function_error!(
+                            "Missing named-only parameter `{param_name}` for call to `{function_name}`",
+                        ));
+                    } else {
+                        return Err(function_error!(
+                            "Missing parameter `{param_name}` for call to `{function_name}`"
+                        ));
                     }
-                    .into());
                 }
                 ParameterKind::Defaulted(x) => {
                     *slot = Some(x.to_value());

starlark/src/tests/call.rs

@@ -240,7 +240,7 @@ def f(x, *, y):
     pass
 noop(f)(1)
 "#,
-        "Missing parameter `y`",
+        "Missing named-only parameter `y`",
     );
 }
 
@@ -277,7 +277,7 @@ g = noop(f) # Hide from static type checker.
 g(x=1, y=2)
 "#,
         // TODO(nga): bad message.
-        "Missing parameter `x` for call",
+        "Missing positional-only parameter `x` for call",
     );
 }
 

starlark/src/tests/derive/module/named_positional.rs

@@ -73,15 +73,18 @@ fn test_named_only() {
     let mut a = Assert::new();
     a.globals_add(named_positional_functions);
     a.eq("31", "named_only(x=31)");
-    a.fail("noop(named_only)(37)", "Missing parameter");
+    a.fail("noop(named_only)(37)", "Missing named-only parameter");
 }
 
 #[test]
 fn test_named_after_args() {
     let mut a = Assert::new();
     a.globals_add(named_positional_functions);
     a.eq("13", "named_after_args(1, 2, x=10)");
-    a.fail("noop(named_after_args)(1, 2, 3)", "Missing parameter");
+    a.fail(
+        "noop(named_after_args)(1, 2, 3)",
+        "Missing named-only parameter",
+    );
 }
 
 #[test]
@@ -91,6 +94,6 @@ fn test_named_after_args_explicitly_marked() {
     a.eq("13", "named_after_args_explicitly_marked(1, 2, x=10)");
     a.fail(
         "noop(named_after_args_explicitly_marked)(1, 2, 3)",
-        "Missing parameter",
+        "Missing named-only parameter",
     );
 }

starlark/src/values/types/record/globals.rs

@@ -139,7 +139,7 @@ rec_type(host=1, port=80)
 rec_type = record(host=str, port=int)
 rec_type(port=80)
 "#,
-            &["Missing parameter", "`host`"],
+            &["Missing named-only parameter", "`host`"],
         );
     }
 

starlark_syntax/src/error.rs

@@ -295,6 +295,12 @@ pub fn value_error_impl(args: fmt::Arguments<'_>) -> Error {
     Error::new_kind(ErrorKind::Value(anyhow::anyhow!("{}", args)))
 }
 
+#[doc(hidden)]
+#[cold]
+pub fn function_error_impl(args: fmt::Arguments<'_>) -> Error {
+    Error::new_kind(ErrorKind::Function(anyhow::anyhow!("{}", args)))
+}
+
 /// Internal error of starlark.
 #[macro_export]
 macro_rules! internal_error {
@@ -325,3 +331,13 @@ macro_rules! value_error {
         $crate::error::value_error_impl(format_args!($format, $($args)*))
     };
 }
+
+#[macro_export]
+macro_rules! function_error {
+    ($format:literal) => {
+        function_error!($format,)
+    };
+    ($format:literal, $($args:tt)*) => {
+        $crate::error::function_error_impl(format_args!($format, $($args)*))
+    };
+}