rust: create `types` module with primitives like `Step` #4342

wchargin · 2020-11-18T04:38:07Z

Summary:
We’d defined a Step newtype in the reservoir module, but that type
will be useful more broadly. We thus move it into a new types module,
and define WallTime and Tag siblings, which we’ll need shortly.

Test Plan:
Most behavior is just #[derive]d; unit tests included for the rest.

wchargin-branch: rust-types-module

Summary: We’d defined a `Step` [newtype] in the `reservoir` module, but that type will be useful more broadly. We thus move it into a new `types` module, and define `WallTime` and `Tag` siblings, which we’ll need shortly. [newtype]: https://doc.rust-lang.org/rust-by-example/generics/new_types.html Test Plan: Most behavior is just `#[derive]`d; unit tests included for the rest. wchargin-branch: rust-types-module wchargin-source: b0f5475b247ef8f9cb8bf2e0bf94d2b648b8009e

stephanwlee · 2020-11-18T04:55:23Z

tensorboard/data/server/types.rs

+            .unwrap_or_else(|| unreachable!("{:?} <> {:?}", &self, &other))
+    }
+}
+impl Eq for WallTime {}


For my education, is this any different than #[derive(Eq)]?

It does the same thing that derive(Eq) would do, since Eq itself has
no methods (it’s a marker interface). But you’re only permitted to
derive(Eq) on types all of whose fields are also Eq. In this case,
we have a field of type f64, which is not Eq (because of NaN), so
we cannot use derive(Eq):

$ git diff -U1 diff --git a/tensorboard/data/server/types.rs b/tensorboard/data/server/types.rs index 59a764504..755329912 100644 --- a/tensorboard/data/server/types.rs +++ b/tensorboard/data/server/types.rs @@ -26,3 +26,3 @@ pub struct Step(pub i64); /// Wall times represent floating-point seconds since Unix epoch. They must be finite and non-NaN. -#[derive(Debug, PartialEq, PartialOrd, Copy, Clone)] +#[derive(Debug, PartialEq, PartialOrd, Copy, Clone, Eq)] pub struct WallTime(f64); @@ -51,3 +51,2 @@ impl Ord for WallTime { } -impl Eq for WallTime {}

$ cargo check error[E0277]: the trait bound `f64: std::cmp::Eq` is not satisfied --> types.rs:28:21 | 28 | pub struct WallTime(f64); | ^^^ the trait `std::cmp::Eq` is not implemented for `f64` |

Instead, impl Eq for WallTime {} is our explicit way of declaring that
our type really does have a total equivalence relation, even though the
types of its fields do not.

stephanwlee · 2020-11-18T04:56:16Z

tensorboard/data/server/types.rs

+
+// Wall times are totally ordered and have a total equivalence relation, since we guarantee that
+// they are not NaN.
+#[allow(clippy::derive_ord_xor_partial_ord)] // okay because it agrees with `PartialOrd` impl


reading this, it looks like PartialOrd can never disagree with Ord. If so, what is this derive_ord_xor_partial_ord really enforcing?

After reading a bit more about this https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord, why is the impl Ord
block required when already given #[derive(PartialOrd)]?

(for my education of course)

Correct: the PartialOrd and Ord implementations for a given type
must agree, else many operations are undefined. (Not the scary kind of
“undefined behavior”; it’s just a contract violation on basically every
API that cares about orderings.)

This line disables a Clippy lint, whose docs you’ve linked to. I think
that the purpose is to flag code like this:

#[derive(PartialOrd)] struct BackwardInt(i64); impl Ord for BackwardInt { fn cmp(&self, other: &Self) -> Ordering { // Our type is ordered backward! self.0.cmp(&other.0).reverse() } }

This Ord implementation is fine by itself, and you might think that
#[derive(PartialOrd)] would implement PartialOrd for our type by
delegating to its Ord. But in fact it would actually delegate to the
PartialOrds of its fields, which would make the PartialOrd and Ord
inconsistent in this case.

