From 5e937ca1afc6aee2296615f64330331952c7cd53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sat, 9 Mar 2024 18:58:02 +0200 Subject: [PATCH] Refactor nu-check (#12137) # Description This PR refactors `nu-check` and makes it possible to check module directories. Also removes the requirement for files to end with .nu: It was too limiting for module directories and there are executable scripts [around](https://github.com/nushell/nu_scripts/tree/main/make_release/release-note) that do not end with .nu, it's a common practice for scripts to omit it. Other changes are: * Removed the `--all` flag and heuristic parse because these are irrelevant now when module syntax is a subset of script syntax (i.e., every module can be parsed as script). * Reduced code duplication and in general tidied up the code * Replaced unspanned errors with spanned ones. # User-Facing Changes * `nu-check` doesn't require files to end with .nu * can check module directories * Removed `--all` flag # Tests + Formatting # After Submitting --- crates/nu-command/src/system/nu_check.rs | 334 +++++++------------ crates/nu-command/tests/commands/nu_check.rs | 113 ++----- 2 files changed, 152 insertions(+), 295 deletions(-) diff --git a/crates/nu-command/src/system/nu_check.rs b/crates/nu-command/src/system/nu_check.rs index 009074d9b17c..90363782013f 100644 --- a/crates/nu-command/src/system/nu_check.rs +++ b/crates/nu-command/src/system/nu_check.rs @@ -1,11 +1,13 @@ -use nu_engine::{find_in_dirs_env, get_dirs_var_from_call, CallExt}; -use nu_parser::{parse, parse_module_block, unescape_unquote_string}; +use nu_engine::{env::get_config, find_in_dirs_env, get_dirs_var_from_call, CallExt}; +use nu_parser::{parse, parse_module_block, parse_module_file_or_dir, unescape_unquote_string}; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack, StateWorkingSet}; use nu_protocol::{ Category, Example, PipelineData, ShellError, Signature, Span, Spanned, SyntaxShape, Type, Value, }; +use std::path::Path; + #[derive(Clone)] pub struct NuCheck; @@ -16,14 +18,15 @@ impl Command for NuCheck { fn signature(&self) -> Signature { Signature::build("nu-check") - .input_output_types(vec![(Type::String, Type::Bool), - (Type::ListStream, Type::Bool), - (Type::List(Box::new(Type::Any)), Type::Bool)]) + .input_output_types(vec![ + (Type::String, Type::Bool), + (Type::ListStream, Type::Bool), + (Type::List(Box::new(Type::Any)), Type::Bool), + ]) // type is string to avoid automatically canonicalizing the path .optional("path", SyntaxShape::String, "File path to parse.") .switch("as-module", "Parse content as module", Some('m')) .switch("debug", "Show error messages", Some('d')) - .switch("all", "Parse content as script first, returns result if success, otherwise, try with module", Some('a')) .category(Category::Strings) } @@ -42,45 +45,30 @@ impl Command for NuCheck { call: &Call, input: PipelineData, ) -> Result { - let path: Option> = call.opt(engine_state, stack, 0)?; - let is_module = call.has_flag(engine_state, stack, "as-module")?; + let path_arg: Option> = call.opt(engine_state, stack, 0)?; + let as_module = call.has_flag(engine_state, stack, "as-module")?; let is_debug = call.has_flag(engine_state, stack, "debug")?; - let is_all = call.has_flag(engine_state, stack, "all")?; - let config = engine_state.get_config(); - let mut contents = vec![]; // DO NOT ever try to merge the working_set in this command let mut working_set = StateWorkingSet::new(engine_state); - if is_all && is_module { - return Err(ShellError::GenericError { - error: "Detected command flags conflict".into(), - msg: "You cannot have both `--all` and `--as-module` on the same command line, please refer to `nu-check --help` for more details".into(), - span: Some(call.head), - help: None, - inner: vec![] - }); - } + let input_span = input.span().unwrap_or(call.head); - let span = input.span().unwrap_or(call.head); match input { PipelineData::Value(Value::String { val, .. }, ..) => { let contents = Vec::from(val); - if is_all { - heuristic_parse(&mut working_set, None, &contents, is_debug, call.head) - } else if is_module { - parse_module(&mut working_set, None, &contents, is_debug, span) + if as_module { + parse_module(&mut working_set, None, &contents, is_debug, input_span) } else { - parse_script(&mut working_set, None, &contents, is_debug, span) + parse_script(&mut working_set, None, &contents, is_debug, input_span) } } PipelineData::ListStream(stream, ..) => { - let list_stream = stream.into_string("\n", config); + let config = get_config(engine_state, stack); + let list_stream = stream.into_string("\n", &config); let contents = Vec::from(list_stream); - if is_all { - heuristic_parse(&mut working_set, None, &contents, is_debug, call.head) - } else if is_module { + if as_module { parse_module(&mut working_set, None, &contents, is_debug, call.head) } else { parse_script(&mut working_set, None, &contents, is_debug, call.head) @@ -90,6 +78,7 @@ impl Command for NuCheck { stdout: Some(stream), .. } => { + let mut contents = vec![]; let raw_stream: Vec<_> = stream.stream.collect(); for r in raw_stream { match r { @@ -98,16 +87,16 @@ impl Command for NuCheck { }; } - if is_all { - heuristic_parse(&mut working_set, None, &contents, is_debug, call.head) - } else if is_module { + if as_module { parse_module(&mut working_set, None, &contents, is_debug, call.head) } else { parse_script(&mut working_set, None, &contents, is_debug, call.head) } } _ => { - if let Some(path_str) = path { + if let Some(path_str) = path_arg { + let path_span = path_str.span; + // look up the path as relative to FILE_PWD or inside NU_LIB_DIRS (same process as source-env) let path = match find_in_dirs_env( &path_str.item, @@ -121,44 +110,32 @@ impl Command for NuCheck { } else { return Err(ShellError::FileNotFound { file: path_str.item, - span: path_str.span, + span: path_span, }); } } Err(error) => return Err(error), }; - // get the expanded path as a string - let path_str = path.to_string_lossy().to_string(); - - let ext: Vec<_> = path_str.rsplitn(2, '.').collect(); - if ext[0] != "nu" { - return Err(ShellError::GenericError { - error: "Cannot parse input".into(), - msg: "File extension must be the type of .nu".into(), - span: Some(call.head), - help: None, - inner: vec![], - }); - } - // Change currently parsed directory let prev_currently_parsed_cwd = if let Some(parent) = path.parent() { let prev = working_set.currently_parsed_cwd.clone(); - working_set.currently_parsed_cwd = Some(parent.into()); - prev } else { working_set.currently_parsed_cwd.clone() }; - let result = if is_all { - heuristic_parse_file(path_str, &mut working_set, call, is_debug) - } else if is_module { - parse_file_module(path_str, &mut working_set, call, is_debug) + let result = if as_module || path.is_dir() { + parse_file_or_dir_module( + path.to_string_lossy().as_bytes(), + &mut working_set, + is_debug, + path_span, + call.head, + ) } else { - parse_file_script(path_str, &mut working_set, call, is_debug) + parse_file_script(&path, &mut working_set, is_debug, path_span, call.head) }; // Restore the currently parsed directory back @@ -168,9 +145,9 @@ impl Command for NuCheck { } else { Err(ShellError::GenericError { error: "Failed to execute command".into(), - msg: "Please run 'nu-check --help' for more details".into(), + msg: "Requires path argument if ran without pipeline input".into(), span: Some(call.head), - help: None, + help: Some("Please run 'nu-check --help' for more details".into()), inner: vec![], }) } @@ -224,101 +201,12 @@ impl Command for NuCheck { } } -fn heuristic_parse( - working_set: &mut StateWorkingSet, - filename: Option<&str>, - contents: &[u8], - is_debug: bool, - span: Span, -) -> Result { - match parse_script(working_set, filename, contents, is_debug, span) { - Ok(v) => Ok(v), - Err(_) => { - match parse_module( - working_set, - filename.map(|f| f.to_string()), - contents, - is_debug, - span, - ) { - Ok(v) => Ok(v), - Err(_) => { - if is_debug { - Err(ShellError::GenericError { - error: "Failed to parse content,tried both script and module".into(), - msg: "syntax error".into(), - span: Some(span), - help: Some("Run `nu-check --help` for more details".into()), - inner: vec![], - }) - } else { - Ok(PipelineData::Value(Value::bool(false, span), None)) - } - } - } - } - } -} - -fn heuristic_parse_file( - path: String, - working_set: &mut StateWorkingSet, - call: &Call, - is_debug: bool, -) -> Result { - let starting_error_count = working_set.parse_errors.len(); - let bytes = working_set.get_span_contents(call.head); - let (filename, err) = unescape_unquote_string(bytes, call.head); - if let Some(err) = err { - working_set.error(err); - } - if starting_error_count == working_set.parse_errors.len() { - if let Ok(contents) = std::fs::read(path) { - match parse_script( - working_set, - Some(filename.as_str()), - &contents, - is_debug, - call.head, - ) { - Ok(v) => Ok(v), - Err(_) => { - match parse_module(working_set, Some(filename), &contents, is_debug, call.head) - { - Ok(v) => Ok(v), - Err(_) => { - if is_debug { - Err(ShellError::GenericError { - error: "Failed to parse content,tried both script and module" - .into(), - msg: "syntax error".into(), - span: Some(call.head), - help: Some("Run `nu-check --help` for more details".into()), - inner: vec![], - }) - } else { - Ok(PipelineData::Value(Value::bool(false, call.head), None)) - } - } - } - } - } - } else { - Err(ShellError::IOError { - msg: "Can not read input".to_string(), - }) - } - } else { - Err(ShellError::NotFound { span: call.head }) - } -} - fn parse_module( working_set: &mut StateWorkingSet, filename: Option, contents: &[u8], is_debug: bool, - span: Span, + call_head: Span, ) -> Result { let filename = filename.unwrap_or_else(|| "empty".to_string()); @@ -328,28 +216,16 @@ fn parse_module( let starting_error_count = working_set.parse_errors.len(); parse_module_block(working_set, new_span, filename.as_bytes()); - if starting_error_count != working_set.parse_errors.len() { - if is_debug { - let msg = format!( - r#"Found : {}"#, - working_set - .parse_errors - .first() - .expect("Unable to parse content as module") - ); - Err(ShellError::GenericError { - error: "Failed to parse content".into(), - msg, - span: Some(span), - help: Some("If the content is intended to be a script, please try to remove `--as-module` flag ".into()), - inner: vec![], - }) - } else { - Ok(PipelineData::Value(Value::bool(false, new_span), None)) - } - } else { - Ok(PipelineData::Value(Value::bool(true, new_span), None)) - } + check_parse( + starting_error_count, + working_set, + is_debug, + Some( + "If the content is intended to be a script, please try to remove `--as-module` flag " + .to_string(), + ), + call_head, + ) } fn parse_script( @@ -357,86 +233,116 @@ fn parse_script( filename: Option<&str>, contents: &[u8], is_debug: bool, - span: Span, + call_head: Span, ) -> Result { let starting_error_count = working_set.parse_errors.len(); parse(working_set, filename, contents, false); + check_parse(starting_error_count, working_set, is_debug, None, call_head) +} + +fn check_parse( + starting_error_count: usize, + working_set: &StateWorkingSet, + is_debug: bool, + help: Option, + call_head: Span, +) -> Result { if starting_error_count != working_set.parse_errors.len() { let msg = format!( r#"Found : {}"#, working_set .parse_errors .first() - .expect("Unable to parse content") + .expect("Missing parser error") ); + if is_debug { Err(ShellError::GenericError { error: "Failed to parse content".into(), msg, - span: Some(span), - help: Some("If the content is intended to be a module, please consider flag of `--as-module` ".into()), + span: Some(call_head), + help, inner: vec![], }) } else { - Ok(PipelineData::Value(Value::bool(false, span), None)) + Ok(PipelineData::Value(Value::bool(false, call_head), None)) } } else { - Ok(PipelineData::Value(Value::bool(true, span), None)) + Ok(PipelineData::Value(Value::bool(true, call_head), None)) } } fn parse_file_script( - path: String, + path: &Path, working_set: &mut StateWorkingSet, - call: &Call, is_debug: bool, + path_span: Span, + call_head: Span, ) -> Result { - let starting_error_count = working_set.parse_errors.len(); - let bytes = working_set.get_span_contents(call.head); - let (filename, err) = unescape_unquote_string(bytes, call.head); - if let Some(err) = err { - working_set.error(err) - } - if starting_error_count == working_set.parse_errors.len() { - if let Ok(contents) = std::fs::read(path) { - parse_script( - working_set, - Some(filename.as_str()), - &contents, - is_debug, - call.head, - ) - } else { - Err(ShellError::IOError { - msg: "Can not read path".to_string(), - }) - } + let filename = check_path(working_set, path_span, call_head)?; + + if let Ok(contents) = std::fs::read(path) { + parse_script(working_set, Some(&filename), &contents, is_debug, call_head) } else { - Err(ShellError::NotFound { span: call.head }) + Err(ShellError::IOErrorSpanned { + msg: "Could not read path".to_string(), + span: path_span, + }) } } -fn parse_file_module( - path: String, +fn parse_file_or_dir_module( + path_bytes: &[u8], working_set: &mut StateWorkingSet, - call: &Call, is_debug: bool, + path_span: Span, + call_head: Span, ) -> Result { + let _ = check_path(working_set, path_span, call_head)?; + let starting_error_count = working_set.parse_errors.len(); - let bytes = working_set.get_span_contents(call.head); - let (filename, err) = unescape_unquote_string(bytes, call.head); - if let Some(err) = err { - working_set.error(err); - } - if starting_error_count == working_set.parse_errors.len() { - if let Ok(contents) = std::fs::read(path) { - parse_module(working_set, Some(filename), &contents, is_debug, call.head) - } else { - Err(ShellError::IOError { - msg: "Can not read path".to_string(), + let _ = parse_module_file_or_dir(working_set, path_bytes, path_span, None); + + if starting_error_count != working_set.parse_errors.len() { + if is_debug { + let msg = format!( + r#"Found : {}"#, + working_set + .parse_errors + .first() + .expect("Missing parser error") + ); + Err(ShellError::GenericError { + error: "Failed to parse content".into(), + msg, + span: Some(path_span), + help: Some("If the content is intended to be a script, please try to remove `--as-module` flag ".into()), + inner: vec![], }) + } else { + Ok(PipelineData::Value(Value::bool(false, call_head), None)) } } else { - Err(ShellError::NotFound { span: call.head }) + Ok(PipelineData::Value(Value::bool(true, call_head), None)) + } +} + +fn check_path( + working_set: &mut StateWorkingSet, + path_span: Span, + call_head: Span, +) -> Result { + let bytes = working_set.get_span_contents(path_span); + let (filename, err) = unescape_unquote_string(bytes, path_span); + if let Some(e) = err { + Err(ShellError::GenericError { + error: "Could not escape filename".to_string(), + msg: "could not escape filename".to_string(), + span: Some(call_head), + help: Some(format!("Returned error: {e}")), + inner: vec![], + }) + } else { + Ok(filename) } } diff --git a/crates/nu-command/tests/commands/nu_check.rs b/crates/nu-command/tests/commands/nu_check.rs index c2c5bc5783ac..c8c825441182 100644 --- a/crates/nu-command/tests/commands/nu_check.rs +++ b/crates/nu-command/tests/commands/nu_check.rs @@ -176,52 +176,6 @@ fn file_not_exist() { }) } -#[test] -fn parse_unsupported_file() { - Playground::setup("nu_check_test_8", |dirs, sandbox| { - sandbox.with_files(vec![FileWithContentToBeTrimmed( - "foo.txt", - r#" - # foo.nu - - export def hello [name: string { - $"hello ($name)!" - } - - export def hi [where: string] { - $"hi ($where)!" - } - "#, - )]); - - let actual = nu!( - cwd: dirs.test(), pipeline( - " - nu-check --as-module foo.txt - " - )); - - assert!(actual - .err - .contains("File extension must be the type of .nu")); - }) -} -#[test] -fn parse_dir_failure() { - Playground::setup("nu_check_test_9", |dirs, _sandbox| { - let actual = nu!( - cwd: dirs.test(), pipeline( - " - nu-check --as-module ~ - " - )); - - assert!(actual - .err - .contains("File extension must be the type of .nu")); - }) -} - #[test] fn parse_module_success_2() { Playground::setup("nu_check_test_10", |dirs, sandbox| { @@ -554,7 +508,7 @@ fn parse_module_success_with_complex_external_stream() { } #[test] -fn parse_with_flag_all_success_for_complex_external_stream() { +fn parse_with_flag_success_for_complex_external_stream() { Playground::setup("nu_check_test_20", |dirs, sandbox| { sandbox.with_files(vec![FileWithContentToBeTrimmed( "grep.nu", @@ -594,7 +548,7 @@ fn parse_with_flag_all_success_for_complex_external_stream() { let actual = nu!( cwd: dirs.test(), pipeline( " - open grep.nu | nu-check --all --debug + open grep.nu | nu-check --debug " )); @@ -603,7 +557,7 @@ fn parse_with_flag_all_success_for_complex_external_stream() { } #[test] -fn parse_with_flag_all_failure_for_complex_external_stream() { +fn parse_with_flag_failure_for_complex_external_stream() { Playground::setup("nu_check_test_21", |dirs, sandbox| { sandbox.with_files(vec![FileWithContentToBeTrimmed( "grep.nu", @@ -643,16 +597,16 @@ fn parse_with_flag_all_failure_for_complex_external_stream() { let actual = nu!( cwd: dirs.test(), pipeline( " - open grep.nu | nu-check --all --debug + open grep.nu | nu-check --debug " )); - assert!(actual.err.contains("syntax error")); + assert!(actual.err.contains("Failed to parse content")); }) } #[test] -fn parse_with_flag_all_failure_for_complex_list_stream() { +fn parse_with_flag_failure_for_complex_list_stream() { Playground::setup("nu_check_test_22", |dirs, sandbox| { sandbox.with_files(vec![FileWithContentToBeTrimmed( "grep.nu", @@ -692,38 +646,11 @@ fn parse_with_flag_all_failure_for_complex_list_stream() { let actual = nu!( cwd: dirs.test(), pipeline( " - open grep.nu | lines | nu-check --all --debug - " - )); - - assert!(actual.err.contains("syntax error")); - }) -} - -#[test] -fn parse_failure_due_conflicted_flags() { - Playground::setup("nu_check_test_23", |dirs, sandbox| { - sandbox.with_files(vec![FileWithContentToBeTrimmed( - "script.nu", - r#" - greet "world" - - def greet [name] { - echo "hello" $name - } - "#, - )]); - - let actual = nu!( - cwd: dirs.test(), pipeline( - " - nu-check -a --as-module script.nu + open grep.nu | lines | nu-check --debug " )); - assert!(actual - .err - .contains("You cannot have both `--all` and `--as-module` on the same command line")); + assert!(actual.err.contains("Failed to parse content")); }) } @@ -793,3 +720,27 @@ fn nu_check_respects_file_pwd() { assert_eq!(actual.out, "true"); }) } +#[test] +fn nu_check_module_dir() { + Playground::setup("nu_check_test_26", |dirs, sandbox| { + sandbox + .mkdir("lol") + .with_files(vec![FileWithContentToBeTrimmed( + "lol/mod.nu", + r#" + export module foo.nu + export def main [] { 'lol' } + "#, + )]) + .with_files(vec![FileWithContentToBeTrimmed( + "lol/foo.nu", + r#" + export def main [] { 'lol foo' } + "#, + )]); + + let actual = nu!(cwd: dirs.test(), pipeline( "nu-check lol")); + + assert_eq!(actual.out, "true"); + }) +}