refactor!: move reserved variable name checking near is_variable and add_variable

Author: blindFS

crates/nu-parser/src/parse_keywords.rs

@@ -58,20 +58,6 @@ pub const ALIASABLE_PARSER_KEYWORDS: &[&[u8]] = &[
     b"overlay use",
 ];
 
-pub const RESERVED_VARIABLE_NAMES: [&str; 4] = ["in", "nu", "env", "it"];
-
-pub fn ensure_not_reserved_variable_name(working_set: &mut StateWorkingSet, lvalue: &Expression) {
-    if lvalue.as_var().is_none() {
-        return;
-    }
-
-    let var_name = String::from_utf8_lossy(working_set.get_span_contents(lvalue.span))
-        .trim_start_matches('$')
-        .to_string();
-
-    verify_not_reserved_variable_name(working_set, &var_name, lvalue.span);
-}
-
 /// These parser keywords cannot be aliased (either not possible, or support not yet added)
 pub const UNALIASABLE_PARSER_KEYWORDS: &[&[u8]] = &[
     b"alias",
@@ -389,13 +375,6 @@ pub fn parse_for(working_set: &mut StateWorkingSet, lite_command: &LiteCommand)
     Expression::new(working_set, Expr::Call(call), call_span, Type::Nothing)
 }
 
-/// If `name` is a keyword, emit an error.
-fn verify_not_reserved_variable_name(working_set: &mut StateWorkingSet, name: &str, span: Span) {
-    if RESERVED_VARIABLE_NAMES.contains(&name) {
-        working_set.error(ParseError::NameIsBuiltinVar(name.to_string(), span))
-    }
-}
-
 // This is meant for parsing attribute blocks without an accompanying `def` or `extern`. It's
 // necessary to provide consistent syntax highlighting, completions, and helpful errors
 //