In our case, the derived PartialOrd is consistent with the manual
Ord (which you can tell since cmp just calls partial_cmp), so I
suppressed the lint error. We could write it out—

impl PartialOrd for WallTime { fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) } }

—but I didn’t see much reason to do that.

Here’s a topical upstream request to teach the lint to recognize this
safe pattern:
rust-lang/rust-clippy#6219

stephanwlee · 2020-11-18T05:03:58Z

tensorboard/data/server/types.rs

+        m.insert(Tag("accuracy".to_string()), 1);
+        m.insert(Tag("loss".to_string()), 2);
+        // We can call `get` given only a `&str`, not an owned `Tag`.
+        assert_eq!(m.get("accuracy"), Some(&1));


Super noob question: What would &1 mean? Is it referring to reference to an i32 1? If so, is it stable and refers to the same value to L81?

What would &1 mean? Is it referring to reference to an i32 1?

Yep! (where the i32 part is implied from type-checking)

If so, is it stable and refers to the same value to L81?

Nope! It’s like:

m.insert(Tag("accuracy".to_string()), 1); // line 81 let tmp = 1; assert_eq!(m.get("accuracy"), Some(&tmp)); // line 84 // `tmp` is implicitly dropped here

We can’t get a reference to the 1 from line 81; not only is it out of
scope, it probably never had a stack location at all (would be passed in
a register for this signature). But that’s okay, because equality on
references is defined in terms of equality on the underlying values:

In addition, the comparison operators transparently defer to the
referent's implementation, allowing references to be compared the same
as owned values.

You can get reference equality semantics by casting to pointers. This
is safe: it’s only dereferencing pointers that’s unsafe. But I’ve
never had to use this, even once. Value equality is basically always
what you want.

wchargin

Thanks for the quick review and the questions!

wchargin · 2020-11-18T05:08:46Z

tensorboard/data/server/types.rs

+            .unwrap_or_else(|| unreachable!("{:?} <> {:?}", &self, &other))
+    }
+}
+impl Eq for WallTime {}


It does the same thing that derive(Eq) would do, since Eq itself has
no methods (it’s a marker interface). But you’re only permitted to
derive(Eq) on types all of whose fields are also Eq. In this case,
we have a field of type f64, which is not Eq (because of NaN), so
we cannot use derive(Eq):

$ git diff -U1 diff --git a/tensorboard/data/server/types.rs b/tensorboard/data/server/types.rs index 59a764504..755329912 100644 --- a/tensorboard/data/server/types.rs +++ b/tensorboard/data/server/types.rs @@ -26,3 +26,3 @@ pub struct Step(pub i64); /// Wall times represent floating-point seconds since Unix epoch. They must be finite and non-NaN. -#[derive(Debug, PartialEq, PartialOrd, Copy, Clone)] +#[derive(Debug, PartialEq, PartialOrd, Copy, Clone, Eq)] pub struct WallTime(f64); @@ -51,3 +51,2 @@ impl Ord for WallTime { } -impl Eq for WallTime {}

$ cargo check error[E0277]: the trait bound `f64: std::cmp::Eq` is not satisfied --> types.rs:28:21 | 28 | pub struct WallTime(f64); | ^^^ the trait `std::cmp::Eq` is not implemented for `f64` |

Instead, impl Eq for WallTime {} is our explicit way of declaring that
our type really does have a total equivalence relation, even though the
types of its fields do not.

wchargin · 2020-11-18T05:14:47Z

tensorboard/data/server/types.rs

+
+// Wall times are totally ordered and have a total equivalence relation, since we guarantee that
+// they are not NaN.
+#[allow(clippy::derive_ord_xor_partial_ord)] // okay because it agrees with `PartialOrd` impl


Correct: the PartialOrd and Ord implementations for a given type
must agree, else many operations are undefined. (Not the scary kind of
“undefined behavior”; it’s just a contract violation on basically every
API that cares about orderings.)

