From def36865effde927b919c3221f1ee44b05798770 Mon Sep 17 00:00:00 2001 From: Wind Date: Wed, 26 Jun 2024 09:33:37 +0800 Subject: [PATCH] Enable reloading changes to a submodule (#13170) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description Fixes: https://github.com/nushell/nushell/issues/12099 Currently if user run `use voice.nu`, and file is unchanged, then run `use voice.nu` again. nushell will use the module directly, even if submodule inside `voice.nu` is changed. After discussed with @kubouch, I think it's ok to re-parse the module file when: 1. It exports sub modules which are defined by a file 2. It uses other modules which are defined by a file ## About the change: To achieve the behavior, we need to add 2 attributes to `Module`: 1. `imported_modules`: it tracks the other modules is imported by the givem `module`, e.g: `use foo.nu` 2. `file`: the path of a module, if a module is defined by a file, it will be `Some(path)`, or else it will be `None`. After the change: use voice.nu always read the file and parse it. use voice will still use the module which is saved in EngineState. # User-Facing Changes use `xxx.nu` will read the file and parse it if it exports submodules or uses submodules # Tests + Formatting Done --------- Co-authored-by: Jakub Žádník --- crates/nu-parser/src/lib.rs | 3 +- crates/nu-parser/src/parse_keywords.rs | 71 +++++++- crates/nu-parser/src/parser.rs | 2 +- crates/nu-protocol/src/lib.rs | 1 + crates/nu-protocol/src/module.rs | 35 +++- .../src/parser_path.rs | 2 +- tests/modules/mod.rs | 161 +++++++++++++++++- 7 files changed, 258 insertions(+), 17 deletions(-) rename crates/{nu-parser => nu-protocol}/src/parser_path.rs (98%) diff --git a/crates/nu-parser/src/lib.rs b/crates/nu-parser/src/lib.rs index eeb6a590b8..89cba8d243 100644 --- a/crates/nu-parser/src/lib.rs +++ b/crates/nu-parser/src/lib.rs @@ -8,7 +8,6 @@ mod parse_keywords; mod parse_patterns; mod parse_shape_specs; mod parser; -mod parser_path; mod type_check; pub use deparse::{escape_for_script_arg, escape_quote_string}; @@ -18,8 +17,8 @@ pub use flatten::{ pub use known_external::KnownExternal; pub use lex::{lex, lex_signature, Token, TokenContents}; pub use lite_parser::{lite_parse, LiteBlock, LiteCommand}; +pub use nu_protocol::parser_path::*; pub use parse_keywords::*; -pub use parser_path::*; pub use parser::{ is_math_expression_like, parse, parse_block, parse_expression, parse_external_call, diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 6d3862db37..70d217c564 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -2,7 +2,6 @@ use crate::{ exportable::Exportable, parse_block, parser::{parse_redirection, redirecting_builtin_error}, - parser_path::ParserPath, type_check::{check_block_input_output, type_compatible}, }; use itertools::Itertools; @@ -15,6 +14,7 @@ use nu_protocol::{ }, engine::{StateWorkingSet, DEFAULT_OVERLAY_NAME}, eval_const::eval_constant, + parser_path::ParserPath, Alias, BlockId, DeclId, Module, ModuleId, ParseError, PositionalArg, ResolvedImportPattern, Span, Spanned, SyntaxShape, Type, Value, VarId, }; @@ -1204,7 +1204,7 @@ pub fn parse_export_in_block( "export alias" => parse_alias(working_set, lite_command, None), "export def" => parse_def(working_set, lite_command, None).0, "export const" => parse_const(working_set, &lite_command.parts[1..]), - "export use" => parse_use(working_set, lite_command).0, + "export use" => parse_use(working_set, lite_command, None).0, "export module" => parse_module(working_set, lite_command, None).0, "export extern" => parse_extern(working_set, lite_command, None), _ => { @@ -1223,6 +1223,7 @@ pub fn parse_export_in_module( working_set: &mut StateWorkingSet, lite_command: &LiteCommand, module_name: &[u8], + parent_module: &mut Module, ) -> (Pipeline, Vec) { let spans = &lite_command.parts[..]; @@ -1428,7 +1429,8 @@ pub fn parse_export_in_module( pipe: lite_command.pipe, redirection: lite_command.redirection.clone(), }; - let (pipeline, exportables) = parse_use(working_set, &lite_command); + let (pipeline, exportables) = + parse_use(working_set, &lite_command, Some(parent_module)); let export_use_decl_id = if let Some(id) = working_set.find_decl(b"export use") { id @@ -1771,7 +1773,7 @@ pub fn parse_module_block( )) } b"use" => { - let (pipeline, _) = parse_use(working_set, command); + let (pipeline, _) = parse_use(working_set, command, Some(&mut module)); block.pipelines.push(pipeline) } @@ -1786,7 +1788,7 @@ pub fn parse_module_block( } b"export" => { let (pipe, exportables) = - parse_export_in_module(working_set, command, module_name); + parse_export_in_module(working_set, command, module_name, &mut module); for exportable in exportables { match exportable { @@ -1896,6 +1898,48 @@ pub fn parse_module_block( (block, module, module_comments) } +fn module_needs_reloading(working_set: &StateWorkingSet, module_id: ModuleId) -> bool { + let module = working_set.get_module(module_id); + + fn submodule_need_reloading(working_set: &StateWorkingSet, submodule_id: ModuleId) -> bool { + let submodule = working_set.get_module(submodule_id); + let submodule_changed = if let Some((file_path, file_id)) = &submodule.file { + let existing_contents = working_set.get_contents_of_file(*file_id); + let file_contents = file_path.read(working_set); + + if let (Some(existing), Some(new)) = (existing_contents, file_contents) { + existing != new + } else { + false + } + } else { + false + }; + + if submodule_changed { + true + } else { + module_needs_reloading(working_set, submodule_id) + } + } + + let export_submodule_changed = module + .submodules + .iter() + .any(|(_, submodule_id)| submodule_need_reloading(working_set, *submodule_id)); + + if export_submodule_changed { + return true; + } + + let private_submodule_changed = module + .imported_modules + .iter() + .any(|submodule_id| submodule_need_reloading(working_set, *submodule_id)); + + private_submodule_changed +} + /// Parse a module from a file. /// /// The module name is inferred from the stem of the file, unless specified in `name_override`. @@ -1934,23 +1978,26 @@ fn parse_module_file( // Check if we've parsed the module before. if let Some(module_id) = working_set.find_module_by_span(new_span) { - return Some(module_id); + if !module_needs_reloading(working_set, module_id) { + return Some(module_id); + } } // Add the file to the stack of files being processed. - if let Err(e) = working_set.files.push(path.path_buf(), path_span) { + if let Err(e) = working_set.files.push(path.clone().path_buf(), path_span) { working_set.error(e); return None; } // Parse the module - let (block, module, module_comments) = + let (block, mut module, module_comments) = parse_module_block(working_set, new_span, module_name.as_bytes()); // Remove the file from the stack of files being processed. working_set.files.pop(); let _ = working_set.add_block(Arc::new(block)); + module.file = Some((path, file_id)); let module_id = working_set.add_module(&module_name, module, module_comments); Some(module_id) @@ -2240,6 +2287,7 @@ pub fn parse_module( pub fn parse_use( working_set: &mut StateWorkingSet, lite_command: &LiteCommand, + parent_module: Option<&mut Module>, ) -> (Pipeline, Vec) { let spans = &lite_command.parts; @@ -2385,12 +2433,14 @@ pub fn parse_use( ); }; + let mut imported_modules = vec![]; let (definitions, errors) = module.resolve_import_pattern( working_set, module_id, &import_pattern.members, None, name_span, + &mut imported_modules, ); working_set.parse_errors.extend(errors); @@ -2432,6 +2482,9 @@ pub fn parse_use( import_pattern.constants = constants.iter().map(|(_, id)| *id).collect(); + if let Some(m) = parent_module { + m.track_imported_modules(&imported_modules) + } // Extend the current scope with the module's exportables working_set.use_decls(definitions.decls); working_set.use_modules(definitions.modules); @@ -2865,6 +2918,7 @@ pub fn parse_overlay_use(working_set: &mut StateWorkingSet, call: Box) -> &[], Some(final_overlay_name.as_bytes()), call.head, + &mut vec![], ) } else { origin_module.resolve_import_pattern( @@ -2875,6 +2929,7 @@ pub fn parse_overlay_use(working_set: &mut StateWorkingSet, call: Box) -> }], Some(final_overlay_name.as_bytes()), call.head, + &mut vec![], ) } } else { diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 1fef8bc133..f6bbd84f6c 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -5377,7 +5377,7 @@ pub fn parse_builtin_commands( } b"alias" => parse_alias(working_set, lite_command, None), b"module" => parse_module(working_set, lite_command, None).0, - b"use" => parse_use(working_set, lite_command).0, + b"use" => parse_use(working_set, lite_command, None).0, b"overlay" => { if let Some(redirection) = lite_command.redirection.as_ref() { working_set.error(redirecting_builtin_error("overlay", redirection)); diff --git a/crates/nu-protocol/src/lib.rs b/crates/nu-protocol/src/lib.rs index 78e7d40680..9c176953d5 100644 --- a/crates/nu-protocol/src/lib.rs +++ b/crates/nu-protocol/src/lib.rs @@ -11,6 +11,7 @@ mod example; mod id; mod lev_distance; mod module; +pub mod parser_path; mod pipeline; #[cfg(feature = "plugin")] mod plugin; diff --git a/crates/nu-protocol/src/module.rs b/crates/nu-protocol/src/module.rs index ebb6f5621e..6b4aec0c60 100644 --- a/crates/nu-protocol/src/module.rs +++ b/crates/nu-protocol/src/module.rs @@ -1,8 +1,9 @@ use crate::{ - ast::ImportPatternMember, engine::StateWorkingSet, BlockId, DeclId, ModuleId, ParseError, Span, - Value, VarId, + ast::ImportPatternMember, engine::StateWorkingSet, BlockId, DeclId, FileId, ModuleId, + ParseError, Span, Value, VarId, }; +use crate::parser_path::ParserPath; use indexmap::IndexMap; pub struct ResolvedImportPattern { @@ -35,6 +36,8 @@ pub struct Module { pub env_block: Option, // `export-env { ... }` block pub main: Option, // `export def main` pub span: Option, + pub imported_modules: Vec, // use other_module.nu + pub file: Option<(ParserPath, FileId)>, } impl Module { @@ -47,6 +50,8 @@ impl Module { env_block: None, main: None, span: None, + imported_modules: vec![], + file: None, } } @@ -59,6 +64,8 @@ impl Module { env_block: None, main: None, span: Some(span), + imported_modules: vec![], + file: None, } } @@ -82,6 +89,12 @@ impl Module { self.env_block = Some(block_id); } + pub fn track_imported_modules(&mut self, module_id: &[ModuleId]) { + for m in module_id { + self.imported_modules.push(*m) + } + } + pub fn has_decl(&self, name: &[u8]) -> bool { if name == self.name && self.main.is_some() { return true; @@ -90,6 +103,9 @@ impl Module { self.decls.contains_key(name) } + /// Resolve `members` from given module, which is indicated by `self_id` to import. + /// + /// When resolving, all modules are recorded in `imported_modules`. pub fn resolve_import_pattern( &self, working_set: &StateWorkingSet, @@ -97,7 +113,9 @@ impl Module { members: &[ImportPatternMember], name_override: Option<&[u8]>, // name under the module was stored (doesn't have to be the same as self.name) backup_span: Span, + imported_modules: &mut Vec, ) -> (ResolvedImportPattern, Vec) { + imported_modules.push(self_id); let final_name = name_override.unwrap_or(&self.name).to_vec(); let (head, rest) = if let Some((head, rest)) = members.split_first() { @@ -112,8 +130,14 @@ impl Module { let submodule = working_set.get_module(*id); let span = submodule.span.or(self.span).unwrap_or(backup_span); - let (sub_results, sub_errors) = - submodule.resolve_import_pattern(working_set, *id, &[], None, span); + let (sub_results, sub_errors) = submodule.resolve_import_pattern( + working_set, + *id, + &[], + None, + span, + imported_modules, + ); errors.extend(sub_errors); for (sub_name, sub_decl_id) in sub_results.decls { @@ -212,6 +236,7 @@ impl Module { rest, None, self.span.unwrap_or(backup_span), + imported_modules, ) } else { ( @@ -234,6 +259,7 @@ impl Module { &[], None, self.span.unwrap_or(backup_span), + imported_modules, ); decls.extend(sub_results.decls); @@ -287,6 +313,7 @@ impl Module { rest, None, self.span.unwrap_or(backup_span), + imported_modules, ); decls.extend(sub_results.decls); diff --git a/crates/nu-parser/src/parser_path.rs b/crates/nu-protocol/src/parser_path.rs similarity index 98% rename from crates/nu-parser/src/parser_path.rs rename to crates/nu-protocol/src/parser_path.rs index 2d0fbce2a2..4935d9fbe2 100644 --- a/crates/nu-parser/src/parser_path.rs +++ b/crates/nu-protocol/src/parser_path.rs @@ -1,4 +1,4 @@ -use nu_protocol::engine::{StateWorkingSet, VirtualPath}; +use crate::engine::{StateWorkingSet, VirtualPath}; use std::{ ffi::OsStr, path::{Path, PathBuf}, diff --git a/tests/modules/mod.rs b/tests/modules/mod.rs index c556d6fa7a..9aa1282f21 100644 --- a/tests/modules/mod.rs +++ b/tests/modules/mod.rs @@ -1,4 +1,4 @@ -use nu_test_support::fs::Stub::FileWithContentToBeTrimmed; +use nu_test_support::fs::Stub::{FileWithContent, FileWithContentToBeTrimmed}; use nu_test_support::playground::Playground; use nu_test_support::{nu, nu_repl_code}; use pretty_assertions::assert_eq; @@ -760,3 +760,162 @@ fn nested_list_export_works() { let actual = nu!(&inp.join("; ")); assert_eq!(actual.out, "bacon"); } + +#[test] +fn reload_submodules() { + Playground::setup("reload_submodule_changed_file", |dirs, sandbox| { + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export module animals.nu"#), + FileWithContent("animals.nu", "export def cat [] { 'meow'}"), + ]); + + let inp = [ + "use voice.nu", + r#""export def cat [] {'woem'}" | save -f animals.nu"#, + "use voice.nu", + "(voice animals cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + // should also verify something unchanged if `use voice`. + let inp = [ + "use voice.nu", + r#""export def cat [] {'meow'}" | save -f animals.nu"#, + "use voice", + "(voice animals cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + // should also works if we use members directly. + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export module animals.nu"#), + FileWithContent("animals.nu", "export def cat [] { 'meow'}"), + ]); + let inp = [ + "use voice.nu animals cat", + r#""export def cat [] {'woem'}" | save -f animals.nu"#, + "use voice.nu animals cat", + "(cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + }); +} + +#[test] +fn use_submodules() { + Playground::setup("use_submodules", |dirs, sandbox| { + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export use animals.nu"#), + FileWithContent("animals.nu", "export def cat [] { 'meow'}"), + ]); + + let inp = [ + "use voice.nu", + r#""export def cat [] {'woem'}" | save -f animals.nu"#, + "use voice.nu", + "(voice animals cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + // should also verify something unchanged if `use voice`. + let inp = [ + "use voice.nu", + r#""export def cat [] {'meow'}" | save -f animals.nu"#, + "use voice", + "(voice animals cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + // also verify something is changed when using members. + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export use animals.nu cat"#), + FileWithContent("animals.nu", "export def cat [] { 'meow'}"), + ]); + let inp = [ + "use voice.nu", + r#""export def cat [] {'woem'}" | save -f animals.nu"#, + "use voice.nu", + "(voice cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export use animals.nu *"#), + FileWithContent("animals.nu", "export def cat [] { 'meow'}"), + ]); + let inp = [ + "use voice.nu", + r#""export def cat [] {'woem'}" | save -f animals.nu"#, + "use voice.nu", + "(voice cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export use animals.nu [cat]"#), + FileWithContent("animals.nu", "export def cat [] { 'meow'}"), + ]); + let inp = [ + "use voice.nu", + r#""export def cat [] {'woem'}" | save -f animals.nu"#, + "use voice.nu", + "(voice cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + }); +} + +#[test] +fn use_nested_submodules() { + Playground::setup("use_submodules", |dirs, sandbox| { + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export use animals.nu"#), + FileWithContent("animals.nu", r#"export use nested_animals.nu"#), + FileWithContent("nested_animals.nu", "export def cat [] { 'meow'}"), + ]); + let inp = [ + "use voice.nu", + r#""export def cat [] {'woem'}" | save -f nested_animals.nu"#, + "use voice.nu", + "(voice animals nested_animals cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + sandbox.with_files(&[ + FileWithContent("voice.nu", r#"export use animals.nu"#), + FileWithContent("animals.nu", r#"export use nested_animals.nu cat"#), + FileWithContent("nested_animals.nu", "export def cat [] { 'meow'}"), + ]); + let inp = [ + "use voice.nu", + r#""export def cat [] {'woem'}" | save -f nested_animals.nu"#, + "use voice.nu", + "(voice animals cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + + sandbox.with_files(&[ + FileWithContent("animals.nu", r#"export use nested_animals.nu cat"#), + FileWithContent("nested_animals.nu", "export def cat [] { 'meow' }"), + ]); + let inp = [ + "module voice { export module animals.nu }", + "use voice", + r#""export def cat [] {'woem'}" | save -f nested_animals.nu"#, + "use voice.nu", + "(voice animals cat) == 'woem'", + ]; + let actual = nu!(cwd: dirs.test(), nu_repl_code(&inp)); + assert_eq!(actual.out, "true"); + }) +}