From f32ecc641f7150704527a3d52005ed3ae24ee7f3 Mon Sep 17 00:00:00 2001
From: Ian Manske <ian.manske@pm.me>
Date: Fri, 3 May 2024 08:35:37 +0000
Subject: [PATCH] Remove some macros (#12742)

# Description
Replaces some macros with regular functions or other code.
---
 crates/nu-cli/src/reedline_config.rs      | 200 ++++++---------------
 crates/nu-cli/src/syntax_highlight.rs     | 205 +++++++++++-----------
 crates/nu-protocol/src/ast/call.rs        |   9 +
 crates/nu-protocol/src/value/filesize.rs  |  41 +++--
 tests/shell/pipeline/commands/internal.rs |  67 ++++---
 5 files changed, 220 insertions(+), 302 deletions(-)

diff --git a/crates/nu-cli/src/reedline_config.rs b/crates/nu-cli/src/reedline_config.rs
index b49dad878cb..3f920a00cb2 100644
--- a/crates/nu-cli/src/reedline_config.rs
+++ b/crates/nu-cli/src/reedline_config.rs
@@ -1,6 +1,7 @@
 use crate::{menus::NuMenuCompleter, NuHelpCompleter};
 use crossterm::event::{KeyCode, KeyModifiers};
 use log::trace;
+use nu_ansi_term::Style;
 use nu_color_config::{color_record_to_nustyle, lookup_ansi_color_style};
 use nu_engine::eval_block;
 use nu_parser::parse;
@@ -158,21 +159,14 @@ fn add_menu(
     }
 }
 
