Add support for floats #34

pierd · 2021-09-30T11:02:56Z

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

ndmitchell

This code looks really good! Thanks so much for taking the time. A few minor minor comments, and then I'll start the merge. So you are aware, the merge process requires someone else at Facebook to look at this diff with our internal tooling and CI, and if they have any comments that are minor I'll make the small tweaks, but if they have larger comments I'll come back to you. I don't foresee any issues though, since I've reviewed it thoroughly, and it really is excellent.

ndmitchell · 2021-10-01T07:25:10Z

starlark/src/stdlib/funcs.rs

+            Ok(if b { 1.0 } else { 0.0 })
+        } else {
+            Err(anyhow!(
+                "float() argument must be a string or a number, not '{}'",


We tend to use backticks around fragments of stuff in an error message.

ndmitchell · 2021-10-01T07:26:25Z

starlark/src/stdlib/funcs.rs

+                "int() cannot convert non-string with explicit base '{}'",
+                base.to_repr()
+            ))
+        } else if let Some(Num::Float(f)) = a.unpack_num() {


Could we add some more test cases above for int on float

ndmitchell · 2021-10-01T07:29:16Z

starlark/src/values/num.rs

+
+//! Helpers for numerical values.
+
+use super::Value;


We tend to avoid use super

ndmitchell · 2021-10-01T07:36:59Z

starlark/src/values/num.rs

+use super::Value;
+
+#[derive(Clone, Copy, Debug)]
+pub enum Num {


Can we have some comments as to the purpose of this type, since it is both subtle and fundamental to this patch. My understanding is that we introduce Num so that things that operate on any numeric types (e.g. add) can easily be written in a way that deals with both. I imagine one day we'll have a BigInt type which also lives in this region. Is that accurate?

That's exactly the idea. I was even thinking if I should move the arithmetic operations from int and float to Num. It could be done in this PR or with some future work on BigInt.

facebook-github-bot · 2021-10-01T17:49:44Z

@ndmitchell has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

bobyangyf · 2021-10-01T21:28:55Z

starlark/src/values/types/float.rs

+//! The floating point number type (3.14, 4e2).
+
+use std::cmp::Ordering;
+


delete the blank line

bobyangyf · 2021-10-01T21:44:44Z

starlark/src/values/types/float.rs

+            "NaN".to_string()
+        } else if self.is_infinite() {
+            if self.is_sign_positive() {
+                "Infinity"
+            } else {
+                "-Infinity"
+            }
+            .to_string()
+        } else {
+            self.to_string()


every branch as a to_string, you can pull that out and do if ... else { ... }.to_string()

The to_string in the final branch is a different to_string, since it is going from f64, not a &str. As it happens, I changed these to to_owned in the mirror I put up, since one of our linters requests that - but left the positioning the same.

bobyangyf · 2021-10-01T21:48:52Z

starlark/src/values/types/float.rs

+                        // This shouldn't happen as we handle potential NaNs above
+                        ValueError::unsupported_with(self, "==", other)


is unreachable more appropriate here, since logically, we should never hit the case where f64's partial_cmp returns Non

I don't trust floating point values enough. There are so many weird special cases, i think having a default be an anyhow error rather than an unreachable is safer.

bobyangyf · 2021-10-01T21:50:06Z

starlark/testcases/eval/go/float.star

+assert.eq(float(p53+1), p53+0) #
+assert.eq(float(p53+2), p53+2)
+assert.eq(float(p53+3), p53+4) #
+assert.eq(float(p53+4), p53+4)
+assert.eq(float(p53+5), p53+4) #
+assert.eq(float(p53+6), p53+6)
+assert.eq(float(p53+7), p53+8) #


why do these lines end with a trailing #, did you intend on adding some comments?

Those come directly from https://github.com/google/starlark-go/blob/87f333178d5942de51b193111d6f636c79833ea5/starlark/testdata/float.star#L49 I think the original author wanted to point out the cases where float loses precision.

I did only some formatting changes to that file to allow for easier discarding of nonconformant lines:

0a1,3 > # @generated > # Copied from https://github.com/google/starlark-go/blob/87f333178d5942de51b193111d6f636c79833ea5/starlark/testdata/ > 237,239c240 < assert.eq( < str(sorted([inf, neginf, nan, 1e300, -1e300, 1.0, -1.0, 1, -1, 1e-300, -1e-300, 0, 0.0, negzero, 1e-300, -1e-300])), < "[-inf, -1e+300, -1.0, -1, -1e-300, -1e-300, 0, 0.0, -0.0, 1e-300, 1e-300, 1.0, 1, 1e+300, +inf, nan]") --- > assert.eq(str(sorted([inf, neginf, nan, 1e300, -1e300, 1.0, -1.0, 1, -1, 1e-300, -1e-300, 0, 0.0, negzero, 1e-300, -1e-300])), "[-inf, -1e+300, -1.0, -1, -1e-300, -1e-300, 0, 0.0, -0.0, 1e-300, 1e-300, 1.0, 1, 1e+300, +inf, nan]") 384,385c385,390 < for a in [1.23e100, 1.23e10, 1.23e1, 1.23, < 1, 4294967295, 8589934591, 9223372036854775807]: --- > for a in [ > 1.23e100, # int overflow in starlark-rust > 1.23e10, # int overflow in starlark-rust > 1.23e1, 1.23, 1, > 4294967295, 8589934591, 9223372036854775807, # int overflow in starlark-rust > ]:

This file is from Bazel, so nothing to do with us :)

ndmitchell · 2021-10-01T22:01:24Z

@pierd - leave those changes from @bobyangyf with me - I'll make them on the internal copy I mirrored, as I had to make a few small formatting fixes in the import.

ndmitchell · 2021-10-04T20:21:52Z

To keep you updated, it was in the process of being merged when the current Facebook outage happened, which has delayed the merge. I made a few formatting and whitespace tweaks, fixed some error messages, but very little. Hopefully this will land tomorrow.

pierd · 2021-10-04T22:38:05Z

No worries, thanks for the update.

ndmitchell · 2021-10-05T11:18:19Z

All merged! There was one last remark about -0.0 and 0.0 need to hash to the same thing which I tweaked, but otherwise it's the same.

After that, as part of the move to have repr be Display (which encourages Rust authors to give a good repr) I had to make the f64 wrapped to be a StarlarkFloat, so we can customise the Display instance - but it was pretty simple and mechanical to change.

Thanks very much for your contribution 🥇

pierd added 22 commits September 10, 2021 18:54

WIP

5ec6b13

Implement float arithmetic

e88f427

Implement float()

0267b5d

Start conformance testing

cebb874

Merge remote-tracking branch 'origin/main' into floats

4ef7bdd

Fix merge

eef55f7

Add assert_lt

69641bd

Add support for checking if a float is in range

91340a6

Introduce num module for numerical values helpers

e50ce39

Fix modulo arithmetic for floats

e0e58c7

Fix some conformance test failures and filter others

0e16fa8

Simplify Num::as_int

ef94fc4

Merge remote-tracking branch 'origin/main' into floats

0e77255

Fix merged changes

4fec218

Fix using floats as dict keys

670098f

Clean up conformance tests

d4e345d

Add more tests and comments to num

138d1d9

Conform to more tests

26ddf62

Fix float to_json

5f9362b

Format

e52f961

Fix typo

5acf1a1

Update README

c15b628

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 30, 2021

ndmitchell reviewed Oct 1, 2021

View reviewed changes

Address review comments

b250b66

bobyangyf reviewed Oct 1, 2021

View reviewed changes

facebook-github-bot closed this in da8000d Oct 5, 2021

pierd deleted the floats branch October 5, 2021 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for floats #34

Add support for floats #34

pierd commented Sep 30, 2021

ndmitchell left a comment

ndmitchell Oct 1, 2021

ndmitchell Oct 1, 2021

ndmitchell Oct 1, 2021

ndmitchell Oct 1, 2021

pierd Oct 1, 2021

facebook-github-bot commented Oct 1, 2021

bobyangyf Oct 1, 2021

bobyangyf Oct 1, 2021

ndmitchell Oct 2, 2021

bobyangyf Oct 1, 2021

ndmitchell Oct 2, 2021

bobyangyf Oct 1, 2021

pierd Oct 2, 2021

ndmitchell Oct 2, 2021

ndmitchell commented Oct 1, 2021

ndmitchell commented Oct 4, 2021

pierd commented Oct 4, 2021

ndmitchell commented Oct 5, 2021

		//! The floating point number type (3.14, 4e2).

		use std::cmp::Ordering;

		// This shouldn't happen as we handle potential NaNs above
		ValueError::unsupported_with(self, "==", other)

Add support for floats #34

Add support for floats #34

Conversation

pierd commented Sep 30, 2021

ndmitchell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndmitchell commented Oct 1, 2021

ndmitchell commented Oct 4, 2021

pierd commented Oct 4, 2021

ndmitchell commented Oct 5, 2021