Use StringValue in Arguments

Summary:
Parameter names in `Arguments` were `Value`. They are `StringValue` now.

Use types to assert values are of correct type.

Mostly for documentation.

Reviewed By: ndmitchell

Differential Revision: D30884359

fbshipit-source-id: 64a9d3e54817e400d52b027dc3f585c270ae99a7

Author: stepancheg

starlark/src/eval/fragment/call.rs

@@ -30,7 +30,10 @@ use crate::{
         Arguments, Evaluator, FrozenDef,
     },
     syntax::ast::{ArgumentP, ExprP},
-    values::{function::NativeFunction, AttrType, FrozenValue, StarlarkValue, Value, ValueLike},
+    values::{
+        function::NativeFunction, AttrType, FrozenStringValue, FrozenValue, StarlarkValue, Value,
+        ValueLike,
+    },
 };
 use gazebo::coerce::coerce_ref;
 use std::mem::MaybeUninit;
@@ -42,7 +45,7 @@ struct ArgsCompiled {
     ///
     /// Note names are guaranteed to be unique here because names are validated in AST:
     /// named arguments in [`Expr::Call`] are unique.
-    names: Vec<(Symbol, FrozenValue)>,
+    names: Vec<(Symbol, FrozenStringValue)>,
     args: Option<ExprCompiled>,
     kwargs: Option<ExprCompiled>,
 }
