extern/nushell
1
0
Fork 1
mirror of https://github.com/nushell/nushell.git synced 2024-11-22 00:13:21 +01:00
Code Issues Packages Projects Releases Wiki Activity

Revert "Pipeline operators: && and ||" (#7452)

Browse Source 
Reverts nushell/nushell#7448

Some surprising behavior in how we do this. For example:

```
〉if (true || false) { print "yes!" } else { print "no!" }
no!
〉if (true or false) { print "yes!" } else { print "no!" }
yes!
```

This means for folks who are using the old `||`, they possibly get the
wrong answer once they upgrade. I don't think we can ship with that as
it will catch too many people by surprise and just make it easier to
write buggy code.
This commit is contained in:
JT 2022-12-13 16:36:13 +13:00 committed by GitHub
parent b7a3e5989d
commit 0c656fd276
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 180 additions and 321 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
crates/nu-cli/src/completions/completer.rs
View File

@ -127,6 +127,7 @@ impl NuCompleter {
match pipeline_element {
PipelineElement::Expression(_, expr)
| PipelineElement::Redirection(_, _, expr)
| PipelineElement::And(_, expr)
| PipelineElement::Or(_, expr) => {
let flattened: Vec<_> = flatten_expression(&working_set, &expr);
let span_offset: usize = alias_offset.iter().sum();

1
crates/nu-cli/src/syntax_highlight.rs
View File

@ -232,6 +232,7 @@ fn find_matching_block_end_in_block(
match e {
PipelineElement::Expression(_, e)
| PipelineElement::Redirection(_, _, e)
| PipelineElement::And(_, e)
| PipelineElement::Or(_, e) => {
if e.span.contains(global_cursor_offset) {
if let Some(pos) = find_matching_block_end_in_expr(

1
crates/nu-command/src/formats/from/nuon.rs
View File

@ -146,6 +146,7 @@ impl Command for FromNuon {
match pipeline.elements.remove(0) {
PipelineElement::Expression(_, expression)
| PipelineElement::Redirection(_, _, expression)
| PipelineElement::And(_, expression)
| PipelineElement::Or(_, expression) => expression,
}
}

210
crates/nu-engine/src/eval.rs
View File

@ -789,16 +789,14 @@ pub fn eval_element_with_input(
redirect_stderr: bool,
) -> Result<(PipelineData, bool), ShellError> {
match element {
PipelineElement::Expression(_, expr) | PipelineElement::Or(_, expr) => {
eval_expression_with_input(
engine_state,
stack,
expr,
input,
redirect_stdout,
redirect_stderr,
)
}
PipelineElement::Expression(_, expr) => eval_expression_with_input(
engine_state,
stack,
expr,
input,
redirect_stdout,
redirect_stderr,
),
PipelineElement::Redirection(span, redirection, expr) => match &expr.expr {
Expr::String(_) => {
let input = match (redirection, input) {
@ -905,6 +903,22 @@ pub fn eval_element_with_input(
}
_ => Err(ShellError::CommandNotFound(*span)),
},
PipelineElement::And(_, expr) => eval_expression_with_input(
engine_state,
stack,
expr,
input,
redirect_stdout,
redirect_stderr,
),
PipelineElement::Or(_, expr) => eval_expression_with_input(
engine_state,
stack,
expr,
input,
redirect_stdout,
redirect_stderr,
),
}
}
@ -938,16 +952,10 @@ pub fn eval_block(
redirect_stderr: bool,
) -> Result<PipelineData, ShellError> {
let num_pipelines = block.len();
let mut pipeline_idx = 0;
while pipeline_idx < block.pipelines.len() {
let pipeline = &block.pipelines[pipeline_idx];
for (pipeline_idx, pipeline) in block.pipelines.iter().enumerate() {
let mut i = 0;
let mut last_element_is_pipeline_or = false;
while i < pipeline.elements.len() && !last_element_is_pipeline_or {
while i < pipeline.elements.len() {
let redirect_stderr = redirect_stderr
|| ((i < pipeline.elements.len() - 1)
&& (matches!(
@ -956,8 +964,6 @@ pub fn eval_block(
| PipelineElement::Redirection(_, Redirection::StdoutAndStderr, _)
)));
last_element_is_pipeline_or = matches!(&pipeline.elements[i], PipelineElement::Or(..));
// if eval internal command failed, it can just make early return with `Err(ShellError)`.
let eval_result = eval_element_with_input(
engine_state,
@ -984,16 +990,10 @@ pub fn eval_block(
// don't return `Err(ShellError)`, so nushell wouldn't show extra error message.
}
(Err(error), true) => input = PipelineData::Value(Value::Error { error }, None),
(output, false) if last_element_is_pipeline_or => match output {
Ok(output) => {
input = output.0;
}
Err(error) => input = PipelineData::Value(Value::Error { error }, None),
},
(output, false) => {
let output = output?;
input = output.0;
// external command may have failed
// external command may runs to failed
// make early return so remaining commands will not be executed.
// don't return `Err(ShellError)`, so nushell wouldn't show extra error message.
if output.1 {
@ -1006,109 +1006,83 @@ pub fn eval_block(
}
if pipeline_idx < (num_pipelines) - 1 {
if last_element_is_pipeline_or {
let input_is_error = matches!(input, PipelineData::Value(Value::Error { .. }, ..));
match input {
PipelineData::Value(Value::Nothing { .. }, ..) => {}
PipelineData::ExternalStream {
ref mut exit_code, ..
} => {
let exit_code = exit_code.take();
let result =
drain_and_print(engine_state, stack, input, redirect_stdout, redirect_stderr);
// Drain the input to the screen via tabular output
let config = engine_state.get_config();
let last_exit_code = stack.last_exit_code(engine_state).unwrap_or(0);
match engine_state.find_decl("table".as_bytes(), &[]) {
Some(decl_id) => {
let table = engine_state.get_decl(decl_id).run(
engine_state,
stack,
&Call::new(Span::new(0, 0)),
input,
)?;
if last_exit_code == 0 && result.is_ok() && !input_is_error {
// Skip the next pipeline ot run because this pipeline was successful and the
// user used the `a || b` connector
pipeline_idx += 1;
print_or_return(table, config)?;
}
None => {
print_or_return(input, config)?;
}
};
if let Some(exit_code) = exit_code {
let mut v: Vec<_> = exit_code.collect();
if let Some(v) = v.pop() {
stack.add_env_var("LAST_EXIT_CODE".into(), v);
}
}
}
input = PipelineData::empty()
} else {
drain_and_print(engine_state, stack, input, redirect_stdout, redirect_stderr)?;
_ => {
// Drain the input to the screen via tabular output
let config = engine_state.get_config();
input = PipelineData::empty()
match engine_state.find_decl("table".as_bytes(), &[]) {
Some(decl_id) => {
let table = engine_state.get_decl(decl_id);
if let Some(block_id) = table.get_block_id() {
let block = engine_state.get_block(block_id);
eval_block(
engine_state,
stack,
block,
input,
redirect_stdout,
redirect_stderr,
)?;
} else {
let table = table.run(
engine_state,
stack,
&Call::new(Span::new(0, 0)),
input,
)?;
print_or_return(table, config)?;
}
}
None => {
print_or_return(input, config)?;
}
};
}
}
}
pipeline_idx += 1;
input = PipelineData::empty()
}
}
Ok(input)
}
fn drain_and_print(
engine_state: &EngineState,
stack: &mut Stack,
mut input: PipelineData,
redirect_stdout: bool,
redirect_stderr: bool,
) -> Result<(), ShellError> {
match input {
PipelineData::Value(Value::Nothing { .. }, ..) => {}
PipelineData::ExternalStream {
ref mut exit_code, ..
} => {
let exit_code = exit_code.take();
// Drain the input to the screen via tabular output
let config = engine_state.get_config();
match engine_state.find_decl("table".as_bytes(), &[]) {
Some(decl_id) => {
let table = engine_state.get_decl(decl_id).run(
engine_state,
stack,
&Call::new(Span::new(0, 0)),
input,
)?;
print_or_return(table, config)?;
}
None => {
print_or_return(input, config)?;
}
};
if let Some(exit_code) = exit_code {
let mut v: Vec<_> = exit_code.collect();
if let Some(v) = v.pop() {
stack.add_env_var("LAST_EXIT_CODE".into(), v);
}
}
}
_ => {
// Drain the input to the screen via tabular output
let config = engine_state.get_config();
match engine_state.find_decl("table".as_bytes(), &[]) {
Some(decl_id) => {
let table = engine_state.get_decl(decl_id);
if let Some(block_id) = table.get_block_id() {
let block = engine_state.get_block(block_id);
eval_block(
engine_state,
stack,
block,
input,
redirect_stdout,
redirect_stderr,
)?;
} else {
let table =
table.run(engine_state, stack, &Call::new(Span::new(0, 0)), input)?;
print_or_return(table, config)?;
}
}
None => {
print_or_return(input, config)?;
}
};
}
}
Ok(())
}
fn print_or_return(pipeline_data: PipelineData, config: &Config) -> Result<(), ShellError> {
for item in pipeline_data {
if let Value::Error { error } = item {

10
crates/nu-parser/src/flatten.rs
View File

@ -455,10 +455,14 @@ pub fn flatten_pipeline_element(
output.append(&mut flatten_expression(working_set, expr));
output
}
PipelineElement::Or(span, expr) => {
let mut output = vec![];
PipelineElement::And(span, expr) => {
let mut output = vec![(*span, FlatShape::And)];
output.append(&mut flatten_expression(working_set, expr));
output
}
PipelineElement::Or(span, expr) => {
let mut output = vec![(*span, FlatShape::Or)];
output.append(&mut flatten_expression(working_set, expr));
output.push((*span, FlatShape::Or));
output
}
}

5
crates/nu-parser/src/lex.rs
View File

@ -7,7 +7,6 @@ pub enum TokenContents {
Comment,
Pipe,
PipePipe,
AndAnd,
Semicolon,
OutGreaterThan,
ErrGreaterThan,
@ -255,10 +254,10 @@ pub fn lex_item(
),
b"&&" => (
Token {
contents: TokenContents::AndAnd,
contents: TokenContents::Item,
span,
},
None,
Some(ParseError::ShellAndAnd(span)),
),
b"2>" => (
Token {

4
crates/nu-parser/src/parse_keywords.rs
View File

@ -1420,9 +1420,7 @@ pub fn parse_module_block(
pipeline
}
LiteElement::Redirection(_, _, command) | LiteElement::Or(_, command) => {
garbage_pipeline(&command.parts)
}
LiteElement::Redirection(_, _, command) => garbage_pipeline(&command.parts),
}
} else {
error = Some(ParseError::Expected("not a pipeline".into(), span));

170
crates/nu-parser/src/parser.rs
View File

@ -1223,7 +1223,6 @@ fn parse_binary_with_base(
}
TokenContents::Pipe
| TokenContents::PipePipe
| TokenContents::AndAnd
| TokenContents::OutGreaterThan
| TokenContents::ErrGreaterThan
| TokenContents::OutErrGreaterThan => {
@ -3805,9 +3804,7 @@ pub fn parse_table_expression(
}
_ => {
match &output.block[0].commands[0] {
LiteElement::Command(_, command)
| LiteElement::Redirection(_, _, command)
| LiteElement::Or(_, command) => {
LiteElement::Command(_, command) | LiteElement::Redirection(_, _, command) => {
let mut table_headers = vec![];
let (headers, err) = parse_value(
@ -3828,8 +3825,7 @@ pub fn parse_table_expression(
match &output.block[1].commands[0] {
LiteElement::Command(_, command)
| LiteElement::Redirection(_, _, command)
| LiteElement::Or(_, command) => {
| LiteElement::Redirection(_, _, command) => {
let mut rows = vec![];
for part in &command.parts {
let (values, err) = parse_value(
@ -5286,9 +5282,7 @@ pub fn parse_block(
for pipeline in &lite_block.block {
if pipeline.commands.len() == 1 {
match &pipeline.commands[0] {
LiteElement::Command(_, command)
| LiteElement::Redirection(_, _, command)
| LiteElement::Or(_, command) => {
LiteElement::Command(_, command) | LiteElement::Redirection(_, _, command) => {
if let Some(err) =
parse_def_predecl(working_set, &command.parts, expand_aliases_denylist)
{
@ -5325,21 +5319,6 @@ pub fn parse_block(
PipelineElement::Expression(*span, expr)
}
LiteElement::Or(span, command) => {
let (expr, err) = parse_expression(
working_set,
&command.parts,
expand_aliases_denylist,
is_subexpression,
);
working_set.type_scope.add_type(expr.ty.clone());
if error.is_none() {
error = err;
}
PipelineElement::Or(*span, expr)
}
LiteElement::Redirection(span, redirection, command) => {
trace!("parsing: pipeline element: redirection");
let (expr, err) = parse_string(
@ -5375,16 +5354,8 @@ pub fn parse_block(
Pipeline { elements: output }
} else {
let (mut pipeline, err) = match &pipeline.commands[0] {
match &pipeline.commands[0] {
LiteElement::Command(_, command) | LiteElement::Redirection(_, _, command) => {
parse_builtin_commands(
working_set,
command,
expand_aliases_denylist,
is_subexpression,
)
}
LiteElement::Or(span, command) => {
let (mut pipeline, err) = parse_builtin_commands(
working_set,
command,
@ -5392,63 +5363,61 @@ pub fn parse_block(
is_subexpression,
);
if let PipelineElement::Expression(_, expr) = &pipeline.elements[0] {
pipeline.elements[0] = PipelineElement::Or(*span, expr.clone())
}
(pipeline, err)
}
};
if idx == 0 {
if let Some(let_decl_id) = working_set.find_decl(b"let", &Type::Any) {
if let Some(let_env_decl_id) = working_set.find_decl(b"let-env", &Type::Any)
{
for element in pipeline.elements.iter_mut() {
if let PipelineElement::Expression(
_,
Expression {
expr: Expr::Call(call),
..
},
) = element
if idx == 0 {
if let Some(let_decl_id) = working_set.find_decl(b"let", &Type::Any) {
if let Some(let_env_decl_id) =
working_set.find_decl(b"let-env", &Type::Any)
{
if call.decl_id == let_decl_id
|| call.decl_id == let_env_decl_id
{
// Do an expansion
if let Some(Expression {
expr: Expr::Keyword(_, _, expr),
..
}) = call.positional_iter_mut().nth(1)
for element in pipeline.elements.iter_mut() {
if let PipelineElement::Expression(
_,
Expression {
expr: Expr::Call(call),
..
},
) = element
{
if expr.has_in_variable(working_set) {
*expr = Box::new(wrap_expr_with_collect(
working_set,
expr,
));
if call.decl_id == let_decl_id
|| call.decl_id == let_env_decl_id
{
// Do an expansion
if let Some(Expression {
expr: Expr::Keyword(_, _, expr),
..
}) = call.positional_iter_mut().nth(1)
{
if expr.has_in_variable(working_set) {
*expr = Box::new(wrap_expr_with_collect(
working_set,
expr,
));
}
}
continue;
} else if element.has_in_variable(working_set)
&& !is_subexpression
{
*element =
wrap_element_with_collect(working_set, element);
}
} else if element.has_in_variable(working_set)
&& !is_subexpression
{
*element =
wrap_element_with_collect(working_set, element);
}
continue;
} else if element.has_in_variable(working_set)
&& !is_subexpression
{
*element = wrap_element_with_collect(working_set, element);
}
} else if element.has_in_variable(working_set) && !is_subexpression
{
*element = wrap_element_with_collect(working_set, element);
}
}
}
if error.is_none() {
error = err;
}
pipeline
}
}
if error.is_none() {
error = err;
}
pipeline
}
})
.into();
@ -5525,6 +5494,7 @@ pub fn discover_captures_in_pipeline_element(
match element {
PipelineElement::Expression(_, expression)
| PipelineElement::Redirection(_, _, expression)
| PipelineElement::And(_, expression)
| PipelineElement::Or(_, expression) => {
discover_captures_in_expr(working_set, expression, seen, seen_blocks)
}
@ -5785,6 +5755,9 @@ fn wrap_element_with_collect(
wrap_expr_with_collect(working_set, expression),
)
}
PipelineElement::And(span, expression) => {
PipelineElement::And(*span, wrap_expr_with_collect(working_set, expression))
}
PipelineElement::Or(span, expression) => {
PipelineElement::Or(*span, wrap_expr_with_collect(working_set, expression))
}
@ -5888,7 +5861,6 @@ impl LiteCommand {
pub enum LiteElement {
Command(Option<Span>, LiteCommand),
Redirection(Span, Redirection, LiteCommand),
Or(Span, LiteCommand),
}
#[derive(Debug)]
@ -5957,8 +5929,15 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
let mut curr_comment: Option<Vec<Span>> = None;
let mut error = None;
for token in tokens.iter() {
match &token.contents {
TokenContents::PipePipe => {
error = error.or(Some(ParseError::ShellOrOr(token.span)));
curr_command.push(token.span);
last_token = TokenContents::Item;
}
TokenContents::Item => {
// If we have a comment, go ahead and attach it
if let Some(curr_comment) = curr_comment.take() {
@ -6094,25 +6073,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
last_token = TokenContents::Eol;
}
TokenContents::PipePipe => {
// Different to redirection, for PipePipe, we'll wrap the current command
// in an "Or". This lets us know during eval that it's the current command
// whose error will be ignored rather than having to look ahead in the pipeline
if !curr_command.is_empty() {
curr_pipeline.push(LiteElement::Or(token.span, curr_command));
curr_command = LiteCommand::new();
}
if !curr_pipeline.is_empty() {
block.push(curr_pipeline);
curr_pipeline = LitePipeline::new();
last_connector = TokenContents::Pipe;
last_connector_span = None;
}
last_token = TokenContents::PipePipe;
}
TokenContents::Semicolon | TokenContents::AndAnd => {
TokenContents::Semicolon => {
if !curr_command.is_empty() {
match last_connector {
TokenContents::OutGreaterThan => {
@ -6141,7 +6102,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
}
_ => {
curr_pipeline
.push(LiteElement::Command(Some(token.span), curr_command));
.push(LiteElement::Command(last_connector_span, curr_command));
}
}
@ -6213,10 +6174,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
block.push(curr_pipeline);
}
if last_token == TokenContents::Pipe
|| last_token == TokenContents::PipePipe
|| last_token == TokenContents::AndAnd
{
if last_token == TokenContents::Pipe {
(
block,
Some(ParseError::UnexpectedEof(
@ -6225,7 +6183,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
)),
)
} else {
(block, None)
(block, error)
}
}

17
crates/nu-protocol/src/ast/pipeline.rs
View File

@ -14,6 +14,7 @@ pub enum Redirection {
pub enum PipelineElement {
Expression(Option<Span>, Expression),
Redirection(Span, Redirection, Expression),
And(Span, Expression),
Or(Span, Expression),
}
@ -22,8 +23,9 @@ impl PipelineElement {
match self {
PipelineElement::Expression(None, expression) => expression.span,
PipelineElement::Expression(Some(span), expression)
| PipelineElement::Or(span, expression)
| PipelineElement::Redirection(span, _, expression) => Span {
| PipelineElement::Redirection(span, _, expression)
| PipelineElement::And(span, expression)
| PipelineElement::Or(span, expression) => Span {
start: span.start,
end: expression.span.end,
},
@ -33,6 +35,7 @@ impl PipelineElement {
match self {
PipelineElement::Expression(_, expression)
| PipelineElement::Redirection(_, _, expression)
| PipelineElement::And(_, expression)
| PipelineElement::Or(_, expression) => expression.has_in_variable(working_set),
}
}
@ -40,8 +43,9 @@ impl PipelineElement {
pub fn replace_in_variable(&mut self, working_set: &mut StateWorkingSet, new_var_id: VarId) {
match self {
PipelineElement::Expression(_, expression)
| PipelineElement::Or(_, expression)
| PipelineElement::Redirection(_, _, expression) => {
| PipelineElement::Redirection(_, _, expression)
| PipelineElement::And(_, expression)
| PipelineElement::Or(_, expression) => {
expression.replace_in_variable(working_set, new_var_id)
}
}
@ -55,8 +59,9 @@ impl PipelineElement {
) {
match self {
PipelineElement::Expression(_, expression)
| PipelineElement::Or(_, expression)
| PipelineElement::Redirection(_, _, expression) => {
| PipelineElement::Redirection(_, _, expression)
| PipelineElement::And(_, expression)
| PipelineElement::Or(_, expression) => {
expression.replace_span(working_set, replaced, new_span)
}
}

7
crates/nu-protocol/src/engine/stack.rs
View File

@ -113,13 +113,6 @@ impl Stack {
.ok_or_else(|| ShellError::NushellFailed("No active overlay".into()))
}
pub fn last_exit_code(&self, engine_state: &EngineState) -> Option<i64> {
match self.get_env_var(engine_state, "LAST_EXIT_CODE") {
Some(Value::Int { val, .. }) => Some(val),
_ => None,
}
}
pub fn captures_to_stack(&self, captures: &HashMap<VarId, Value>) -> Stack {
// FIXME: this is probably slow
let mut env_vars = self.env_vars.clone();

1
tests/shell/pipeline/mod.rs
View File

@ -1,5 +1,4 @@
mod commands;
mod pipeline_operators;
use nu_test_support::nu;

74
tests/shell/pipeline/pipeline_operators.rs
View File

@ -1,74 +0,0 @@
use nu_test_support::nu;
#[test]
fn and_operator1() {
let actual = nu!(
cwd: ".",
r#"
3 / 0 && 2 + 3
"#
);
assert!(actual.err.contains("division by zero"));
}
#[test]
fn and_operator2() {
let actual = nu!(
cwd: ".",
r#"
0 | 3 / $in && 2 + 3
"#
);
assert!(actual.err.contains("division by zero"));
}
#[test]
fn or_operator1() {
let actual = nu!(
cwd: ".",
r#"
3 / 0 || 2 + 3
"#
);
assert!(actual.out.contains('5'));
}
#[test]
fn or_operator2() {
let actual = nu!(
cwd: ".",
r#"
0 | 3 / $in || 2 + 3
"#
);
assert!(actual.out.contains('5'));
}
#[test]
fn or_operator3() {
// On success, don't run the next step
let actual = nu!(
cwd: ".",
r#"
0 || 2 + 3
"#
);
assert_eq!(actual.out, "0");
}
#[test]
fn or_operator4() {
let actual = nu!(
cwd: ".",
r#"
1 / 0 || 2 / 0 || 10 + 9
"#
);
assert!(actual.out.contains("19"));
}