Node is Display (#7264)

Use standard traits, rather than our own methods which happen to do the same thing.

src/rust/engine/graph/src/entry.rs

@@ -574,6 +574,6 @@ impl<N: Node> Entry<N> {
       Some(Err(ref x)) => format!("{:?}", x),
       None => "<None>".to_string(),
     };
-    format!("{} == {}", self.node.content().format(), state).replace("\"", "\\\"")
+    format!("{} == {}", self.node.content(), state).replace("\"", "\\\"")
   }
 }

src/rust/engine/graph/src/lib.rs

@@ -370,7 +370,7 @@ impl<N: Node> InnerGraph<N> {
     let format = |eid: EntryId, depth: usize, is_last: bool| -> String {
       let entry = self.unsafe_entry_for_id(eid);
       let indent = "  ".repeat(depth);
-      let output = format!("{}Computing {}", indent, entry.node().format());
+      let output = format!("{}Computing {}", indent, entry.node());
       if is_last {
         format!(
           "{}\n{}  {}",
@@ -430,7 +430,7 @@ impl<N: Node> InnerGraph<N> {
 
       if deps.peek().is_none() {
         // If the entry has no running deps, it is a leaf. Emit it.
-        res.insert(self.unsafe_entry_for_id(id).node().format(), duration);
+        res.insert(format!("{}", self.unsafe_entry_for_id(id).node()), duration);
         if res.len() >= k {
           break;
         }
@@ -1055,10 +1055,6 @@ mod tests {
       }
     }
 
-    fn format(&self) -> String {
-      format!("{:?}", self)
-    }
-
     fn digest(_result: Self::Item) -> Option<Digest> {
       None
     }
@@ -1068,6 +1064,12 @@ mod tests {
     }
   }
 
+  impl std::fmt::Display for TNode {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
+      write!(f, "{:?}", self)
+    }
+  }
+
   impl TNode {
     ///
     /// Validates the given TNode output. Both node ids and context ids should increase left to

src/rust/engine/graph/src/node.rs

@@ -1,7 +1,7 @@
 // Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
 // Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-use std::fmt::Debug;
+use std::fmt::{Debug, Display};
 use std::hash::Hash;
 
 use boxfuture::BoxFuture;
@@ -21,17 +21,14 @@ pub type EntryId = stable_graph::NodeIndex<u32>;
 ///
 /// Note that it is assumed that Nodes are very cheap to clone.
 ///
-pub trait Node: Clone + Debug + Eq + Hash + Send + 'static {
+pub trait Node: Clone + Debug + Display + Eq + Hash + Send + 'static {
   type Context: NodeContext<Node = Self>;
 
   type Item: Clone + Debug + Eq + Send + 'static;
   type Error: NodeError;
 
   fn run(self, context: Self::Context) -> BoxFuture<Self::Item, Self::Error>;
 
-  // TODO: Use a `Display` bound instead.
-  fn format(&self) -> String;
-
   ///
   /// If the given Node output represents an FS operation, returns its Digest.
   ///

src/rust/engine/src/nodes.rs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0 (see LICENSE).
 
 use std::collections::{BTreeMap, HashMap};
+use std::fmt::Display;
 use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
@@ -1061,27 +1062,6 @@ impl Node for NodeKey {
     }
   }
 
-  fn format(&self) -> String {
-    fn keystr(key: &Key) -> String {
-      externs::key_to_str(&key)
-    }
-    fn typstr(tc: &TypeConstraint) -> String {
-      externs::key_to_str(&tc.0)
-    }
-    // TODO: these should all be converted to fmt::Debug implementations, and then this method can
-    // go away in favor of the auto-derived Debug for this type.
-    match self {
-      &NodeKey::DigestFile(ref s) => format!("DigestFile({:?})", s.0),
-      &NodeKey::DownloadedFile(ref s) => format!("DownloadedFile({:?})", s.0),
-      &NodeKey::ExecuteProcess(ref s) => format!("ExecuteProcess({:?}", s.0),
-      &NodeKey::ReadLink(ref s) => format!("ReadLink({:?})", s.0),
-      &NodeKey::Scandir(ref s) => format!("Scandir({:?})", s.0),
-      &NodeKey::Select(ref s) => format!("Select({}, {})", s.params, typstr(&s.product)),
-      &NodeKey::Task(ref s) => format!("{:?}", s),
-      &NodeKey::Snapshot(ref s) => format!("Snapshot({})", keystr(&s.0)),
-    }
-  }
-
   fn digest(res: NodeResult) -> Option<hashing::Digest> {
     match res {
       NodeResult::Digest(d) => Some(d),
@@ -1103,6 +1083,26 @@ impl Node for NodeKey {
   }
 }
 
+impl Display for NodeKey {
+  fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
+    match self {
+      &NodeKey::DigestFile(ref s) => write!(f, "DigestFile({:?})", s.0),
+      &NodeKey::DownloadedFile(ref s) => write!(f, "DownloadedFile({:?})", s.0),
+      &NodeKey::ExecuteProcess(ref s) => write!(f, "ExecuteProcess({:?}", s.0),
+      &NodeKey::ReadLink(ref s) => write!(f, "ReadLink({:?})", s.0),
+      &NodeKey::Scandir(ref s) => write!(f, "Scandir({:?})", s.0),
+      &NodeKey::Select(ref s) => write!(
+        f,
+        "Select({}, {})",
+        s.params,
+        externs::key_to_str(&s.product.0)
+      ),
+      &NodeKey::Task(ref s) => write!(f, "{:?}", s),
+      &NodeKey::Snapshot(ref s) => write!(f, "Snapshot({})", externs::key_to_str(&s.0)),
+    }
+  }
+}
+
 impl NodeError for Failure {
   fn invalidated() -> Failure {
     Failure::Invalidated

src/rust/engine/src/scheduler.rs

@@ -13,7 +13,7 @@ use crate::context::{Context, Core};
 use crate::core::{Failure, Params, TypeConstraint, Value};
 use crate::nodes::{NodeKey, Select, Tracer, TryInto, Visualizer};
 use crate::selectors;
-use graph::{EntryId, Graph, InvalidationResult, Node, NodeContext};
+use graph::{EntryId, Graph, InvalidationResult, NodeContext};
 use indexmap::IndexMap;
 use log::{debug, info, warn};
 use parking_lot::Mutex;
@@ -210,10 +210,7 @@ impl Scheduler {
                   // Otherwise (if it is a success, some other type of Failure, or if we've run
                   // out of retries) recover to complete the join, which will cause the results to
                   // propagate to the user.
-                  debug!(
-                    "Root {} completed.",
-                    NodeKey::Select(Box::new(root)).format()
-                  );
+                  debug!("Root {} completed.", NodeKey::Select(Box::new(root)));
                   Ok(other.map(|res| {
                     res
                       .try_into()