This line disables a Clippy lint, whose docs you’ve linked to. I think
that the purpose is to flag code like this:

#[derive(PartialOrd)] struct BackwardInt(i64); impl Ord for BackwardInt { fn cmp(&self, other: &Self) -> Ordering { // Our type is ordered backward! self.0.cmp(&other.0).reverse() } }

This Ord implementation is fine by itself, and you might think that
#[derive(PartialOrd)] would implement PartialOrd for our type by
delegating to its Ord. But in fact it would actually delegate to the
PartialOrds of its fields, which would make the PartialOrd and Ord
inconsistent in this case.

In our case, the derived PartialOrd is consistent with the manual
Ord (which you can tell since cmp just calls partial_cmp), so I
suppressed the lint error. We could write it out—

impl PartialOrd for WallTime { fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) } }

—but I didn’t see much reason to do that.

wchargin · 2020-11-18T05:22:24Z

tensorboard/data/server/types.rs

+        m.insert(Tag("accuracy".to_string()), 1);
+        m.insert(Tag("loss".to_string()), 2);
+        // We can call `get` given only a `&str`, not an owned `Tag`.
+        assert_eq!(m.get("accuracy"), Some(&1));


What would &1 mean? Is it referring to reference to an i32 1?

Yep! (where the i32 part is implied from type-checking)

If so, is it stable and refers to the same value to L81?

Nope! It’s like:

m.insert(Tag("accuracy".to_string()), 1); // line 81 let tmp = 1; assert_eq!(m.get("accuracy"), Some(&tmp)); // line 84 // `tmp` is implicitly dropped here

We can’t get a reference to the 1 from line 81; not only is it out of
scope, it probably never had a stack location at all (would be passed in
a register for this signature). But that’s okay, because equality on
references is defined in terms of equality on the underlying values:

In addition, the comparison operators transparently defer to the
referent's implementation, allowing references to be compared the same
as owned values.

You can get reference equality semantics by casting to pointers. This
is safe: it’s only dereferencing pointers that’s unsafe. But I’ve
never had to use this, even once. Value equality is basically always
what you want.

wchargin added the core:rustboard //tensorboard/data/server/... label Nov 18, 2020

wchargin requested a review from stephanwlee November 18, 2020 04:38

google-cla bot added the cla: yes label Nov 18, 2020

stephanwlee approved these changes Nov 18, 2020

View reviewed changes

wchargin mentioned this pull request Nov 18, 2020

rust: create multi-file run loader #4343

Merged

wchargin commented Nov 18, 2020

View reviewed changes

wchargin merged commit 6086f25 into master Nov 18, 2020

wchargin deleted the wchargin-rust-types-module branch November 18, 2020 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rust: create `types` module with primitives like `Step` #4342

rust: create `types` module with primitives like `Step` #4342

Uh oh!

wchargin commented Nov 18, 2020

Uh oh!

stephanwlee Nov 18, 2020

Uh oh!

wchargin Nov 18, 2020

Uh oh!

stephanwlee Nov 18, 2020

Uh oh!

wchargin Nov 18, 2020 •

edited

Loading

Uh oh!

wchargin Nov 18, 2020

Uh oh!

stephanwlee Nov 18, 2020

Uh oh!

wchargin Nov 18, 2020

Uh oh!

wchargin left a comment

Uh oh!

wchargin Nov 18, 2020

Uh oh!

wchargin Nov 18, 2020 •

edited

Loading

Uh oh!

wchargin Nov 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rust: create types module with primitives like Step #4342

rust: create types module with primitives like Step #4342

Uh oh!

Conversation

wchargin commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchargin Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchargin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchargin Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rust: create `types` module with primitives like `Step` #4342

rust: create `types` module with primitives like `Step` #4342

wchargin Nov 18, 2020 •

edited

Loading

wchargin Nov 18, 2020 •

edited

Loading