Add support for floats (#34)

Summary: Addresses: #3 Spec: https://github.com/bazelbuild/starlark/blob/689f54426951638ef5b7c41a14d8fc48e65c5f77/spec.md#floating-point-numbers Changes: - adds lexer support for float literal - adds `/` and `/=` operators for division - adds floats and division operators support to parser - adds `float` function - updates int arithmetic and comparisons to account for the other operand being float - implements `SimpleValue` for float (backed by `f64`) - adds `assert_lt` implementation (required for some conformance tests) - adds `float.star` from starlark-go (with some formatting for easier nonconformant line removal) Known issues: - `str` for floats doesn't conform to the spec - string interpolation is not implemented for floats Pull Request resolved: #34 Reviewed By: krallin Differential Revision: D31345070 Pulled By: ndmitchell fbshipit-source-id: 510b0533375265187db26534c1d4683a0f2cf49a
facebook · Oct 5, 2021 · da8000d · da8000d
1 parent 721d826
commit da8000d
Show file tree

Hide file tree

Showing 22 changed files with 1,230 additions and 52 deletions.
diff --git a/README.md b/README.md
@@ -41,7 +41,7 @@ There are three components:
 In this section we outline where we don't comply with the [Starlark spec](https://github.com/bazelbuild/starlark/blob/master/spec.md).
 
 * We have plenty of extensions, e.g. type annotations, recursion, top-level `for`.
-* We don't yet support later additions to Starlark, such as [floats](https://github.com/facebookexperimental/starlark-rust/issues/3) or [bytes](https://github.com/facebookexperimental/starlark-rust/issues/4).
+* We don't yet support later additions to Starlark, such as [bytes](https://github.com/facebookexperimental/starlark-rust/issues/4).
 * We are currently limited to [32 bit integers](https://github.com/facebookexperimental/starlark-rust/issues/6). Constructing larger values will result in Starlark failing with an overflow error.
 * In some cases creating circular data structures may lead to stack overflows.
 

diff --git a/starlark/src/analysis/dubious.rs b/starlark/src/analysis/dubious.rs
@@ -22,6 +22,7 @@ use crate::{
         ast::{AstExpr, AstLiteral, Expr},
         AstModule,
     },
+    values::num::Num,
 };
 use gazebo::variants::VariantName;
 use std::collections::HashMap;
@@ -46,6 +47,7 @@ fn duplicate_dictionary_key(module: &AstModule, res: &mut Vec<LintT<Dubious>>) {
     #[derive(PartialEq, Eq, Hash)]
     enum Key<'a> {
         Int(i32),
+        Float(u64),
         String(&'a str),
         Identifier(&'a str),
     }
@@ -54,6 +56,18 @@ fn duplicate_dictionary_key(module: &AstModule, res: &mut Vec<LintT<Dubious>>) {
         match &**x {
             Expr::Literal(x) => match &*x {
                 AstLiteral::IntLiteral(x) => Some((Key::Int(x.node), x.span)),
+                AstLiteral::FloatLiteral(x) => {
+                    let n = Num::from(x.node);
+                    if let Some(i) = n.as_int() {
+                        // make an integer float always collide with other ints
+                        Some((Key::Int(i), x.span))
+                    } else {
+                        // use bits representation of float to be able to always compare them for equality
+                        // First normalise -0.0
+                        let v = if x.node == 0.0 { 0.0 } else { x.node };
+                        Some((Key::Float(v.to_bits()), x.span))
+                    }
+                }
                 AstLiteral::StringLiteral(x) => Some((Key::String(&x.node), x.span)),
             },
             Expr::Identifier(x, ()) => Some((Key::Identifier(&x.node), x.span)),
@@ -117,6 +131,8 @@ mod test {
             r#"
 {'no1': 1, 'no1': 2}
 {42: 1, 78: 9, 'no2': 100, 42: 6, 'no2': 8}
+{123.0: "f", 123: "i"}
+{0.25: "frac", 25e-2: "exp"}
 
 # Variables can't change as a result of expression evaluation,
 # so it's always an error if you see the same expression
@@ -130,7 +146,9 @@ mod test {
         duplicate_dictionary_key(&m, &mut res);
         assert_eq!(
             res.map(|x| x.problem.about()),
-            &["\"no1\"", "42", "\"no2\"", "no3", "no3", "no4"]
+            &[
+                "\"no1\"", "42", "\"no2\"", "123", "0.25", "no3", "no3", "no4"
+            ]
         );
     }
 }
diff --git a/starlark/src/assert/assert.rs b/starlark/src/assert/assert.rs
@@ -68,6 +68,14 @@ fn assert_different<'v>(a: Value<'v>, b: Value<'v>) -> anyhow::Result<NoneType>
     }
 }
 
+fn assert_less_than<'v>(a: Value<'v>, b: Value<'v>) -> anyhow::Result<NoneType> {
+    if a.compare(b)? != std::cmp::Ordering::Less {
+        Err(anyhow!("assert_lt: but {} >= {}", a, b))
+    } else {
+        Ok(NoneType)
+    }
+}
+
 /// How often we garbage collection _should_ be transparent to the tests,
 /// so we run each test in three configurations.
 #[derive(Clone, Copy, Dupe, Debug)]
@@ -89,6 +97,10 @@ fn assert_star(builder: &mut crate::environment::GlobalsBuilder) {
         assert_different(a, b)
     }
 
+    fn lt(a: Value, b: Value) -> NoneType {
+        assert_less_than(a, b)
+    }
+
     fn contains(xs: Value, x: Value) -> NoneType {
         if !xs.is_in(x)? {
             Err(anyhow!("assert.contains: expected {} to be in {}", x, xs))
@@ -137,6 +149,10 @@ fn test_methods(builder: &mut GlobalsBuilder) {
         assert_different(a, b)
     }
 
+    fn assert_lt(a: Value, b: Value) -> NoneType {
+        assert_less_than(a, b)
+    }
+
     fn assert_true(a: Value) -> NoneType {
         if !a.to_bool() {
             Err(anyhow!("assertion failed"))

diff --git a/starlark/src/eval/fragment/expr.rs b/starlark/src/eval/fragment/expr.rs
@@ -68,6 +68,7 @@ pub(crate) enum ExprBinOp {
     Add,
     Multiply,
     Percent,
+    Divide,
     FloorDivide,
     BitAnd,
     BitOr,
@@ -417,6 +418,9 @@ impl Spanned<ExprCompiledValue> {
                 ExprBinOp::Percent => expr!("percent", l, r, |eval| {
                     expr_throw(l.percent(r, eval.heap()), span, eval)?
                 }),
+                ExprBinOp::Divide => expr!("divide", l, r, |eval| {
+                    expr_throw(l.div(r, eval.heap()), span, eval)?
+                }),
                 ExprBinOp::FloorDivide => expr!("floor_divide", l, r, |eval| {
                     expr_throw(l.floor_div(r, eval.heap()), span, eval)?
                 }),
@@ -778,6 +782,7 @@ impl AstLiteral {
     fn compile(&self, heap: &FrozenHeap) -> FrozenValue {
         match self {
             AstLiteral::IntLiteral(i) => FrozenValue::new_int(i.node),
+            AstLiteral::FloatLiteral(f) => heap.alloc(f.node),
             AstLiteral::StringLiteral(x) => heap.alloc(x.node.as_str()),
         }
     }
@@ -1030,6 +1035,7 @@ impl Compiler<'_> {
                         BinOp::Add => ExprCompiledValue::Op(ExprBinOp::Add, box (l, r)),
                         BinOp::Multiply => ExprCompiledValue::Op(ExprBinOp::Multiply, box (l, r)),
                         BinOp::Percent => ExprCompiledValue::Op(ExprBinOp::Percent, box (l, r)),
+                        BinOp::Divide => ExprCompiledValue::Op(ExprBinOp::Divide, box (l, r)),
                         BinOp::FloorDivide => {
                             ExprCompiledValue::Op(ExprBinOp::FloorDivide, box (l, r))
                         }

diff --git a/starlark/src/eval/fragment/stmt.rs b/starlark/src/eval/fragment/stmt.rs
@@ -902,6 +902,9 @@ impl Compiler<'_> {
                     AssignOp::Multiply => {
                         self.assign_modify(span, lhs, rhs, |l, r, eval| l.mul(r, eval.heap()))
                     }
+                    AssignOp::Divide => {
+                        self.assign_modify(span, lhs, rhs, |l, r, eval| l.div(r, eval.heap()))
+                    }
                     AssignOp::FloorDivide => {
                         self.assign_modify(span, lhs, rhs, |l, r, eval| l.floor_div(r, eval.heap()))
                     }

diff --git a/starlark/src/eval/tests/go.rs b/starlark/src/eval/tests/go.rs
@@ -49,20 +49,11 @@ fn test_go() {
         ],
     );
     // Skip benchmark.star, for benchmarking not testing
-    assert.conformance(&ignore_bad_lines(
-        test_case!("bool.star"),
-        &[
-            "0.0", // Floats, unsupported
-        ],
-    ));
+    assert.conformance(test_case!("bool.star"));
     assert.conformance(&ignore_bad_lines(
         test_case!("builtin.star"),
         &[
-            "2.0",                   // Floats, unsupported
             "[] not in {123: \"\"}", // We disagree, see test_not_in_unhashable
-            // Exponents, unsupported
-            "1e15",
-            "1e100",
             // Set, unsupported
             "set(",
             "(myset)",
@@ -101,7 +92,32 @@ fn test_go() {
             "Verify position of an \"unhashable key\"", // FIXME: we should do better
         ],
     );
-    // Skip float.star, since we don't support floats
+    assert.conformance(&ignore_bad_lines(
+        test_case!("float.star"),
+        &[
+            // int's outside our range
+            "1229999999999999973",
+            "9223372036854775808",
+            "1000000000000000000",
+            "1230000000000000049",
+            "9223372036854775807",
+            "p53",
+            "maxint64",
+            "int too large to convert to float",
+            "int(1e100)",
+            "1000000 * 1000000 * 1000000",
+            "int overflow in starlark-rust",
+            // str for floats doesn't conform to the spec
+            "assert.eq(str(1.23e45),",
+            "assert.eq(str(-1.23e-45),",
+            "assert.eq(str(sorted([inf, neginf, nan, 1e300, -1e300,",
+            // string interpolation for floats not implemented
+            "%d",
+            "%e",
+            "%f",
+            "%g",
+        ],
+    ));
     assert.conformance(&ignore_bad_lines(
         test_case!("function.star"),
         &[
@@ -117,7 +133,6 @@ fn test_go() {
         &ignore_bad_lines(
             test_case!("misc.star"),
             &[
-                "2.0",                          // We don't support float
                 "'<built-in function freeze>'", // Different display of functions
             ],
         ),

diff --git a/starlark/src/lib.rs b/starlark/src/lib.rs
@@ -389,6 +389,7 @@
 #![allow(clippy::wrong_self_convention)]
 #![allow(clippy::comparison_chain)]
 #![allow(clippy::missing_safety_doc)]
+#![allow(clippy::float_cmp)]
 // FIXME: Temporary
 #![allow(clippy::useless_transmute)] // Seems to be a clippy bug, but we should be using less transmute anyway
 

diff --git a/starlark/src/stdlib/funcs.rs b/starlark/src/stdlib/funcs.rs
@@ -26,10 +26,12 @@ use crate::{
     values::{
         bool::BOOL_TYPE,
         dict::Dict,
+        float::FLOAT_TYPE,
         function::{BoundMethod, NativeAttribute},
         int::INT_TYPE,
         list::List,
         none::NoneType,
+        num::Num,
         range::Range,
         string::STRING_TYPE,
         tuple::Tuple,
@@ -320,6 +322,63 @@ pub(crate) fn global_functions(builder: &mut GlobalsBuilder) {
         Ok(List::new(v))
     }
 
+    /// [float](
+    /// https://github.com/google/skylark/blob/a5f7082aabed29c0e429c722292c66ec8ecf9591/doc/spec.md#float
+    /// ): interprets its argument as a floating-point number.
+    ///
+    /// If x is a `float`, the result is x.
+    /// if x is an `int`, the result is the nearest floating point value to x.
+    /// If x is a string, the string is interpreted as a floating-point literal.
+    /// With no arguments, `float()` returns `0.0`.
+    ///
+    /// ```
+    /// # starlark::assert::all_true(r#"
+    /// float() == 0.0
+    /// float(1) == 1.0
+    /// float('1') == 1.0
+    /// float('1.0') == 1.0
+    /// float('.25') == 0.25
+    /// float('1e2') == 100.0
+    /// float(False) == 0.0
+    /// float(True) == 1.0
+    /// # "#);
+    /// # starlark::assert::fail(r#"
+    /// float("hello")   # error: not a valid number
+    /// # "#, "not a valid number");
+    /// # starlark::assert::fail(r#"
+    /// float([])   # error: argument must be a string, a number, or a boolean
+    /// # "#, "argument must be a string, a number, or a boolean");
+    /// ```
+    #[starlark_type(FLOAT_TYPE)]
+    fn float(ref a: Option<Value>) -> f64 {
+        if a.is_none() {
+            return Ok(0.0);
+        }
+        let a = a.unwrap();
+        if let Some(f) = a.unpack_num().map(|n| n.as_float()) {
+            Ok(f)
+        } else if let Some(s) = a.unpack_str() {
+            match s.parse::<f64>() {
+                Ok(f) => {
+                    if f.is_infinite() && !s.to_lowercase().contains("inf") {
+                        // if a resulting float is infinite but the parsed string is not explicitly infinity then we should fail with an error
+                        Err(anyhow!("float() floating-point number too large: {}", s))
+                    } else {
+                        Ok(f)
+                    }
+                }
+                Err(x) => Err(anyhow!("{} is not a valid number: {}", a.to_repr(), x)),
+            }
+        } else if let Some(b) = a.unpack_bool() {
+            Ok(if b { 1.0 } else { 0.0 })
+        } else {
+            Err(anyhow!(
+                "float() argument must be a string, a number, or a boolean, not `{}`",
+                a.get_type()
+            ))
+        }
+    }
+
     /// [getattr](
     /// https://github.com/google/skylark/blob/a0e5de7e63b47e716cca7226662a4c95d47bf873/doc/spec.md#getattr
     /// ): returns the value of an attribute
@@ -438,10 +497,23 @@ pub(crate) fn global_functions(builder: &mut GlobalsBuilder) {
     /// int('16', 10) == 16
     /// int('16', 8) == 14
     /// int('16', 16) == 22
+    /// int(0.0) == 0
+    /// int(3.14) == 3
+    /// int(-12345.6789) == -12345
+    /// int(2e9) == 2000000000
     /// # "#);
     /// # starlark::assert::fail(r#"
     /// int("hello")   # error: not a valid number
     /// # "#, "not a valid number");
+    /// # starlark::assert::fail(r#"
+    /// int(1e100)   # error: overflow
+    /// # "#, "cannot convert float to integer");
+    /// # starlark::assert::fail(r#"
+    /// int(float("nan"))   # error: cannot convert NaN to int
+    /// # "#, "cannot convert float to integer");
+    /// # starlark::assert::fail(r#"
+    /// int(float("inf"))   # error: cannot convert infinity to int
+    /// # "#, "cannot convert float to integer");
     /// ```
     #[starlark_type(INT_TYPE)]
     fn int(ref a: Option<Value>, base: Option<Value>) -> i32 {
@@ -510,14 +582,21 @@ pub(crate) fn global_functions(builder: &mut GlobalsBuilder) {
                     x,
                 )),
             }
-        } else {
-            match base {
-                Some(base) => Err(anyhow!(
-                    "int() cannot convert non-string with explicit base '{}'",
-                    base.to_repr()
+        } else if let Some(base) = base {
+            Err(anyhow!(
+                "int() cannot convert non-string with explicit base '{}'",
+                base.to_repr()
+            ))
+        } else if let Some(Num::Float(f)) = a.unpack_num() {
+            match Num::from(f.trunc()).as_int() {
+                Some(i) => Ok(i),
+                None => Err(anyhow!(
+                    "int() cannot convert float to integer: {}",
+                    a.to_repr()
                 )),
-                None => Ok(a.to_int()?),
             }
+        } else {
+            Ok(a.to_int()?)
         }
     }