From 34e7bd861ce3976c17e8d11010e13eccaec5ca45 Mon Sep 17 00:00:00 2001
From: Yash Thakur <45539777+ysthakur@users.noreply.github.com>
Date: Tue, 20 Aug 2024 22:35:13 -0400
Subject: [PATCH] Fix bug introduced by #13595 (#13658)

<!--
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 https://github.com/nushell/nushell/pull/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 #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 #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.
-->
---
 crates/nu-parser/src/parser.rs        |  5 ++++
 crates/nu-parser/tests/test_parser.rs | 34 +++++++++++++++++----------
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs
index 742bd2264493..33ecdadd3a20 100644
--- a/crates/nu-parser/src/parser.rs
+++ b/crates/nu-parser/src/parser.rs
@@ -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 ".."
@@ -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,
diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs
index 48ee9a7d9a75..f72b52b433b8 100644
--- a/crates/nu-parser/tests/test_parser.rs
+++ b/crates/nu-parser/tests/test_parser.rs
@@ -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]
@@ -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)]