ParameterSpec::new_parts

Summary: Safer constructors: `new_parts` and `new_named_only`.

Reviewed By: JakobDegen

Differential Revision: D63165431

fbshipit-source-id: d06329779d32472cefdf9fd40bf6fc58db59354e

starlark/src/__derive_refs/sig.rs

@@ -16,6 +16,7 @@
  */
 
 use crate::eval::ParametersSpec;
+use crate::eval::ParametersSpecParam;
 use crate::values::FrozenValue;
 
 pub enum NativeSigArg {
@@ -24,6 +25,16 @@ pub enum NativeSigArg {
     Defaulted(&'static str, FrozenValue),
 }
 
+impl NativeSigArg {
+    fn param(&self) -> (&str, ParametersSpecParam<FrozenValue>) {
+        match self {
+            NativeSigArg::Required(name) => (name, ParametersSpecParam::Required),
+            NativeSigArg::Optional(name) => (name, ParametersSpecParam::Optional),
+            NativeSigArg::Defaulted(name, value) => (name, ParametersSpecParam::Defaulted(*value)),
+        }
+    }
+}
+
 pub fn parameter_spec(
     name: &'static str,
     pos_only: &[NativeSigArg],
@@ -32,42 +43,12 @@ pub fn parameter_spec(
     named_only: &[NativeSigArg],
     kwargs: bool,
 ) -> ParametersSpec<FrozenValue> {
-    let mut spec = ParametersSpec::new(name.to_owned());
-
-    for pos_only in pos_only {
-        match pos_only {
-            NativeSigArg::Required(name) => spec.required(name),
-            NativeSigArg::Optional(name) => spec.optional(name),
-            NativeSigArg::Defaulted(name, value) => spec.defaulted(name, *value),
-        }
-    }
-    spec.no_more_positional_only_args();
-
-    for pos_or_named in pos_or_named {
-        match pos_or_named {
-            NativeSigArg::Required(name) => spec.required(name),
-            NativeSigArg::Optional(name) => spec.optional(name),
-            NativeSigArg::Defaulted(name, value) => spec.defaulted(name, *value),
-        }
-    }
-
-    if args {
-        spec.args();
-    } else {
-        spec.no_more_positional_args();
-    }
-
-    for named_only in named_only {
-        match named_only {
-            NativeSigArg::Required(name) => spec.required(name),
-            NativeSigArg::Optional(name) => spec.optional(name),
-            NativeSigArg::Defaulted(name, value) => spec.defaulted(name, *value),
-        }
-    }
-
-    if kwargs {
-        spec.kwargs();
-    }
-
-    spec.finish()
+    ParametersSpec::new_parts(
+        name,
+        pos_only.iter().map(NativeSigArg::param),
+        pos_or_named.iter().map(NativeSigArg::param),
+        args,
+        named_only.iter().map(NativeSigArg::param),
+        kwargs,
+    )
 }

starlark/src/eval.rs

@@ -37,6 +37,7 @@ pub use runtime::file_loader::ReturnFileLoader;
 pub use runtime::params::parser::ParametersParser;
 pub use runtime::params::spec::ParametersSpec;
 pub use runtime::params::spec::ParametersSpecBuilder;
+pub use runtime::params::spec::ParametersSpecParam;
 pub use runtime::profile::data::ProfileData;
 pub use runtime::profile::mode::ProfileMode;
 pub use soft_error::SoftErrorHandler;

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

@@ -79,17 +79,24 @@ mod tests {
     use crate::eval::compiler::def::FrozenDef;
     use crate::eval::runtime::params::display::PARAM_FMT_OPTIONAL;
     use crate::eval::runtime::params::spec::ParametersSpec;
+    use crate::eval::ParametersSpecParam;
     use crate::typing::Ty;
     use crate::values::FrozenValue;
 
     #[test]
     fn test_documentation() -> anyhow::Result<()> {
         // Make sure that documentation for some odder parameter specs works properly.
-        let mut p = ParametersSpec::<FrozenValue>::new("f".to_owned());
-        p.args();
-        p.optional("a");
-        p.optional("b");
-        let p = p.finish();
+        let p = ParametersSpec::<FrozenValue>::new_parts(
+            "f",
+            [],
+            [],
+            true,
+            [
+                ("a", ParametersSpecParam::Optional),
+                ("b", ParametersSpecParam::Optional),
+            ],
+            false,
+        );
 
         let expected = DocParams {
             args: Some(DocParam {

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

@@ -55,6 +55,19 @@ use crate::values::StringValue;
 use crate::values::Value;
 use crate::values::ValueLike;
 
+/// Describe parameter for [`ParametersSpec`].
+#[derive(
+    Debug, Clone, Dupe, PartialEq, Eq, PartialOrd, Ord, Trace, Freeze, Allocative
+)]
+pub enum ParametersSpecParam<V> {
+    /// Parameter is required.
+    Required,
+    /// Parameter is optional (returned as `None`).
+    Optional,
+    /// Parameter has default value.
+    Defaulted(V),
+}
+
 #[derive(Debug, Copy, Clone, Dupe, Coerce, PartialEq, Trace, Freeze, Allocative)]
 #[repr(C)]
 pub(crate) enum ParameterKind<V> {
@@ -178,6 +191,14 @@ impl<V: Copy> ParametersSpecBuilder<V> {
         self.add(name, ParameterKind::Defaulted(val));
     }
 
+    fn param(&mut self, name: &str, param: ParametersSpecParam<V>) {
+        match param {
+            ParametersSpecParam::Required => self.required(name),
+            ParametersSpecParam::Optional => self.optional(name),
+            ParametersSpecParam::Defaulted(x) => self.defaulted(name, x),
+        }
+    }
+
     /// Add an `*args` parameter which will be an iterable sequence of parameters,
     /// recorded into a [`Vec`]. A function can only have one `args`
     /// parameter. After this call, any subsequent
@@ -312,6 +333,70 @@ impl<V> ParametersSpec<V> {
         }
     }
 
+    /// Create a new [`ParametersSpec`].
+    pub fn new_parts<'a>(
+        function_name: &str,
+        pos_only: impl IntoIterator<Item = (&'a str, ParametersSpecParam<V>)>,
+        pos_or_named: impl IntoIterator<Item = (&'a str, ParametersSpecParam<V>)>,
+        args: bool,
+        named_only: impl IntoIterator<Item = (&'a str, ParametersSpecParam<V>)>,
+        kwargs: bool,
+    ) -> ParametersSpec<V>
+    where
+        V: Copy,
+    {
+        let pos_only = pos_only.into_iter();
+        let pos_or_named = pos_or_named.into_iter();
+        let named_only = named_only.into_iter();
+
+        let mut builder = ParametersSpec::with_capacity(
+            function_name.to_owned(),
+            pos_only.size_hint().0
+                + pos_or_named.size_hint().0
+                + args as usize
+                + named_only.size_hint().0
+                + kwargs as usize,
+        );
+
+        for (name, val) in pos_only {
+            builder.param(name, val);
+        }
+        builder.no_more_positional_only_args();
+        for (name, val) in pos_or_named {
+            builder.param(name, val);
+        }
+        if args {
+            builder.args();
+        } else {
+            builder.no_more_positional_args();
+        }
+        for (name, val) in named_only {
+            builder.param(name, val);
+        }
+        if kwargs {
+            builder.kwargs();
+        }
+        builder.finish()
+    }
+
+    /// Parameter parse with only named parameters.
+    pub fn new_named_only<'a>(
+        function_name: &str,
+        named_only: impl IntoIterator<Item = (&'a str, ParametersSpecParam<V>)>,
+    ) -> ParametersSpec<V>
+    where
+        V: Copy,
+    {
+        Self::new_parts(
+            function_name,
+            std::iter::empty(),
+            std::iter::empty(),
+            false,
+            named_only,
+            false,
+        )
+    }
+
     /// Produce an approximate signature for the function, combining the name and arguments.
     pub fn signature(&self) -> String {
         let mut collector = String::new();

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

@@ -42,6 +42,7 @@ use crate::environment::MethodsStatic;
 use crate::eval::Arguments;
 use crate::eval::Evaluator;
 use crate::eval::ParametersSpec;
+use crate::eval::ParametersSpecParam;
 use crate::starlark_complex_values;
 use crate::typing::callable::TyCallable;
 use crate::typing::starlark_value::TyStarlarkValue;
@@ -165,16 +166,18 @@ impl<'v> RecordType<'v> {
     fn make_parameter_spec(
         fields: &SmallMap<String, FieldGen<Value<'v>>>,
     ) -> ParametersSpec<FrozenValue> {
-        let mut parameters = ParametersSpec::with_capacity("record".to_owned(), fields.len());
-        parameters.no_more_positional_args();
-        for (name, field) in fields {
-            if field.default.is_some() {
-                parameters.optional(name);
-            } else {
-                parameters.required(name);
-            }
-        }
-        parameters.finish()
+        ParametersSpec::new_named_only(
+            "record",
+            fields.iter().map(|(name, field)| {
+                (
+                    name.as_str(),
+                    match field.default {
+                        None => ParametersSpecParam::Required,
+                        Some(_default) => ParametersSpecParam::Optional,
+                    },
+                )
+            }),
+        )
     }
 }