-macro_rules! add_style {
-    // first arm match add!(1,2), add!(2,3) etc
-    ($name:expr, $record: expr, $span:expr, $config: expr, $menu:expr, $f:expr) => {
-        $menu = match extract_value($name, $record, $span) {
-            Ok(text) => {
-                let style = match text {
-                    Value::String { val, .. } => lookup_ansi_color_style(&val),
-                    Value::Record { .. } => color_record_to_nustyle(&text),
-                    _ => lookup_ansi_color_style("green"),
-                };
-                $f($menu, style)
-            }
-            Err(_) => $menu,
-        };
-    };
+fn get_style(record: &Record, name: &str, span: Span) -> Option<Style> {
+    extract_value(name, record, span)
+        .ok()
+        .map(|text| match text {
+            Value::String { val, .. } => lookup_ansi_color_style(val),
+            Value::Record { .. } => color_record_to_nustyle(text),
+            _ => lookup_ansi_color_style("green"),
+        })
 }
 
 // Adds a columnar menu to the editor engine
@@ -215,46 +209,21 @@ pub(crate) fn add_columnar_menu(
 
     let span = menu.style.span();
     if let Value::Record { val, .. } = &menu.style {
-        add_style!(
-            "text",
-            val,
-            span,
-            config,
-            columnar_menu,
-            ColumnarMenu::with_text_style
-        );
-        add_style!(
-            "selected_text",
-            val,
-            span,
-            config,
-            columnar_menu,
-            ColumnarMenu::with_selected_text_style
-        );
-        add_style!(
-            "description_text",
-            val,
-            span,
-            config,
-            columnar_menu,
-            ColumnarMenu::with_description_text_style
-        );
-        add_style!(
-            "match_text",
-            val,
-            span,
-            config,
-            columnar_menu,
-            ColumnarMenu::with_match_text_style
-        );
-        add_style!(
-            "selected_match_text",
-            val,
-            span,
-            config,
-            columnar_menu,
-            ColumnarMenu::with_selected_match_text_style
-        );
+        if let Some(style) = get_style(val, "text", span) {
+            columnar_menu = columnar_menu.with_text_style(style);
+        }
+        if let Some(style) = get_style(val, "selected_text", span) {
+            columnar_menu = columnar_menu.with_selected_text_style(style);
+        }
+        if let Some(style) = get_style(val, "description_text", span) {
+            columnar_menu = columnar_menu.with_description_text_style(style);
+        }
+        if let Some(style) = get_style(val, "match_text", span) {
+            columnar_menu = columnar_menu.with_match_text_style(style);
+        }
+        if let Some(style) = get_style(val, "selected_match_text", span) {
+            columnar_menu = columnar_menu.with_selected_match_text_style(style);
+        }
     }
 
     let marker = menu.marker.to_expanded_string("", config);
@@ -313,30 +282,15 @@ pub(crate) fn add_list_menu(
 
     let span = menu.style.span();
     if let Value::Record { val, .. } = &menu.style {
-        add_style!(
-            "text",
-            val,
-            span,
-            config,
-            list_menu,
-            ListMenu::with_text_style
-        );
-        add_style!(
-            "selected_text",
-            val,
-            span,
-            config,
-            list_menu,
-            ListMenu::with_selected_text_style
-        );
-        add_style!(
-            "description_text",
-            val,
-            span,
-            config,
-            list_menu,
-            ListMenu::with_description_text_style
-        );
+        if let Some(style) = get_style(val, "text", span) {
+            list_menu = list_menu.with_text_style(style);
+        }
+        if let Some(style) = get_style(val, "selected_text", span) {
+            list_menu = list_menu.with_selected_text_style(style);
+        }
+        if let Some(style) = get_style(val, "description_text", span) {
+            list_menu = list_menu.with_description_text_style(style);
+        }
     }
 
     let marker = menu.marker.to_expanded_string("", config);
@@ -520,46 +474,21 @@ pub(crate) fn add_ide_menu(
 
     let span = menu.style.span();
     if let Value::Record { val, .. } = &menu.style {
-        add_style!(
-            "text",
-            val,
-            span,
-            config,
-            ide_menu,
-            IdeMenu::with_text_style
-        );
-        add_style!(
-            "selected_text",
-            val,
-            span,
-            config,
-            ide_menu,
-            IdeMenu::with_selected_text_style
-        );
-        add_style!(
-            "description_text",
-            val,
-            span,
-            config,
-            ide_menu,
-            IdeMenu::with_description_text_style
-        );
-        add_style!(
-            "match_text",
-            val,
-            span,
-            config,
-            ide_menu,
-            IdeMenu::with_match_text_style
-        );
-        add_style!(
-            "selected_match_text",
-            val,
-            span,
-            config,
-            ide_menu,
-            IdeMenu::with_selected_match_text_style
-        );
+        if let Some(style) = get_style(val, "text", span) {
+            ide_menu = ide_menu.with_text_style(style);
+        }
+        if let Some(style) = get_style(val, "selected_text", span) {
+            ide_menu = ide_menu.with_selected_text_style(style);
+        }
+        if let Some(style) = get_style(val, "description_text", span) {
+            ide_menu = ide_menu.with_description_text_style(style);
+        }
+        if let Some(style) = get_style(val, "match_text", span) {
+            ide_menu = ide_menu.with_match_text_style(style);
+        }
+        if let Some(style) = get_style(val, "selected_match_text", span) {
+            ide_menu = ide_menu.with_selected_match_text_style(style);
+        }
     }
 
     let marker = menu.marker.to_expanded_string("", config);
@@ -650,30 +579,15 @@ pub(crate) fn add_description_menu(
 
     let span = menu.style.span();
     if let Value::Record { val, .. } = &menu.style {
-        add_style!(
-            "text",
-            val,
-            span,
-            config,
-            description_menu,
-            DescriptionMenu::with_text_style
-        );
-        add_style!(
-            "selected_text",
-            val,
-            span,
-            config,
-            description_menu,
-            DescriptionMenu::with_selected_text_style
-        );
-        add_style!(
-            "description_text",
-            val,
-            span,
-            config,
-            description_menu,
-            DescriptionMenu::with_description_text_style
-        );
+        if let Some(style) = get_style(val, "text", span) {
+            description_menu = description_menu.with_text_style(style);
+        }
+        if let Some(style) = get_style(val, "selected_text", span) {
+            description_menu = description_menu.with_selected_text_style(style);
+        }
+        if let Some(style) = get_style(val, "description_text", span) {
+            description_menu = description_menu.with_description_text_style(style);
+        }
     }
 
     let marker = menu.marker.to_expanded_string("", config);
diff --git a/crates/nu-cli/src/syntax_highlight.rs b/crates/nu-cli/src/syntax_highlight.rs
index da293f03ac7..6d890ad295c 100644
--- a/crates/nu-cli/src/syntax_highlight.rs
+++ b/crates/nu-cli/src/syntax_highlight.rs
@@ -4,7 +4,7 @@ use nu_color_config::{get_matching_brackets_style, get_shape_color};
 use nu_engine::env;
 use nu_parser::{flatten_block, parse, FlatShape};
 use nu_protocol::{
-    ast::{Argument, Block, Expr, Expression, PipelineRedirection, RecordItem},
+    ast::{Block, Expr, Expression, PipelineRedirection, RecordItem},
     engine::{EngineState, Stack, StateWorkingSet},
     Config, Span,
 };
@@ -86,27 +86,6 @@ impl Highlighter for NuHighlighter {
                 [(shape.0.start - global_span_offset)..(shape.0.end - global_span_offset)]
                 .to_string();
 
-            macro_rules! add_colored_token_with_bracket_highlight {
-                ($shape:expr, $span:expr, $text:expr) => {{
-                    let spans = split_span_by_highlight_positions(
-                        line,
-                        $span,
-                        &matching_brackets_pos,
-                        global_span_offset,
-                    );
-                    spans.iter().for_each(|(part, highlight)| {
-                        let start = part.start - $span.start;
-                        let end = part.end - $span.start;
-                        let text = (&next_token[start..end]).to_string();
-                        let mut style = get_shape_color($shape.to_string(), &self.config);
-                        if *highlight {
-                            style = get_matching_brackets_style(style, &self.config);
-                        }
-                        output.push((style, text));
-                    });
-                }};
-            }
-
             let mut add_colored_token = |shape: &FlatShape, text: String| {
                 output.push((get_shape_color(shape.to_string(), &self.config), text));
             };
@@ -131,21 +110,29 @@ impl Highlighter for NuHighlighter {
                 FlatShape::RawString => 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)
-                }
-                FlatShape::Table => {
-                    add_colored_token_with_bracket_highlight!(shape.1, shape.0, next_token)
-                }
-                FlatShape::Record => {
-                    add_colored_token_with_bracket_highlight!(shape.1, shape.0, next_token)
-                }
-
-                FlatShape::Block => {
-                    add_colored_token_with_bracket_highlight!(shape.1, shape.0, next_token)
-                }
-                FlatShape::Closure => {
-                    add_colored_token_with_bracket_highlight!(shape.1, shape.0, next_token)
+                FlatShape::List
+                | FlatShape::Table
+                | FlatShape::Record
+                | FlatShape::Block
+                | FlatShape::Closure => {
+                    let span = shape.0;
+                    let shape = &shape.1;
+                    let spans = split_span_by_highlight_positions(
+                        line,
+                        span,
+                        &matching_brackets_pos,
+                        global_span_offset,
+                    );
+                    for (part, highlight) in spans {
+                        let start = part.start - span.start;
+                        let end = part.end - span.start;
+                        let text = next_token[start..end].to_string();
+                        let mut style = get_shape_color(shape.to_string(), &self.config);
+                        if highlight {
+                            style = get_matching_brackets_style(style, &self.config);
+                        }
+                        output.push((style, text));
+                    }
                 }
 
                 FlatShape::Filepath => add_colored_token(&shape.1, next_token),
@@ -311,20 +298,6 @@ fn find_matching_block_end_in_expr(
     global_span_offset: usize,
     global_cursor_offset: usize,
 ) -> Option<usize> {
-    macro_rules! find_in_expr_or_continue {
-        ($inner_expr:ident) => {
-            if let Some(pos) = find_matching_block_end_in_expr(
-                line,
-                working_set,
-                $inner_expr,
-                global_span_offset,
-                global_cursor_offset,
-            ) {
-                return Some(pos);
-            }
-        };
-    }
-
     if expression.span.contains(global_cursor_offset) && expression.span.start >= global_span_offset
     {
         let expr_first = expression.span.start;
@@ -372,15 +345,19 @@ fn find_matching_block_end_in_expr(
                     Some(expr_last)
                 } else {
                     // cursor is inside table
-                    for inner_expr in table.columns.as_ref() {
-                        find_in_expr_or_continue!(inner_expr);
-                    }
-                    for row in table.rows.as_ref() {
-                        for inner_expr in row.as_ref() {
-                            find_in_expr_or_continue!(inner_expr);
-                        }
-                    }
-                    None
+                    table
+                        .columns
+                        .iter()
+                        .chain(table.rows.iter().flat_map(AsRef::as_ref))
+                        .find_map(|expr| {
+                            find_matching_block_end_in_expr(
+                                line,
+                                working_set,
+                                expr,
+                                global_span_offset,
+                                global_cursor_offset,
+                            )
+                        })
                 }
             }
 
@@ -393,36 +370,45 @@ fn find_matching_block_end_in_expr(
                     Some(expr_last)
                 } else {
                     // cursor is inside record
-                    for expr in exprs {
-                        match expr {
-                            RecordItem::Pair(k, v) => {
-                                find_in_expr_or_continue!(k);
-                                find_in_expr_or_continue!(v);
-                            }
-                            RecordItem::Spread(_, record) => {
-                                find_in_expr_or_continue!(record);
-                            }
-                        }
-                    }
-                    None
+                    exprs.iter().find_map(|expr| match expr {
+                        RecordItem::Pair(k, v) => find_matching_block_end_in_expr(
+                            line,
+                            working_set,
+                            k,
+                            global_span_offset,
+                            global_cursor_offset,
+                        )
+                        .or_else(|| {
+                            find_matching_block_end_in_expr(
+                                line,
+                                working_set,
+                                v,
+                                global_span_offset,
+                                global_cursor_offset,
+                            )
+                        }),
+                        RecordItem::Spread(_, record) => find_matching_block_end_in_expr(
+                            line,
+                            working_set,
+                            record,
+                            global_span_offset,
+                            global_cursor_offset,
+                        ),
+                    })
                 }
             }
 
-            Expr::Call(call) => {
-                for arg in &call.arguments {
-                    let opt_expr = match arg {
-                        Argument::Named((_, _, opt_expr)) => opt_expr.as_ref(),
-                        Argument::Positional(inner_expr) => Some(inner_expr),
-                        Argument::Unknown(inner_expr) => Some(inner_expr),
-                        Argument::Spread(inner_expr) => Some(inner_expr),
-                    };
-
-                    if let Some(inner_expr) = opt_expr {
-                        find_in_expr_or_continue!(inner_expr);
-                    }
-                }
-                None
-            }
+            Expr::Call(call) => call.arguments.iter().find_map(|arg| {
+                arg.expr().and_then(|expr| {
+                    find_matching_block_end_in_expr(
+                        line,
+                        working_set,
+                        expr,
+                        global_span_offset,
+                        global_cursor_offset,
+                    )
+                })
+            }),
 
             Expr::FullCellPath(b) => find_matching_block_end_in_expr(
                 line,
@@ -432,12 +418,15 @@ fn find_matching_block_end_in_expr(
                 global_cursor_offset,
             ),
 
-            Expr::BinaryOp(lhs, op, rhs) => {
-                find_in_expr_or_continue!(lhs);
-                find_in_expr_or_continue!(op);
-                find_in_expr_or_continue!(rhs);
-                None
-            }
+            Expr::BinaryOp(lhs, op, rhs) => [lhs, op, rhs].into_iter().find_map(|expr| {
+                find_matching_block_end_in_expr(
+                    line,
+                    working_set,
+                    expr,
+                    global_span_offset,
+                    global_cursor_offset,
+                )
+            }),
 
             Expr::Block(block_id)
             | Expr::Closure(block_id)
@@ -462,12 +451,15 @@ fn find_matching_block_end_in_expr(
                 }
             }
 
-            Expr::StringInterpolation(inner_expr) => {
-                for inner_expr in inner_expr {
-                    find_in_expr_or_continue!(inner_expr);
-                }
-                None
-            }
+            Expr::StringInterpolation(exprs) => exprs.iter().find_map(|expr| {
+                find_matching_block_end_in_expr(
+                    line,
+                    working_set,
+                    expr,
+                    global_span_offset,
+                    global_cursor_offset,
+                )
+            }),
 
             Expr::List(list) => {
                 if expr_last == global_cursor_offset {
@@ -477,12 +469,15 @@ fn find_matching_block_end_in_expr(
                     // cursor is at list start
                     Some(expr_last)
                 } else {
-                    // cursor is inside list
-                    for item in list {
-                        let expr = item.expr();
-                        find_in_expr_or_continue!(expr);
-                    }
-                    None
+                    list.iter().find_map(|item| {
+                        find_matching_block_end_in_expr(
+                            line,
+                            working_set,
+                            item.expr(),
+                            global_span_offset,
+                            global_cursor_offset,
+                        )
+                    })
                 }
             }
         };
diff --git a/crates/nu-protocol/src/ast/call.rs b/crates/nu-protocol/src/ast/call.rs
index 0dc1a70c53f..6689bc3a555 100644
--- a/crates/nu-protocol/src/ast/call.rs
+++ b/crates/nu-protocol/src/ast/call.rs
@@ -36,6 +36,15 @@ impl Argument {
             Argument::Spread(e) => e.span,
         }
     }
+
+    pub fn expr(&self) -> Option<&Expression> {
+        match self {
+            Argument::Named((_, _, expr)) => expr.as_ref(),
+            Argument::Positional(expr) | Argument::Unknown(expr) | Argument::Spread(expr) => {
+                Some(expr)
+            }
+        }
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
diff --git a/crates/nu-protocol/src/value/filesize.rs b/crates/nu-protocol/src/value/filesize.rs
index dfbe97ad9c6..79ecc136b91 100644
--- a/crates/nu-protocol/src/value/filesize.rs
+++ b/crates/nu-protocol/src/value/filesize.rs
@@ -72,24 +72,29 @@ fn get_filesize_format(
 ) -> Option<byte_unit::Unit> {
     // filesize_metric always overrides the unit of filesize_format.
     let metric = filesize_metric.unwrap_or(!format_value.ends_with("ib"));
-    macro_rules! either {
-        ($metric:ident, $binary:ident) => {
-            Some(if metric {
-                byte_unit::Unit::$metric
-            } else {
-                byte_unit::Unit::$binary
-            })
-        };
-    }
-    match format_value {
-        "b" => Some(byte_unit::Unit::B),
-        "kb" | "kib" => either!(KB, KiB),
-        "mb" | "mib" => either!(MB, MiB),
-        "gb" | "gib" => either!(GB, GiB),
-        "tb" | "tib" => either!(TB, TiB),
-        "pb" | "pib" => either!(TB, TiB),
-        "eb" | "eib" => either!(EB, EiB),
-        _ => None,
+
+    if metric {
+        match format_value {
+            "b" => Some(byte_unit::Unit::B),
+            "kb" | "kib" => Some(byte_unit::Unit::KB),
+            "mb" | "mib" => Some(byte_unit::Unit::MB),
+            "gb" | "gib" => Some(byte_unit::Unit::GB),
+            "tb" | "tib" => Some(byte_unit::Unit::TB),
+            "pb" | "pib" => Some(byte_unit::Unit::TB),
+            "eb" | "eib" => Some(byte_unit::Unit::EB),
+            _ => None,
+        }
+    } else {
+        match format_value {
+            "b" => Some(byte_unit::Unit::B),
+            "kb" | "kib" => Some(byte_unit::Unit::KiB),
+            "mb" | "mib" => Some(byte_unit::Unit::MiB),
+            "gb" | "gib" => Some(byte_unit::Unit::GiB),
+            "tb" | "tib" => Some(byte_unit::Unit::TiB),
+            "pb" | "pib" => Some(byte_unit::Unit::TiB),
+            "eb" | "eib" => Some(byte_unit::Unit::EiB),
+            _ => None,
+        }
     }
 }
 
diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs
index dbc4b0a3188..06163639d71 100644
--- a/tests/shell/pipeline/commands/internal.rs
+++ b/tests/shell/pipeline/commands/internal.rs
@@ -1065,63 +1065,58 @@ mod tilde_expansion {
 mod variable_scoping {
     use nu_test_support::nu;
 
-    macro_rules! test_variable_scope {
-        ($func:literal == $res:literal $(,)*) => {
-            let actual = nu!($func);
-
-            assert_eq!(actual.out, $res);
-        };
+    fn test_variable_scope(code: &str, expected: &str) {
+        let actual = nu!(code);
+        assert_eq!(actual.out, expected);
     }
-    macro_rules! test_variable_scope_list {
-        ($func:literal == $res:expr $(,)*) => {
-            let actual = nu!($func);
 
-            let result: Vec<&str> = actual.out.matches("ZZZ").collect();
-            assert_eq!(result, $res);
-        };
+    fn test_variable_scope_list(code: &str, expected: &[&str]) {
+        let actual = nu!(code);
+        let result: Vec<&str> = actual.out.matches("ZZZ").collect();
+        assert_eq!(result, expected);
     }
 
     #[test]
     fn access_variables_in_scopes() {
-        test_variable_scope!(
+        test_variable_scope(
             " def test [input] { echo [0 1 2] | do { do { echo $input } } }
-                test ZZZ "
-                == "ZZZ"
+                test ZZZ ",
+            "ZZZ",
         );
-        test_variable_scope!(
+        test_variable_scope(
             r#" def test [input] { echo [0 1 2] | do { do { if $input == "ZZZ" { echo $input } else { echo $input } } } }
-                test ZZZ "#
-                == "ZZZ"
+                test ZZZ "#,
+            "ZZZ",
         );
-        test_variable_scope!(
+        test_variable_scope(
             r#" def test [input] { echo [0 1 2] | do { do { if $input == "ZZZ" { echo $input } else { echo $input } } } }
-                test ZZZ "#
-                == "ZZZ"
+                test ZZZ "#,
+            "ZZZ",
         );
-        test_variable_scope!(
+        test_variable_scope(
             " def test [input] { echo [0 1 2] | do { echo $input } }
-                test ZZZ "
-                == "ZZZ"
+                test ZZZ ",
+            "ZZZ",
         );
-        test_variable_scope!(
+        test_variable_scope(
             " def test [input] { echo [0 1 2] | do { if $input == $input { echo $input } else { echo $input } } }
-                test ZZZ "
-                == "ZZZ"
+                test ZZZ ",
+                "ZZZ"
         );
-        test_variable_scope_list!(
+        test_variable_scope_list(
             " def test [input] { echo [0 1 2] | each { |_| echo $input } }
-                test ZZZ "
-                == ["ZZZ", "ZZZ", "ZZZ"]
+                test ZZZ ",
+            &["ZZZ", "ZZZ", "ZZZ"],
         );
-        test_variable_scope_list!(
+        test_variable_scope_list(
             " def test [input] { echo [0 1 2] | each { |it| if $it > 0 {echo $input} else {echo $input}} }
-                test ZZZ "
-                == ["ZZZ", "ZZZ", "ZZZ"]
+                test ZZZ ",
+            &["ZZZ", "ZZZ", "ZZZ"],
         );
-        test_variable_scope_list!(
+        test_variable_scope_list(
             " def test [input] { echo [0 1 2] | each { |_| if $input == $input {echo $input} else {echo $input}} }
-                test ZZZ "
-                == ["ZZZ", "ZZZ", "ZZZ"]
+                test ZZZ ",
+            &["ZZZ", "ZZZ", "ZZZ"],
         );
     }
 }