Disallow ^ in def command names (#7606)

# Description

Closes #7273.

Also slightly edits/tidies up parser.rs.

# User-Facing Changes

`^` is now forbidden in `def` and `def-env` command names. EDIT: also
`alias`.

# 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 -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

# 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-command/tests/commands/alias.rs

@@ -43,35 +43,38 @@ fn alias_hiding_2() {
 
 #[test]
 fn alias_fails_with_invalid_name() {
+    let err_msg = "alias name can't be a number, a filesize, or contain a hash # or caret ^";
     let actual = nu!(
-        cwd: "tests/fixtures/formats", pipeline(
+        cwd: ".", pipeline(
         r#"
             alias 1234 = echo "test"   
         "#
     ));
-    assert!(actual
+    assert!(actual.err.contains(err_msg));
-        .err
-        .contains("alias name can't be a number, a filesize, or contain a hash"));
 
     let actual = nu!(
-        cwd: "tests/fixtures/formats", pipeline(
+        cwd: ".", pipeline(
         r#"
             alias 5gib = echo "test"   
         "#
     ));
-    assert!(actual
+    assert!(actual.err.contains(err_msg));
-        .err
-        .contains("alias name can't be a number, a filesize, or contain a hash"));
 
     let actual = nu!(
-        cwd: "tests/fixtures/formats", pipeline(
+        cwd: ".", pipeline(
         r#"
             alias "te#t" = echo "test"   
         "#
     ));
-    assert!(actual
+    assert!(actual.err.contains(err_msg));
-        .err
+
-        .contains("alias name can't be a number, a filesize, or contain a hash"));
+    let actual = nu!(
+        cwd: ".", pipeline(
+        r#"
+            alias ^foo = "bar"
+        "#
+    ));
+    assert!(actual.err.contains(err_msg));
 }
 
 #[test]

crates/nu-command/tests/commands/def.rs

@@ -28,38 +28,33 @@ fn def_errors_with_multiple_short_flags() {
         "#
     ));
 
-    assert!(actual.err.contains("expected one short flag"));
+    assert!(actual.err.contains("expected only one short flag"));
 }
 
 #[test]
 fn def_fails_with_invalid_name() {
+    let err_msg = "command name can't be a number, a filesize, or contain a hash # or caret ^";
     let actual = nu!(
-        cwd: "tests/fixtures/formats", pipeline(
+        cwd: ".", pipeline(
         r#"
             def 1234 = echo "test"
         "#
     ));
-    assert!(actual
+    assert!(actual.err.contains(err_msg));
-        .err
-        .contains("command name can't be a number, a filesize, or contain a hash"));
 
     let actual = nu!(
-        cwd: "tests/fixtures/formats", pipeline(
+        cwd: ".", pipeline(
         r#"
             def 5gib = echo "test"
         "#
     ));
-    assert!(actual
+    assert!(actual.err.contains(err_msg));
-        .err
-        .contains("command name can't be a number, a filesize, or contain a hash"));
 
     let actual = nu!(
-        cwd: "tests/fixtures/formats", pipeline(
+        cwd: ".", pipeline(
         r#"
-            def "te#t" = echo "test"
+            def ^foo [] {}
         "#
     ));
-    assert!(actual
+    assert!(actual.err.contains(err_msg));
-        .err
-        .contains("command name can't be a number, a filesize, or contain a hash"));
 }

crates/nu-parser/src/errors.rs

@@ -172,12 +172,15 @@ pub enum ParseError {
 
     #[error("Alias name not supported.")]
     #[diagnostic(code(nu::parser::variable_not_valid), url(docsrs))]
-    AliasNotValid(#[label = "alias name can't be a number, a filesize, or contain a hash"] Span),
+    AliasNotValid(
+        #[label = "alias name can't be a number, a filesize, or contain a hash # or caret ^"] Span,
+    ),
 
     #[error("Command name not supported.")]
     #[diagnostic(code(nu::parser::variable_not_valid), url(docsrs))]
     CommandDefNotValid(
-        #[label = "command name can't be a number, a filesize, or contain a hash"] Span,
+        #[label = "command name can't be a number, a filesize, or contain a hash # or caret ^"]
+        Span,
     ),
 
     #[error("Module not found.")]

crates/nu-parser/src/parse_keywords.rs

@@ -58,6 +58,7 @@ pub fn parse_def_predecl(
         working_set.exit_scope();
         if let (Some(name), Some(mut signature)) = (name, signature) {
             if name.contains('#')
+                || name.contains('^')
                 || name.parse::<bytesize::ByteSize>().is_ok()
                 || name.parse::<f64>().is_ok()
             {
@@ -661,6 +662,7 @@ pub fn parse_alias(
 
                 let checked_name = String::from_utf8_lossy(&alias_name);
                 if checked_name.contains('#')
+                    || checked_name.contains('^')
                     || checked_name.parse::<bytesize::ByteSize>().is_ok()
                     || checked_name.parse::<f64>().is_ok()
                 {

crates/nu-parser/src/parser.rs

@@ -3265,6 +3265,7 @@ pub fn parse_signature_helper(
                 let span = *span;
                 let contents = working_set.get_span_contents(span);
 
+                // The : symbol separates types
                 if contents == b":" {
                     match parse_mode {
                         ParseMode::ArgMode => {
@@ -3276,7 +3277,9 @@ pub fn parse_signature_helper(
                                 error.or_else(|| Some(ParseError::Expected("type".into(), span)));
                         }
                     }
-                } else if contents == b"=" {
+                }
+                // The = symbol separates a variable from its default value
+                else if contents == b"=" {
                     match parse_mode {
                         ParseMode::ArgMode | ParseMode::TypeMode => {
                             parse_mode = ParseMode::DefaultValueMode;
@@ -3291,8 +3294,10 @@ pub fn parse_signature_helper(
                 } else {
                     match parse_mode {
                         ParseMode::ArgMode => {
+                            // Long flag with optional short form, e.g. --output, --age (-a)
                             if contents.starts_with(b"--") && contents.len() > 2 {
-                                // Long flag
+                                // Split the long flag from the short flag with the ( character as delimiter.
+                                // The trailing ) is removed further down.
                                 let flags: Vec<_> =
                                     contents.split(|x| x == &b'(').map(|x| x.to_vec()).collect();
 
@@ -3317,6 +3322,7 @@ pub fn parse_signature_helper(
                                 let var_id =
                                     working_set.add_variable(variable_name, span, Type::Any, false);
 
+                                // If there's no short flag, exit now. Otherwise, parse it.
                                 if flags.len() == 1 {
                                     args.push(Arg::Flag(Flag {
                                         arg: None,
@@ -3329,7 +3335,10 @@ pub fn parse_signature_helper(
                                     }));
                                 } else if flags.len() >= 3 {
                                     error = error.or_else(|| {
-                                        Some(ParseError::Expected("one short flag".into(), span))
+                                        Some(ParseError::Expected(
+                                            "only one short flag alternative".into(),
+                                            span,
+                                        ))
                                     });
                                 } else {
                                     let short_flag = &flags[1];
@@ -3337,12 +3346,18 @@ pub fn parse_signature_helper(
                                         || !short_flag.ends_with(b")")
                                     {
                                         error = error.or_else(|| {
-                                            Some(ParseError::Expected("short flag".into(), span))
+                                            Some(ParseError::Expected(
+                                                "short flag alternative for the long flag".into(),
+                                                span,
+                                            ))
                                         });
                                         short_flag
                                     } else {
+                                        // Obtain the flag's name by removing the starting - and trailing )
                                         &short_flag[1..(short_flag.len() - 1)]
                                     };
+                                    // Note that it is currently possible to make a short flag with non-alphanumeric characters,
+                                    // like -).
 
                                     let short_flag =
                                         String::from_utf8_lossy(short_flag).to_string();
@@ -3388,9 +3403,9 @@ pub fn parse_signature_helper(
                                         });
                                     }
                                 }
-                            } else if contents.starts_with(b"-") && contents.len() > 1 {
+                            }
-                                // Short flag
+                            // Mandatory short flag, e.g. -e (must be one character)
-
+                            else if contents.starts_with(b"-") && contents.len() > 1 {
                                 let short_flag = &contents[1..];
                                 let short_flag = String::from_utf8_lossy(short_flag).to_string();
                                 let chars: Vec<char> = short_flag.chars().collect();
@@ -3404,6 +3419,7 @@ pub fn parse_signature_helper(
                                 let mut encoded_var_name = vec![0u8; 4];
                                 let len = chars[0].encode_utf8(&mut encoded_var_name).len();
                                 let variable_name = encoded_var_name[0..len].to_vec();
+
                                 if !is_variable(&variable_name) {
                                     error = error.or_else(|| {
                                         Some(ParseError::Expected(
@@ -3425,7 +3441,10 @@ pub fn parse_signature_helper(
                                     var_id: Some(var_id),
                                     default_value: None,
                                 }));
-                            } else if contents.starts_with(b"(-") {
+                            }
+                            // Short flag alias for long flag, e.g. --b, (-a)
+                            // This is the same as --b (-a)
+                            else if contents.starts_with(b"(-") {
                                 let short_flag = &contents[2..];
 
                                 let short_flag = if !short_flag.ends_with(b")") {
@@ -3468,7 +3487,9 @@ pub fn parse_signature_helper(
                                         Some(ParseError::Expected("short flag".into(), span))
                                     });
                                 }
-                            } else if contents.ends_with(b"?") {
+                            }
+                            // Positional arg, optional
+                            else if contents.ends_with(b"?") {
                                 let contents: Vec<_> = contents[..(contents.len() - 1)].into();
                                 let name = String::from_utf8_lossy(&contents).to_string();
 
@@ -3484,7 +3505,6 @@ pub fn parse_signature_helper(
                                 let var_id =
                                     working_set.add_variable(contents, span, Type::Any, false);
 
-                                // Positional arg, optional
                                 args.push(Arg::Positional(
                                     PositionalArg {
                                         desc: String::new(),
@@ -3495,9 +3515,12 @@ pub fn parse_signature_helper(
                                     },
                                     false,
                                 ))
-                            } else if let Some(contents) = contents.strip_prefix(b"...") {
+                            }
+                            // Rest param
+                            else if let Some(contents) = contents.strip_prefix(b"...") {
                                 let name = String::from_utf8_lossy(contents).to_string();
                                 let contents_vec: Vec<u8> = contents.to_vec();
+
                                 if !is_variable(&contents_vec) {
                                     error = error.or_else(|| {
                                         Some(ParseError::Expected(
@@ -3517,7 +3540,9 @@ pub fn parse_signature_helper(
                                     var_id: Some(var_id),
                                     default_value: None,
                                 }));
-                            } else {
+                            }
+                            // Normal param
+                            else {
                                 let name = String::from_utf8_lossy(contents).to_string();
                                 let contents_vec = contents.to_vec();
 

src/tests/test_engine.rs

@@ -319,6 +319,11 @@ fn default_value12() -> TestResult {
     fail_test(r#"def foo [--x:int = "a"] { $x }"#, "default value not int")
 }
 
+#[test]
+fn default_value_expression() -> TestResult {
+    run_test(r#"def foo [x = ("foo" | str length)] { $x }; foo"#, "3")
+}
+
 #[test]
 fn loose_each() -> TestResult {
     run_test(r#"[[1, 2, 3], [4, 5, 6]] | each { $in.1 } | math sum"#, "7")