@@ -200,7 +203,10 @@ impl Compiler<'_> {
             match x.node {
                 ArgumentP::Positional(x) => res.pos_named.push(self.expr(x).as_compiled()),
                 ArgumentP::Named(name, value) => {
-                    let fv = self.module_env.frozen_heap().alloc(name.node.as_str());
+                    let fv = self
+                        .module_env
+                        .frozen_heap()
+                        .alloc_string_value(name.node.as_str());
                     res.names.push((Symbol::new(&name.node), fv));
                     res.pos_named.push(self.expr(value).as_compiled());
                 }

starlark/src/eval/mod.rs

@@ -171,7 +171,7 @@ impl<'v, 'a> Evaluator<'v, 'a> {
         positional: &[Value<'v>],
         named: &[(&str, Value<'v>)],
     ) -> anyhow::Result<Value<'v>> {
-        let names = named.map(|(s, _)| (Symbol::new(*s), self.heap().alloc(*s)));
+        let names = named.map(|(s, _)| (Symbol::new(*s), self.heap().alloc_string_value(*s)));
         let named = named.map(|x| x.1);
         let params = Arguments {
             this: None,

starlark/src/eval/runtime/arguments.rs

@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-use crate::collections::BorrowHashed;
+use crate::{collections::BorrowHashed, values::StringValue};
 /// Deal with all aspects of runtime parameter evaluation.
 /// First build a Parameters structure, then use collect to collect the
 /// parameters into slots.
@@ -425,7 +425,7 @@ impl<'v, V: ValueLike<'v>> ParametersSpec<V> {
                     None => {
                         add_kwargs(
                             &mut kwargs,
-                            Hashed::new_unchecked(name.small_hash(), *name_value),
+                            Hashed::new_unchecked(name.small_hash(), name_value.to_value()),
                             *v,
                         );
                     }
@@ -633,7 +633,7 @@ pub struct Arguments<'v, 'a> {
     /// Names of named arguments.
     ///
     /// `named` length must be equal to `names` length.
-    pub names: &'a [(Symbol, Value<'v>)],
+    pub names: &'a [(Symbol, StringValue<'v>)],
     /// `*args` argument.
     pub args: Option<Value<'v>>,
     /// `**kwargs` argument.
@@ -649,7 +649,8 @@ impl<'v, 'a> Arguments<'v, 'a> {
             None => {
                 let mut result = SmallMap::with_capacity(self.names.len());
                 for (k, v) in self.names.iter().zip(self.named) {
-                    result.insert_hashed(Hashed::new_unchecked(k.0.small_hash(), k.1), *v);
+                    result
+                        .insert_hashed(Hashed::new_unchecked(k.0.small_hash(), k.1.to_value()), *v);
                 }
                 Ok(Dict::new(result))
             }
@@ -664,7 +665,10 @@ impl<'v, 'a> Arguments<'v, 'a> {
                     let mut result =
                         SmallMap::with_capacity(self.names.len() + kwargs.content.len());
                     for (k, v) in self.names.iter().zip(self.named) {
-                        result.insert_hashed(Hashed::new_unchecked(k.0.small_hash(), k.1), *v);
+                        result.insert_hashed(
+                            Hashed::new_unchecked(k.0.small_hash(), k.1.to_value()),
+                            *v,
+                        );
                     }
                     for (k, v) in kwargs.iter_hashed() {
                         let s = Arguments::unpack_kwargs_key(*k.key())?;
@@ -985,7 +989,7 @@ mod test {
         p.kwargs = None;
         let named = [Value::new_none()];
         p.named = &named;
-        let names = [(Symbol::new("test"), heap.alloc("test"))];
+        let names = [(Symbol::new("test"), heap.alloc_string_value("test"))];
         p.names = &names;
         assert!(p.no_named_args().is_err());
     }

starlark/src/stdlib/extra.rs

@@ -23,7 +23,8 @@ use crate::{
     eval::{Arguments, Evaluator, ParametersSpec},
     values::{
         dict::Dict, function::FUNCTION_TYPE, list::List, none::NoneType, tuple::Tuple,
-        ComplexValue, Freezer, StarlarkValue, Trace, Value, ValueLike,
+        ComplexValue, Freezer, FrozenStringValue, FrozenValue, StarlarkValue, StringValue,
+        StringValueLike, Trace, Value, ValueLike,
     },
 };
 use gazebo::{
@@ -77,11 +78,12 @@ pub fn partial(builder: &mut GlobalsBuilder) {
             .content
             .keys()
             .map(|x| {
+                let x = StringValue::new(*x).unwrap();
                 (
                     // We duplicate string here.
                     // If this becomes hot, we should do better.
-                    Symbol::new_hashed(x.unpack_starlark_str().unwrap().as_str_hashed()),
-                    *x,
+                    Symbol::new_hashed(x.unpack_starlark_str().as_str_hashed()),
+                    x,
                 )
             })
             .collect();
@@ -147,15 +149,17 @@ pub fn abs(builder: &mut GlobalsBuilder) {
 
 #[derive(Debug, Coerce, Trace)]
 #[repr(C)]
-struct PartialGen<V> {
+struct PartialGen<V, S> {
     func: V,
     pos: Vec<V>,
     named: Vec<V>,
-    names: Vec<(Symbol, V)>,
+    names: Vec<(Symbol, S)>,
     signature: ParametersSpec<V>,
 }
 
-starlark_complex_value!(Partial);
+type Partial<'v> = PartialGen<Value<'v>, StringValue<'v>>;
+type FrozenPartial = PartialGen<FrozenValue, FrozenStringValue>;
+starlark_complex_values!(Partial);
 
 impl<'v> ComplexValue<'v> for Partial<'v> {
     type Frozen = FrozenPartial;
@@ -172,7 +176,7 @@ impl<'v> ComplexValue<'v> for Partial<'v> {
     }
 }
 
-impl<'v, V: ValueLike<'v>> StarlarkValue<'v> for PartialGen<V>
+impl<'v, V: ValueLike<'v>, S: StringValueLike<'v>> StarlarkValue<'v> for PartialGen<V, S>
 where
     Self: AnyLifetime<'v>,
 {

starlark/src/values/alloc_value.rs

@@ -70,7 +70,6 @@ impl FrozenHeap {
         val.alloc_frozen_value(self)
     }
 
-    #[allow(dead_code)] // TODO: remove in the following diff
     pub(crate) fn alloc_string_value(&self, s: &str) -> FrozenStringValue {
         unsafe { FrozenStringValue::new_unchecked(self.alloc_str(s)) }
     }
@@ -82,7 +81,6 @@ impl Heap {
         x.alloc_value(self)
     }
 
-    #[allow(dead_code)] // TODO: remove in the following diff
     pub(crate) fn alloc_string_value<'v>(&'v self, s: &str) -> StringValue<'v> {
         unsafe { StringValue::new_unchecked(self.alloc_str(s)) }
     }

starlark/src/values/layout/constant.rs

@@ -115,7 +115,6 @@ impl FrozenStringValue {
     }
 }
 
-#[allow(dead_code)] // TODO: remove in the following diff
 impl<'v> StringValue<'v> {
     /// Construct without a check that the value contains a string.
     ///

starlark/src/values/layout/mod.rs

@@ -27,7 +27,6 @@ mod pointer_i32;
 mod value;
 mod value_captured;
 
-#[allow(unused_imports)] // TODO: remove in the following diff
 pub(crate) use constant::StringValueLike;
 pub use constant::{ConstFrozenStringN, FrozenStringValue, StringValue};
 pub use heap::{Freezer, FrozenHeap, FrozenHeapRef, Heap, Tracer};

starlark/src/values/layout/pointer.rs

@@ -134,7 +134,6 @@ impl<'p, P> Pointer<'p, P> {
     }
 
     /// Unpack pointer when it is known to be not an integer.
-    #[allow(dead_code)] // TODO: remove in the following diff
     pub(crate) unsafe fn unpack_ptr_no_int_unchecked(self) -> &'p P {
         let p = self.pointer.get();
         debug_assert!(p & TAG_INT == 0);