fix(parser): pitfall of redefining def

Author: blindFS

crates/nu-parser/src/parse_keywords.rs

@@ -4,7 +4,7 @@ use crate::{
     parser::{compile_block, parse_attribute, parse_redirection, redirecting_builtin_error},
     type_check::{check_block_input_output, type_compatible},
 };
-use itertools::Itertools;
+
 use log::trace;
 use nu_path::canonicalize_with;
 use nu_protocol::{
@@ -533,7 +533,7 @@ fn parse_def_inner(
             (spans[0], 1)
         };
 
-    let def_call = working_set.get_span_contents(name_span).to_vec();
+    let def_call = working_set.get_span_contents(name_span);
     if def_call != b"def" {
         working_set.error(ParseError::UnknownState(
             "internal error: Wrong call name for def function".into(),
@@ -549,7 +549,11 @@ fn parse_def_inner(
     // Parsing the spans and checking that they match the register signature
     // Using a parsed call makes more sense than checking for how many spans are in the call
     // Also, by creating a call, it can be checked if it matches the declaration signature
-    let (call, call_span) = match working_set.find_decl(&def_call) {
+    //
+    // NOTE: Here we only search for `def` in the permanent state,
+    // since recursively redefining `def` is dangerous,
+    // see https://github.com/nushell/nushell/issues/16586
+    let (call, call_span) = match working_set.permanent_state.find_decl(def_call, &[]) {
         None => {
             working_set.error(ParseError::UnknownState(
                 "internal error: def declaration not found".into(),
@@ -573,11 +577,7 @@ fn parse_def_inner(
 
             if let Some(name_span) = decl_name_span {
                 // Check whether name contains [] or () -- possible missing space error
-                if let Some(err) = detect_params_in_name(
-                    working_set,
-                    name_span,
-                    String::from_utf8_lossy(&def_call).as_ref(),
-                ) {
+                if let Some(err) = detect_params_in_name(working_set, name_span, decl_id) {
                     working_set.error(err);
                     return (garbage(working_set, Span::concat(spans)), None);
                 }
@@ -827,11 +827,7 @@ fn parse_extern_inner(
             let (command_spans, rest_spans) = spans.split_at(split_id);
 
             if let Some(name_span) = rest_spans.first() {
-                if let Some(err) = detect_params_in_name(
-                    working_set,
-                    *name_span,
-                    String::from_utf8_lossy(&extern_call).as_ref(),
-                ) {
+                if let Some(err) = detect_params_in_name(working_set, *name_span, decl_id) {
                     working_set.error(err);
                     return garbage(working_set, concat_span);
                 }
@@ -4216,35 +4212,24 @@ pub fn find_in_dirs(
 fn detect_params_in_name(
     working_set: &StateWorkingSet,
     name_span: Span,
-    decl_name: &str,
+    decl_id: DeclId,
 ) -> Option<ParseError> {
     let name = working_set.get_span_contents(name_span);
-
-    let extract_span = |delim: u8| {
-        // it is okay to unwrap because we know the slice contains the byte
-        let (idx, _) = name
-            .iter()
-            .find_position(|c| **c == delim)
-            .unwrap_or((name.len(), &b' '));
-        let param_span = Span::new(name_span.start + idx - 1, name_span.start + idx - 1);
-        let error = ParseError::LabeledErrorWithHelp {
-            error: "no space between name and parameters".into(),
-            label: "expected space".into(),
-            help: format!(
-                "consider adding a space between the `{decl_name}` command's name and its parameters"
-            ),
-            span: param_span,
-        };
-        Some(error)
-    };
-
-    if name.contains(&b'[') {
-        extract_span(b'[')
-    } else if name.contains(&b'(') {
-        extract_span(b'(')
-    } else {
-        None
+    for (offset, char) in name.iter().enumerate() {
+        if *char == b'[' || *char == b'(' {
+            return Some(ParseError::LabeledErrorWithHelp {
+                error: "no space between name and parameters".into(),
+                label: "expected space".into(),
+                help: format!(
+                    "consider adding a space between the `{}` command's name and its parameters",
+                    working_set.get_decl(decl_id).name()
+                ),
+                span: Span::new(offset + name_span.start - 1, offset + name_span.start - 1),
+            });
+        }
     }
+
+    None
 }
 
 /// Run has_flag_const and push possible error to working_set

tests/repl/test_parser.rs

@@ -1042,3 +1042,12 @@ fn external_argument_with_subexpressions() -> TestResult {
 fn quote_escape_but_not_env_shorthand() -> TestResult {
     run_test(r#""\"=foo""#, "\"=foo")
 }
+
+// https://github.com/nushell/nushell/issues/16586
+#[test]
+fn redefine_def_should_not_panic() -> TestResult {
+    fail_test(
+        r#"def def (=a|s)>"#,
+        "Missing required positional argument.",
+    )
+}