@@ -721,18 +700,18 @@ fn parse_def_inner(
     let mut result = None;
 
     if let (Some(mut signature), Some(block_id)) = (sig.as_signature(), block.as_block()) {
-        for arg_name in &signature.required_positional {
-            verify_not_reserved_variable_name(working_set, &arg_name.name, sig.span);
-        }
-        for arg_name in &signature.optional_positional {
-            verify_not_reserved_variable_name(working_set, &arg_name.name, sig.span);
-        }
-        if let Some(arg_name) = &signature.rest_positional {
-            verify_not_reserved_variable_name(working_set, &arg_name.name, sig.span);
-        }
-        for flag_name in &signature.get_names() {
-            verify_not_reserved_variable_name(working_set, flag_name, sig.span);
-        }
+        // for arg_name in &signature.required_positional {
+        //     verify_not_reserved_variable_name(working_set, &arg_name.name, sig.span);
+        // }
+        // for arg_name in &signature.optional_positional {
+        //     verify_not_reserved_variable_name(working_set, &arg_name.name, sig.span);
+        // }
+        // if let Some(arg_name) = &signature.rest_positional {
+        //     verify_not_reserved_variable_name(working_set, &arg_name.name, sig.span);
+        // }
+        // for flag_name in &signature.get_names() {
+        //     verify_not_reserved_variable_name(working_set, flag_name, sig.span);
+        // }
 
         if has_wrapped {
             if let Some(rest) = &signature.rest_positional {
@@ -3282,8 +3261,6 @@ pub fn parse_let(working_set: &mut StateWorkingSet, spans: &[Span]) -> Pipeline
                         working_set.error(ParseError::ExtraTokens(spans[idx + 2]));
                     }
 
-                    ensure_not_reserved_variable_name(working_set, &lvalue);
-
                     let var_id = lvalue.as_var();
                     let rhs_type = rvalue.ty.clone();
 
@@ -3398,8 +3375,6 @@ pub fn parse_const(working_set: &mut StateWorkingSet, spans: &[Span]) -> (Pipeli
                         working_set.error(ParseError::ExtraTokens(spans[idx + 2]));
                     }
 
-                    ensure_not_reserved_variable_name(working_set, &lvalue);
-
                     let var_id = lvalue.as_var();
                     let rhs_type = rvalue.ty.clone();
 
@@ -3561,8 +3536,6 @@ pub fn parse_mut(working_set: &mut StateWorkingSet, spans: &[Span]) -> Pipeline
                         working_set.error(ParseError::ExtraTokens(spans[idx + 2]));
                     }
 
-                    ensure_not_reserved_variable_name(working_set, &lvalue);
-
                     let var_id = lvalue.as_var();
                     let rhs_type = rvalue.ty.clone();
 

crates/nu-parser/src/parse_patterns.rs

@@ -2,7 +2,7 @@
 
 use crate::{
     lex, lite_parse,
-    parser::{is_variable, parse_value},
+    parser::{ensure_not_reserved_variable_name, is_variable, parse_value},
 };
 use nu_protocol::{
     ParseError, Span, SyntaxShape, Type, VarId,
@@ -54,8 +54,10 @@ fn parse_variable_pattern_helper(working_set: &mut StateWorkingSet, span: Span)
         if let Some(var_id) = working_set.find_variable_in_current_frame(bytes) {
             Some(var_id)
         } else {
-            let var_id = working_set.add_variable(bytes.to_vec(), span, Type::Any, false);
+            let name = bytes.to_vec();
+            ensure_not_reserved_variable_name(working_set, &name, span);
 
+            let var_id = working_set.add_variable(name, span, Type::Any, false);
             Some(var_id)
         }
     } else {

crates/nu-parser/src/parser.rs

@@ -1036,7 +1036,6 @@ pub fn parse_internal_call(
     let _ = working_set.add_span(call.head);
 
     let decl = working_set.get_decl(decl_id);
-    let decl_name = decl.name().to_string();
     let signature = working_set.get_signature(decl);
     let output = signature.get_output_type();
 
@@ -1379,12 +1378,6 @@ pub fn parse_internal_call(
     // Maybe more `CallKind::Invalid` if errors found during argument parsing.
     let call_kind = check_call(working_set, command_span, &signature, &call);
 
-    if decl_name == "let"
-        && let Some(lvalue) = call.positional_nth(0)
-    {
-        ensure_not_reserved_variable_name(working_set, lvalue);
-    }
-
     deprecation
         .into_iter()
         .filter_map(|entry| entry.parse_warning(&signature.name, &call))
@@ -3774,6 +3767,8 @@ pub fn parse_var_with_opt_type(
                 return (garbage(working_set, spans[*spans_idx - 1]), None);
             }
 
+            ensure_not_reserved_variable_name(working_set, &var_name, name_span);
+
             let id = working_set.add_variable(var_name, spans[*spans_idx - 1], ty.clone(), mutable);
 
             (
@@ -3789,6 +3784,8 @@ pub fn parse_var_with_opt_type(
                 return (garbage(working_set, spans[*spans_idx]), None);
             }
 
+            ensure_not_reserved_variable_name(working_set, &var_name, name_span);
+
             let id = working_set.add_variable(var_name, spans[*spans_idx], Type::Any, mutable);
 
             working_set.error(ParseError::MissingType(spans[*spans_idx]));
@@ -3808,6 +3805,8 @@ pub fn parse_var_with_opt_type(
             return (garbage(working_set, spans[*spans_idx]), None);
         }
 
+        ensure_not_reserved_variable_name(working_set, &var_name, name_span);
+
         let id = working_set.add_variable(
             var_name,
             Span::concat(&spans[*spans_idx..*spans_idx + 1]),
@@ -3822,6 +3821,23 @@ pub fn parse_var_with_opt_type(
     }
 }
 
+const RESERVED_VARIABLE_NAMES: [&[u8]; 3] = [b"in", b"nu", b"env"];
+
+pub(crate) fn ensure_not_reserved_variable_name(
+    working_set: &mut StateWorkingSet,
+    name: &[u8],
+    span: Span,
+) {
+    let var_name = name.strip_prefix(b"$").unwrap_or(name);
+
+    if RESERVED_VARIABLE_NAMES.contains(&var_name) {
+        working_set.error(ParseError::NameIsBuiltinVar(
+            String::from_utf8_lossy(var_name).to_string(),
+            span,
+        ))
+    }
+}
+
 pub fn expand_to_cell_path(
     working_set: &mut StateWorkingSet,
     expression: &mut Expression,
@@ -4196,6 +4212,12 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
                                     ))
                                 }
 
+                                ensure_not_reserved_variable_name(
+                                    working_set,
+                                    &variable_name,
+                                    span,
+                                );
+
                                 let var_id = working_set.add_variable(
                                     variable_name,
                                     span,
@@ -4285,6 +4307,12 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
                                     ))
                                 }
 
+                                ensure_not_reserved_variable_name(
+                                    working_set,
+                                    &variable_name,
+                                    span,
+                                );
+
                                 let var_id = working_set.add_variable(
                                     variable_name,
                                     span,
@@ -4357,6 +4385,12 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
                                     ))
                                 }
 
+                                ensure_not_reserved_variable_name(
+                                    working_set,
+                                    optional_param,
+                                    span,
+                                );
+
                                 let var_id = working_set.add_variable(
                                     optional_param.to_vec(),
                                     span,
@@ -4390,6 +4424,8 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
                                     ))
                                 }
 
+                                ensure_not_reserved_variable_name(working_set, contents, span);
+
                                 let var_id =
                                     working_set.add_variable(contents_vec, span, Type::Any, false);