Skip to content

Commit

Permalink
Fix bug introduced by nushell#13595 (nushell#13658)
Browse files Browse the repository at this point in the history
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

@devyn found that nushell#13595, which
made ranges be type-checked at parse time, introduced a bug that caused
`../foo` to be parsed as a string rather than a command call. This was
caused by `parse_range` returning a `Some` despite there being parse
errors (`/foo` doesn't match `SyntaxShape::Number`). To go back to the
old behavior, `parse_range` now returns `None` anytime there's any parse
errors met while parsing the range.

Unfortunately, this means that something like `..$foo` will be parsed as
a string if `$foo` isn't defined and as a range if it is defined. That
was the behavior before nushell#13595, and it should probably be fixed at some
point, but I'm just trying to quickly fix the bug.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

Things should go back to the way they were before nushell#13595, except the
type-checking stuff from that PR is still here.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

Added a test. Reverted another test that tests that `0..<$day` is parsed
successfully as a string if the variable isn't defined.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
  • Loading branch information
ysthakur authored Aug 21, 2024
1 parent d667b3c commit 34e7bd8
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 12 deletions.
5 changes: 5 additions & 0 deletions crates/nu-parser/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1594,6 +1594,7 @@ pub fn parse_number(working_set: &mut StateWorkingSet, span: Span) -> Expression

pub fn parse_range(working_set: &mut StateWorkingSet, span: Span) -> Option<Expression> {
trace!("parsing: range");
let starting_error_count = working_set.parse_errors.len();

// Range follows the following syntax: [<from>][<next_operator><next>]<range_operator>[<to>]
// where <next_operator> is ".."
Expand Down Expand Up @@ -1715,6 +1716,10 @@ pub fn parse_range(working_set: &mut StateWorkingSet, span: Span) -> Option<Expr
(None, span)
};

if working_set.parse_errors.len() != starting_error_count {
return None;
}

let operator = RangeOperator {
inclusion,
span: range_op_span,
Expand Down
34 changes: 22 additions & 12 deletions crates/nu-parser/tests/test_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1828,23 +1828,13 @@ mod range {
}

#[test]
fn vars_read_as_units() {
fn vars_not_read_as_units() {
let engine_state = EngineState::new();
let mut working_set = StateWorkingSet::new(&engine_state);

let _ = parse(&mut working_set, None, b"0..<$day", true);

assert!(
working_set.parse_errors.len() == 1,
"Errors: {:?}",
working_set.parse_errors
);
let err = &working_set.parse_errors[0].to_string();
assert!(
err.contains("Variable not found"),
"Expected variable not found error, got {}",
err
);
assert!(working_set.parse_errors.is_empty());
}

#[rstest]
Expand Down Expand Up @@ -1878,6 +1868,26 @@ mod range {
err
);
}

#[test]
fn dont_mess_with_external_calls() {
let engine_state = EngineState::new();
let mut working_set = StateWorkingSet::new(&engine_state);
working_set.add_decl(Box::new(ToCustom));

let result = parse(&mut working_set, None, b"../foo", true);

assert!(
working_set.parse_errors.is_empty(),
"Errors: {:?}",
working_set.parse_errors
);
let expr = &result.pipelines[0].elements[0].expr.expr;
assert!(
matches!(expr, Expr::ExternalCall(..)),
"Should've been parsed as a call"
);
}
}

#[cfg(test)]
Expand Down

0 comments on commit 34e7bd8

Please sign in to comment.