From d8478ca690e7f7f99f52567d0386bc21612a8436 Mon Sep 17 00:00:00 2001
From: Reilly Wood <26268125+rgwood@users.noreply.github.com>
Date: Sat, 25 Mar 2023 00:17:20 -0700
Subject: [PATCH] Clean up unnecessary macro use (#8607)

Some minor code cleanup.

We've accumulated a few macros over the years that arguably don't need
to be macros. This PR removes 4 macros by either:
1. Inlining the macro
2. Replacing the macro with a local function
3. Replacing the macro with a closure
---
 crates/nu-cli/src/syntax_highlight.rs        |  62 ++++++-----
 crates/nu-color-config/src/nu_style.rs       |  32 +++---
 crates/nu-color-config/src/style_computer.rs |  13 ++-
 crates/nu-explore/src/commands/help.rs       |  26 +++--
 crates/nu-protocol/src/value/mod.rs          | 104 ++++++-------------
 5 files changed, 96 insertions(+), 141 deletions(-)

diff --git a/crates/nu-cli/src/syntax_highlight.rs b/crates/nu-cli/src/syntax_highlight.rs
index 589cc719ba..77764599fd 100644
--- a/crates/nu-cli/src/syntax_highlight.rs
+++ b/crates/nu-cli/src/syntax_highlight.rs
@@ -79,29 +79,27 @@ impl Highlighter for NuHighlighter {
                 }};
             }
 
-            macro_rules! add_colored_token {
-                ($shape:expr, $text:expr) => {
-                    output.push((get_shape_color($shape.to_string(), &self.config), $text))
-                };
-            }
+            let mut add_colored_token = |shape: &FlatShape, text: String| {
+                output.push((get_shape_color(shape.to_string(), &self.config), text));
+            };
 
             match shape.1 {
-                FlatShape::Garbage => add_colored_token!(shape.1, next_token),
-                FlatShape::Nothing => add_colored_token!(shape.1, next_token),
-                FlatShape::Binary => add_colored_token!(shape.1, next_token),
-                FlatShape::Bool => add_colored_token!(shape.1, next_token),
-                FlatShape::Int => add_colored_token!(shape.1, next_token),
-                FlatShape::Float => add_colored_token!(shape.1, next_token),
-                FlatShape::Range => add_colored_token!(shape.1, next_token),
-                FlatShape::InternalCall => add_colored_token!(shape.1, next_token),
-                FlatShape::External => add_colored_token!(shape.1, next_token),
-                FlatShape::ExternalArg => add_colored_token!(shape.1, next_token),
-                FlatShape::Literal => add_colored_token!(shape.1, next_token),
-                FlatShape::Operator => add_colored_token!(shape.1, next_token),
-                FlatShape::Signature => add_colored_token!(shape.1, next_token),
-                FlatShape::String => add_colored_token!(shape.1, next_token),
-                FlatShape::StringInterpolation => add_colored_token!(shape.1, next_token),
-                FlatShape::DateTime => add_colored_token!(shape.1, next_token),
+                FlatShape::Garbage => add_colored_token(&shape.1, next_token),
+                FlatShape::Nothing => add_colored_token(&shape.1, next_token),
+                FlatShape::Binary => add_colored_token(&shape.1, next_token),
+                FlatShape::Bool => add_colored_token(&shape.1, next_token),
+                FlatShape::Int => add_colored_token(&shape.1, next_token),
+                FlatShape::Float => add_colored_token(&shape.1, next_token),
+                FlatShape::Range => add_colored_token(&shape.1, next_token),
+                FlatShape::InternalCall => add_colored_token(&shape.1, next_token),
+                FlatShape::External => add_colored_token(&shape.1, next_token),
+                FlatShape::ExternalArg => add_colored_token(&shape.1, next_token),
+                FlatShape::Literal => add_colored_token(&shape.1, next_token),
+                FlatShape::Operator => add_colored_token(&shape.1, next_token),
+                FlatShape::Signature => add_colored_token(&shape.1, next_token),
+                FlatShape::String => add_colored_token(&shape.1, next_token),
+                FlatShape::StringInterpolation => add_colored_token(&shape.1, next_token),
+                FlatShape::DateTime => add_colored_token(&shape.1, next_token),
                 FlatShape::List => {
                     add_colored_token_with_bracket_highlight!(shape.1, shape.0, next_token)
                 }
@@ -116,17 +114,17 @@ impl Highlighter for NuHighlighter {
                     add_colored_token_with_bracket_highlight!(shape.1, shape.0, next_token)
                 }
 
-                FlatShape::Filepath => add_colored_token!(shape.1, next_token),
-                FlatShape::Directory => add_colored_token!(shape.1, next_token),
-                FlatShape::GlobPattern => add_colored_token!(shape.1, next_token),
-                FlatShape::Variable => add_colored_token!(shape.1, next_token),
-                FlatShape::Flag => add_colored_token!(shape.1, next_token),
-                FlatShape::Pipe => add_colored_token!(shape.1, next_token),
-                FlatShape::And => add_colored_token!(shape.1, next_token),
-                FlatShape::Or => add_colored_token!(shape.1, next_token),
-                FlatShape::Redirection => add_colored_token!(shape.1, next_token),
-                FlatShape::Custom(..) => add_colored_token!(shape.1, next_token),
-                FlatShape::MatchPattern => add_colored_token!(shape.1, next_token),
+                FlatShape::Filepath => add_colored_token(&shape.1, next_token),
+                FlatShape::Directory => add_colored_token(&shape.1, next_token),
+                FlatShape::GlobPattern => add_colored_token(&shape.1, next_token),
+                FlatShape::Variable => add_colored_token(&shape.1, next_token),
+                FlatShape::Flag => add_colored_token(&shape.1, next_token),
+                FlatShape::Pipe => add_colored_token(&shape.1, next_token),
+                FlatShape::And => add_colored_token(&shape.1, next_token),
+                FlatShape::Or => add_colored_token(&shape.1, next_token),
+                FlatShape::Redirection => add_colored_token(&shape.1, next_token),
+                FlatShape::Custom(..) => add_colored_token(&shape.1, next_token),
+                FlatShape::MatchPattern => add_colored_token(&shape.1, next_token),
             }
             last_seen_span = shape.0.end;
         }
