From 234484b6f87b2716f7b0a7d17d0fbddba9d7889d Mon Sep 17 00:00:00 2001 From: Solomon Date: Thu, 5 Dec 2024 06:35:15 -0700 Subject: [PATCH] normalize special characters in module names to allow variable access (#14353) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #14252 # User-Facing Changes - Special characters in module names are replaced with underscores when importing constants, preventing "expected valid variable name": ```nushell > module foo-bar { export const baz = 1 } > use foo-bar > $foo_bar.baz ``` - "expected valid variable name" errors now include a suggestion list: ```nushell > module foo-bar { export const baz = 1 } > use foo-bar > $foo-bar Error: nu::parser::parse_mismatch_with_did_you_mean × Parse mismatch during operation. ╭─[entry #1:1:1] 1 │ $foo-bar; · ────┬─── · ╰── expected valid variable name. Did you mean '$foo_bar'? ╰──── ``` --- crates/nu-parser/src/parser.rs | 32 ++++++++++---------- crates/nu-protocol/src/errors/parse_error.rs | 5 +++ crates/nu-protocol/src/module.rs | 31 ++++++++++++++++++- tests/repl/test_modules.rs | 23 ++++++++++++++ 4 files changed, 74 insertions(+), 17 deletions(-) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 0f8e699729..eecd7863a9 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -2122,7 +2122,21 @@ pub fn parse_variable_expr(working_set: &mut StateWorkingSet, span: Span) -> Exp String::from_utf8_lossy(contents).to_string() }; - if let Some(id) = parse_variable(working_set, span) { + let bytes = working_set.get_span_contents(span); + let suggestion = || { + DidYouMean::new( + &working_set.list_variables(), + working_set.get_span_contents(span), + ) + }; + if !is_variable(bytes) { + working_set.error(ParseError::ExpectedWithDidYouMean( + "valid variable name", + suggestion(), + span, + )); + garbage(working_set, span) + } else if let Some(id) = working_set.find_variable(bytes) { Expression::new( working_set, Expr::Var(id), @@ -2133,9 +2147,7 @@ pub fn parse_variable_expr(working_set: &mut StateWorkingSet, span: Span) -> Exp working_set.error(ParseError::EnvVarNotVar(name, span)); garbage(working_set, span) } else { - let ws = &*working_set; - let suggestion = DidYouMean::new(&ws.list_variables(), ws.get_span_contents(span)); - working_set.error(ParseError::VariableNotFound(suggestion, span)); + working_set.error(ParseError::VariableNotFound(suggestion(), span)); garbage(working_set, span) } } @@ -5612,18 +5624,6 @@ pub fn parse_expression(working_set: &mut StateWorkingSet, spans: &[Span]) -> Ex } } -pub fn parse_variable(working_set: &mut StateWorkingSet, span: Span) -> Option { - let bytes = working_set.get_span_contents(span); - - if is_variable(bytes) { - working_set.find_variable(bytes) - } else { - working_set.error(ParseError::Expected("valid variable name", span)); - - None - } -} - pub fn parse_builtin_commands( working_set: &mut StateWorkingSet, lite_command: &LiteCommand, diff --git a/crates/nu-protocol/src/errors/parse_error.rs b/crates/nu-protocol/src/errors/parse_error.rs index 786ce97509..f7de9f4890 100644 --- a/crates/nu-protocol/src/errors/parse_error.rs +++ b/crates/nu-protocol/src/errors/parse_error.rs @@ -55,6 +55,10 @@ pub enum ParseError { #[diagnostic(code(nu::parser::parse_mismatch_with_full_string_msg))] ExpectedWithStringMsg(String, #[label("expected {0}")] Span), + #[error("Parse mismatch during operation.")] + #[diagnostic(code(nu::parser::parse_mismatch_with_did_you_mean))] + ExpectedWithDidYouMean(&'static str, DidYouMean, #[label("expected {0}. {1}")] Span), + #[error("Command does not support {0} input.")] #[diagnostic(code(nu::parser::input_type_mismatch))] InputMismatch(Type, #[label("command doesn't support {0} input")] Span), @@ -551,6 +555,7 @@ impl ParseError { ParseError::Unbalanced(_, _, s) => *s, ParseError::Expected(_, s) => *s, ParseError::ExpectedWithStringMsg(_, s) => *s, + ParseError::ExpectedWithDidYouMean(_, _, s) => *s, ParseError::Mismatch(_, _, s) => *s, ParseError::UnsupportedOperationLHS(_, _, s, _) => *s, ParseError::UnsupportedOperationRHS(_, _, _, _, s, _) => *s, diff --git a/crates/nu-protocol/src/module.rs b/crates/nu-protocol/src/module.rs index 02bac131dc..0b1a371864 100644 --- a/crates/nu-protocol/src/module.rs +++ b/crates/nu-protocol/src/module.rs @@ -167,7 +167,7 @@ impl Module { vec![] } else { vec![( - final_name.clone(), + normalize_module_name(&final_name), Value::record( const_rows .into_iter() @@ -425,3 +425,32 @@ impl Module { result } } + +/// normalize module names for exporting as record constant +fn normalize_module_name(bytes: &[u8]) -> Vec { + bytes + .iter() + .map(|x| match is_identifier_byte(*x) { + true => *x, + false => b'_', + }) + .collect() +} + +fn is_identifier_byte(b: u8) -> bool { + b != b'.' + && b != b'[' + && b != b'(' + && b != b'{' + && b != b'+' + && b != b'-' + && b != b'*' + && b != b'^' + && b != b'/' + && b != b'=' + && b != b'!' + && b != b'<' + && b != b'>' + && b != b'&' + && b != b'|' +} diff --git a/tests/repl/test_modules.rs b/tests/repl/test_modules.rs index c2fc6b8dc2..d088d4ea30 100644 --- a/tests/repl/test_modules.rs +++ b/tests/repl/test_modules.rs @@ -1,4 +1,5 @@ use crate::repl::tests::{fail_test, run_test, TestResult}; +use rstest::rstest; #[test] fn module_def_imports_1() -> TestResult { @@ -145,6 +146,28 @@ fn export_module_which_defined_const() -> TestResult { ) } +#[rstest] +#[case("spam-mod")] +#[case("spam/mod")] +#[case("spam=mod")] +fn export_module_with_normalized_var_name(#[case] name: &str) -> TestResult { + let def = format!( + "module {name} {{ export const b = 3; export module {name}2 {{ export const c = 4 }} }}" + ); + run_test(&format!("{def}; use {name}; $spam_mod.b"), "3")?; + run_test(&format!("{def}; use {name} *; $spam_mod2.c"), "4") +} + +#[rstest] +#[case("spam-mod")] +#[case("spam/mod")] +fn use_module_with_invalid_var_name(#[case] name: &str) -> TestResult { + fail_test( + &format!("module {name} {{ export const b = 3 }}; use {name}; ${name}"), + "expected valid variable name. Did you mean '$spam_mod'", + ) +} + #[test] fn cannot_export_private_const() -> TestResult { fail_test(