From ba880277bf62fbde5267375da25e3aaebf01b092 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Tue, 26 Dec 2023 17:46:49 +0000 Subject: [PATCH] Remove unnecessary `replace_in_variable` (#11424) # Description `Expression::replace_in_variable` is only called in one place, and it is called with `new_var_id` = `IN_VARIABLE_ID`. So, it ends up doing nothing. E.g., adding `debug_assert_eq!(new_var_id, IN_VARIABLE_ID)` in `replace_in_variable` does not trigger any panic. # User-Facing Changes Breaking change for `nu_protocol`. --- crates/nu-parser/src/parser.rs | 5 +- crates/nu-protocol/src/ast/expression.rs | 198 ----------------------- crates/nu-protocol/src/ast/pipeline.rs | 26 +-- 3 files changed, 2 insertions(+), 227 deletions(-) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 5dd42aeec..79ada981a 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -6086,11 +6086,8 @@ fn wrap_expr_with_collect(working_set: &mut StateWorkingSet, expr: &Expression) default_value: None, }); - let mut expr = expr.clone(); - expr.replace_in_variable(working_set, var_id); - let block = Block { - pipelines: vec![Pipeline::from_vec(vec![expr])], + pipelines: vec![Pipeline::from_vec(vec![expr.clone()])], signature: Box::new(signature), ..Default::default() }; diff --git a/crates/nu-protocol/src/ast/expression.rs b/crates/nu-protocol/src/ast/expression.rs index 8498ff63d..d03306caa 100644 --- a/crates/nu-protocol/src/ast/expression.rs +++ b/crates/nu-protocol/src/ast/expression.rs @@ -301,204 +301,6 @@ impl Expression { } } - pub fn replace_in_variable(&mut self, working_set: &mut StateWorkingSet, new_var_id: VarId) { - match &mut self.expr { - Expr::BinaryOp(left, _, right) => { - left.replace_in_variable(working_set, new_var_id); - right.replace_in_variable(working_set, new_var_id); - } - Expr::UnaryNot(expr) => { - expr.replace_in_variable(working_set, new_var_id); - } - Expr::Block(block_id) => { - let block = working_set.get_block(*block_id); - - let new_expr = if let Some(pipeline) = block.pipelines.first() { - if let Some(element) = pipeline.elements.first() { - let mut new_element = element.clone(); - new_element.replace_in_variable(working_set, new_var_id); - Some(new_element) - } else { - None - } - } else { - None - }; - - let block = working_set.get_block_mut(*block_id); - - if let Some(new_expr) = new_expr { - if let Some(pipeline) = block.pipelines.get_mut(0) { - if let Some(expr) = pipeline.elements.get_mut(0) { - *expr = new_expr - } - } - } - - block.captures = block - .captures - .iter() - .map(|x| if *x != IN_VARIABLE_ID { *x } else { new_var_id }) - .collect(); - } - Expr::Closure(block_id) => { - let block = working_set.get_block(*block_id); - - let new_element = if let Some(pipeline) = block.pipelines.first() { - if let Some(element) = pipeline.elements.first() { - let mut new_element = element.clone(); - new_element.replace_in_variable(working_set, new_var_id); - Some(new_element) - } else { - None - } - } else { - None - }; - - let block = working_set.get_block_mut(*block_id); - - if let Some(new_element) = new_element { - if let Some(pipeline) = block.pipelines.get_mut(0) { - if let Some(element) = pipeline.elements.get_mut(0) { - *element = new_element - } - } - } - - block.captures = block - .captures - .iter() - .map(|x| if *x != IN_VARIABLE_ID { *x } else { new_var_id }) - .collect(); - } - Expr::Binary(_) => {} - Expr::Bool(_) => {} - Expr::Call(call) => { - for positional in call.positional_iter_mut() { - positional.replace_in_variable(working_set, new_var_id); - } - for named in call.named_iter_mut() { - if let Some(expr) = &mut named.2 { - expr.replace_in_variable(working_set, new_var_id) - } - } - } - Expr::CellPath(_) => {} - Expr::DateTime(_) => {} - Expr::ExternalCall(head, args, _) => { - head.replace_in_variable(working_set, new_var_id); - for arg in args { - arg.replace_in_variable(working_set, new_var_id) - } - } - Expr::Filepath(_) => {} - Expr::Directory(_) => {} - Expr::Float(_) => {} - Expr::FullCellPath(full_cell_path) => { - full_cell_path - .head - .replace_in_variable(working_set, new_var_id); - } - Expr::ImportPattern(_) => {} - Expr::Overlay(_) => {} - Expr::Garbage => {} - Expr::Nothing => {} - Expr::GlobPattern(_) => {} - Expr::Int(_) => {} - Expr::MatchBlock(_) => {} - Expr::Keyword(_, _, expr) => expr.replace_in_variable(working_set, new_var_id), - Expr::List(list) => { - for l in list { - l.replace_in_variable(working_set, new_var_id) - } - } - Expr::Operator(_) => {} - Expr::Range(left, middle, right, ..) => { - if let Some(left) = left { - left.replace_in_variable(working_set, new_var_id) - } - if let Some(middle) = middle { - middle.replace_in_variable(working_set, new_var_id) - } - if let Some(right) = right { - right.replace_in_variable(working_set, new_var_id) - } - } - Expr::Record(items) => { - for item in items { - match item { - RecordItem::Pair(field_name, field_value) => { - field_name.replace_in_variable(working_set, new_var_id); - field_value.replace_in_variable(working_set, new_var_id); - } - RecordItem::Spread(_, record) => { - record.replace_in_variable(working_set, new_var_id); - } - } - } - } - Expr::Signature(_) => {} - Expr::String(_) => {} - Expr::StringInterpolation(items) => { - for i in items { - i.replace_in_variable(working_set, new_var_id) - } - } - Expr::RowCondition(block_id) | Expr::Subexpression(block_id) => { - let block = working_set.get_block(*block_id); - - let new_element = if let Some(pipeline) = block.pipelines.first() { - if let Some(element) = pipeline.elements.first() { - let mut new_element = element.clone(); - new_element.replace_in_variable(working_set, new_var_id); - Some(new_element) - } else { - None - } - } else { - None - }; - - let block = working_set.get_block_mut(*block_id); - - if let Some(new_element) = new_element { - if let Some(pipeline) = block.pipelines.get_mut(0) { - if let Some(element) = pipeline.elements.get_mut(0) { - *element = new_element - } - } - } - - block.captures = block - .captures - .iter() - .map(|x| if *x != IN_VARIABLE_ID { *x } else { new_var_id }) - .collect(); - } - Expr::Table(headers, cells) => { - for header in headers { - header.replace_in_variable(working_set, new_var_id) - } - - for row in cells { - for cell in row.iter_mut() { - cell.replace_in_variable(working_set, new_var_id) - } - } - } - - Expr::ValueWithUnit(expr, _) => expr.replace_in_variable(working_set, new_var_id), - Expr::Var(x) => { - if *x == IN_VARIABLE_ID { - *x = new_var_id - } - } - Expr::VarDecl(_) => {} - Expr::Spread(expr) => expr.replace_in_variable(working_set, new_var_id), - } - } - pub fn replace_span( &mut self, working_set: &mut StateWorkingSet, diff --git a/crates/nu-protocol/src/ast/pipeline.rs b/crates/nu-protocol/src/ast/pipeline.rs index 0abb18f10..28843db36 100644 --- a/crates/nu-protocol/src/ast/pipeline.rs +++ b/crates/nu-protocol/src/ast/pipeline.rs @@ -1,4 +1,4 @@ -use crate::{ast::Expression, engine::StateWorkingSet, Span, VarId}; +use crate::{ast::Expression, engine::StateWorkingSet, Span}; use serde::{Deserialize, Serialize}; use std::ops::{Index, IndexMut}; @@ -88,30 +88,6 @@ impl PipelineElement { } } - pub fn replace_in_variable(&mut self, working_set: &mut StateWorkingSet, new_var_id: VarId) { - match self { - PipelineElement::Expression(_, expression) - | PipelineElement::Redirection(_, _, expression, _) - | PipelineElement::And(_, expression) - | PipelineElement::Or(_, expression) - | PipelineElement::SameTargetRedirection { - cmd: (_, expression), - .. - } => expression.replace_in_variable(working_set, new_var_id), - PipelineElement::SeparateRedirection { - out: (_, out_expr, _), - err: (_, err_expr, _), - } => { - if out_expr.has_in_variable(working_set) { - out_expr.replace_in_variable(working_set, new_var_id) - } - if err_expr.has_in_variable(working_set) { - err_expr.replace_in_variable(working_set, new_var_id) - } - } - } - } - pub fn replace_span( &mut self, working_set: &mut StateWorkingSet,