Skip to content

Commit

Permalink
Remove unnecessary replace_in_variable (nushell#11424)
Browse files Browse the repository at this point in the history 
# 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`.
  • Loading branch information
@IanManske
IanManske authored Dec 26, 2023
1 parent 40241e9 commit ba88027
Show file tree
Hide file tree
Showing 3 changed files with 2 additions and 227 deletions.
5 changes: 1 addition & 4 deletions crates/nu-parser/src/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
};
Expand Down
198 changes: 0 additions & 198 deletions crates/nu-protocol/src/ast/expression.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
26 changes: 1 addition & 25 deletions crates/nu-protocol/src/ast/pipeline.rs
View file
Original file line number Diff line number Diff line change
@@ -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};

Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit ba88027

Please sign in to comment.