diff --git a/crates/nu-color-config/src/nu_style.rs b/crates/nu-color-config/src/nu_style.rs
index 0d85726037..d84bf27667 100644
--- a/crates/nu-color-config/src/nu_style.rs
+++ b/crates/nu-color-config/src/nu_style.rs
@@ -20,22 +20,26 @@ impl From<Style> for NuStyle {
 }
 
 fn style_get_attr(s: Style) -> Option<String> {
-    macro_rules! check {
-        ($attrs:expr, $b:expr, $c:expr) => {
-            if $b {
-                $attrs.push($c);
-            }
-        };
-    }
-
     let mut attrs = String::new();
 
-    check!(attrs, s.is_blink, 'l');
-    check!(attrs, s.is_bold, 'b');
-    check!(attrs, s.is_dimmed, 'd');
-    check!(attrs, s.is_reverse, 'r');
-    check!(attrs, s.is_strikethrough, 's');
-    check!(attrs, s.is_underline, 'u');
+    if s.is_blink {
+        attrs.push('l');
+    };
+    if s.is_bold {
+        attrs.push('b');
+    };
+    if s.is_dimmed {
+        attrs.push('d');
+    };
+    if s.is_reverse {
+        attrs.push('r');
+    };
+    if s.is_strikethrough {
+        attrs.push('s');
+    };
+    if s.is_underline {
+        attrs.push('u');
+    };
 
     if attrs.is_empty() {
         None
diff --git a/crates/nu-color-config/src/style_computer.rs b/crates/nu-color-config/src/style_computer.rs
index a40fac7894..158f7320df 100644
--- a/crates/nu-color-config/src/style_computer.rs
+++ b/crates/nu-color-config/src/style_computer.rs
@@ -21,13 +21,6 @@ pub enum ComputableStyle {
     Closure(Value),
 }
 
-// macro used for adding initial values to the style hashmap
-macro_rules! initial {
-    ($a:expr, $b:expr) => {
-        ($a.to_string(), ComputableStyle::Static($b))
-    };
-}
-
 // An alias for the mapping used internally by StyleComputer.
 pub type StyleMapping = HashMap<String, ComputableStyle>;
 //
@@ -153,6 +146,12 @@ impl<'a> StyleComputer<'a> {
     pub fn from_config(engine_state: &'a EngineState, stack: &'a Stack) -> StyleComputer<'a> {
         let config = engine_state.get_config();
 
+        macro_rules! initial {
+            ($a:expr, $b:expr) => {
+                ($a.to_string(), ComputableStyle::Static($b))
+            };
+        }
+
         // Create the hashmap
         let mut map: StyleMapping = HashMap::from([
             initial!("separator", Color::White.normal()),
diff --git a/crates/nu-explore/src/commands/help.rs b/crates/nu-explore/src/commands/help.rs
index a8df915a8d..1719f3ec81 100644
--- a/crates/nu-explore/src/commands/help.rs
+++ b/crates/nu-explore/src/commands/help.rs
@@ -187,10 +187,8 @@ fn help_frame_data(
 }
 
 fn help_manual_data(manual: &HelpManual, aliases: &[String]) -> (Vec<String>, Vec<Vec<Value>>) {
-    macro_rules! nu_str {
-        ($text:expr) => {
-            Value::string($text.to_string(), NuSpan::unknown())
-        };
+    fn nu_str(s: &impl ToString) -> Value {
+        Value::string(s.to_string(), NuSpan::unknown())
     }
 
     let arguments = manual
@@ -198,7 +196,7 @@ fn help_manual_data(manual: &HelpManual, aliases: &[String]) -> (Vec<String>, Ve
         .iter()
         .map(|e| Value::Record {
             cols: vec![String::from("example"), String::from("description")],
-            vals: vec![nu_str!(e.example), nu_str!(e.description)],
+            vals: vec![nu_str(&e.example), nu_str(&e.description)],
             span: NuSpan::unknown(),
         })
         .collect();
@@ -213,7 +211,7 @@ fn help_manual_data(manual: &HelpManual, aliases: &[String]) -> (Vec<String>, Ve
         .iter()
         .map(|e| Value::Record {
             cols: vec![String::from("example"), String::from("description")],
-            vals: vec![nu_str!(e.example), nu_str!(e.description)],
+            vals: vec![nu_str(&e.example), nu_str(&e.description)],
             span: NuSpan::unknown(),
         })
         .collect();
@@ -231,7 +229,7 @@ fn help_manual_data(manual: &HelpManual, aliases: &[String]) -> (Vec<String>, Ve
                 String::from("context"),
                 String::from("description"),
             ],
-            vals: vec![nu_str!(e.code), nu_str!(e.context), nu_str!(e.description)],
+            vals: vec![nu_str(&e.code), nu_str(&e.context), nu_str(&e.description)],
             span: NuSpan::unknown(),
         })
         .collect();
@@ -249,7 +247,7 @@ fn help_manual_data(manual: &HelpManual, aliases: &[String]) -> (Vec<String>, Ve
                 .iter()
                 .map(|v| Value::Record {
                     cols: vec![String::from("example"), String::from("description")],
-                    vals: vec![nu_str!(v.example), nu_str!(v.description)],
+                    vals: vec![nu_str(&v.example), nu_str(&v.description)],
                     span: NuSpan::unknown(),
                 })
                 .collect();
@@ -266,9 +264,9 @@ fn help_manual_data(manual: &HelpManual, aliases: &[String]) -> (Vec<String>, Ve
                     String::from("values"),
                 ],
                 vals: vec![
-                    nu_str!(o.group),
-                    nu_str!(o.key),
-                    nu_str!(o.description),
+                    nu_str(&o.group),
+                    nu_str(&o.key),
+                    nu_str(&o.description),
                     values,
                 ],
                 span: NuSpan::unknown(),
@@ -280,9 +278,9 @@ fn help_manual_data(manual: &HelpManual, aliases: &[String]) -> (Vec<String>, Ve
         span: NuSpan::unknown(),
     };
 
-    let name = nu_str!(manual.name);
-    let aliases = nu_str!(aliases.join(", "));
-    let desc = nu_str!(manual.description);
+    let name = nu_str(&manual.name);
+    let aliases = nu_str(&aliases.join(", "));
+    let desc = nu_str(&manual.description);
 
     let headers = vec![
         String::from("name"),
diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs
index 3f20f07900..76ff6073ee 100644
--- a/crates/nu-protocol/src/value/mod.rs
+++ b/crates/nu-protocol/src/value/mod.rs
@@ -735,12 +735,7 @@ impl Value {
         from_user_input: bool,
     ) -> Result<Value, ShellError> {
         let mut current = self;
-        // TODO remove this
-        macro_rules! err_or_null {
-            ($e:expr, $span:expr) => {
-                return Err($e)
-            };
-        }
+
         for member in cell_path {
             // FIXME: this uses a few extra clones for simplicity, but there may be a way
             // to traverse the path without them
@@ -758,18 +753,9 @@ impl Value {
                             } else if *optional {
                                 return Ok(Value::nothing(*origin_span)); // short-circuit
                             } else if val.is_empty() {
-                                err_or_null!(
-                                    ShellError::AccessEmptyContent { span: *origin_span },
-                                    *origin_span
-                                )
+                                return Err(ShellError::AccessEmptyContent { span: *origin_span })
                             } else {
-                                err_or_null!(
-                                    ShellError::AccessBeyondEnd {
-                                        max_idx: val.len() - 1,
-                                        span: *origin_span
-                                    },
-                                    *origin_span
-                                );
+                                return Err(ShellError::AccessBeyondEnd { max_idx:val.len()-1,span: *origin_span });
                             }
                         }
                         Value::Binary { val, .. } => {
@@ -778,18 +764,9 @@ impl Value {
                             } else if *optional {
                                 return Ok(Value::nothing(*origin_span)); // short-circuit
                             } else if val.is_empty() {
-                                err_or_null!(
-                                    ShellError::AccessEmptyContent { span: *origin_span },
-                                    *origin_span
-                                )
+                                return Err(ShellError::AccessEmptyContent { span: *origin_span })
                             } else {
-                                err_or_null!(
-                                    ShellError::AccessBeyondEnd {
-                                        max_idx: val.len() - 1,
-                                        span: *origin_span
-                                    },
-                                    *origin_span
-                                );
+                                return Err(ShellError::AccessBeyondEnd { max_idx:val.len()-1,span: *origin_span });
                             }
                         }
                         Value::Range { val, .. } => {
@@ -798,10 +775,9 @@ impl Value {
                             } else if *optional {
                                 return Ok(Value::nothing(*origin_span)); // short-circuit
                             } else {
-                                err_or_null!(
-                                    ShellError::AccessBeyondEndOfStream { span: *origin_span },
-                                    *origin_span
-                                );
+                                return Err(ShellError::AccessBeyondEndOfStream {
+  span: *origin_span
+});
                             }
                         }
                         Value::CustomValue { val, .. } => {
@@ -823,19 +799,14 @@ impl Value {
                         // Records (and tables) are the only built-in which support column names,
                         // so only use this message for them.
                         Value::Record { .. } => {
-                            err_or_null!(ShellError::TypeMismatch {
-                                err_message: "Can't access record values with a row index. Try specifying a column name instead".into(),
-                                span: *origin_span, }, *origin_span)
+                            return Err(ShellError::TypeMismatch {
+                                err_message:"Can't access record values with a row index. Try specifying a column name instead".into(),
+                                span: *origin_span,
+                            })
                         }
                         Value::Error { error } => return Err(*error.to_owned()),
                         x => {
-                            err_or_null!(
-                                ShellError::IncompatiblePathAccess {
-                                    type_name: format!("{}", x.get_type()),
-                                    span: *origin_span
-                                },
-                                *origin_span
-                            )
+                            return Err(ShellError::IncompatiblePathAccess { type_name:format!("{}",x.get_type()), span: *origin_span })
                         }
                     }
                 }
@@ -862,20 +833,14 @@ impl Value {
                         } else {
                             if from_user_input {
                                 if let Some(suggestion) = did_you_mean(&cols, column_name) {
-                                    err_or_null!(
-                                        ShellError::DidYouMean(suggestion, *origin_span),
-                                        *origin_span
-                                    );
+                                    return Err(ShellError::DidYouMean(suggestion, *origin_span));
                                 }
                             }
-                            err_or_null!(
-                                ShellError::CantFindColumn {
-                                    col_name: column_name.to_string(),
-                                    span: *origin_span,
-                                    src_span: span
-                                },
-                                *origin_span
-                            );
+                            return Err(ShellError::CantFindColumn {
+                                col_name: column_name.to_string(),
+                                span: *origin_span,
+                                src_span: span,
+                            });
                         }
                     }
                     Value::LazyRecord { val, span } => {
@@ -888,20 +853,14 @@ impl Value {
                         } else {
                             if from_user_input {
                                 if let Some(suggestion) = did_you_mean(&columns, column_name) {
-                                    err_or_null!(
-                                        ShellError::DidYouMean(suggestion, *origin_span),
-                                        *origin_span
-                                    );
+                                    return Err(ShellError::DidYouMean(suggestion, *origin_span));
                                 }
                             }
-                            err_or_null!(
-                                ShellError::CantFindColumn {
-                                    col_name: column_name.to_string(),
-                                    span: *origin_span,
-                                    src_span: *span
-                                },
-                                *origin_span
-                            );
+                            return Err(ShellError::CantFindColumn {
+                                col_name: column_name.to_string(),
+                                span: *origin_span,
+                                src_span: *span,
+                            });
                         }
                     }
                     // String access of Lists always means Table access.
@@ -951,15 +910,12 @@ impl Value {
                     Value::Nothing { .. } if *optional => {
                         return Ok(Value::nothing(*origin_span)); // short-circuit
                     }
-                    Value::Error { error } => err_or_null!(*error.to_owned(), *origin_span),
+                    Value::Error { error } => return Err(*error.to_owned()),
                     x => {
-                        err_or_null!(
-                            ShellError::IncompatiblePathAccess {
-                                type_name: format!("{}", x.get_type()),
-                                span: *origin_span
-                            },
-                            *origin_span
-                        )
+                        return Err(ShellError::IncompatiblePathAccess {
+                            type_name: format!("{}", x.get_type()),
+                            span: *origin_span,
+                        })
                     }
                 },
             }