Enable reloading changes to a submodule (#13170)

# 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 <kubouch@gmail.com>

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,

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<Exportable>) {
     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) {
+        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<Exportable>) {
     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<Call>) ->
                 &[],
                 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<Call>) ->
                 }],
                 Some(final_overlay_name.as_bytes()),
                 call.head,
+                &mut vec![],
             )
         }
     } else {

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));

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;

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<BlockId>, // `export-env { ... }` block
     pub main: Option<DeclId>,       // `export def main`
     pub span: Option<Span>,
+    pub imported_modules: Vec<ModuleId>, // 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<ModuleId>,
     ) -> (ResolvedImportPattern, Vec<ParseError>) {
+        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);

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},

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");
+    })
+}