Allow recursive module dirs; Require mod.nu in dirs (#9185)

crates/nu-parser/src/parse_keywords.rs

@@ -1825,14 +1825,22 @@ pub fn parse_module_file_or_dir(
                 return None;
             };
 
-            let mut file_paths = vec![];
+            let mod_nu_path = module_path.join("mod.nu");
+
+            if !(mod_nu_path.exists() && mod_nu_path.is_file()) {
+                working_set.error(ParseError::ModuleMissingModNuFile(path_span));
+                return None;
+            }
+
+            let mut paths = vec![];
 
             for entry in dir_contents.flatten() {
                 let entry_path = entry.path();
 
-                if entry_path.is_file()
+                if (entry_path.is_file()
                     && entry_path.extension() == Some(OsStr::new("nu"))
-                    && entry_path.file_stem() != Some(OsStr::new("mod"))
+                    && entry_path.file_stem() != Some(OsStr::new("mod")))
+                    || (entry_path.is_dir() && entry_path.join("mod.nu").exists())
                 {
                     if entry_path.file_stem() == Some(OsStr::new(&module_name)) {
                         working_set.error(ParseError::InvalidModuleFileName(
@@ -1843,64 +1851,54 @@ pub fn parse_module_file_or_dir(
                         return None;
                     }
 
-                    file_paths.push(entry_path);
+                    paths.push(entry_path);
                 }
             }
 
-            file_paths.sort();
+            paths.sort();
 
             // working_set.enter_scope();
 
             let mut submodules = vec![];
 
-            for file_path in file_paths {
-                if let Some(submodule_id) =
-                    parse_module_file(working_set, file_path, path_span, None)
-                {
+            for p in paths {
+                if let Some(submodule_id) = parse_module_file_or_dir(
+                    working_set,
+                    p.to_string_lossy().as_bytes(),
+                    path_span,
+                    None,
+                ) {
                     let submodule_name = working_set.get_module(submodule_id).name();
                     submodules.push((submodule_name, submodule_id));
                 }
             }
 
-            let mod_nu_path = module_path.join("mod.nu");
-
-            if mod_nu_path.exists() && mod_nu_path.is_file() {
-                if let Some(module_id) = parse_module_file(
-                    working_set,
-                    mod_nu_path,
-                    path_span,
-                    name_override.or(Some(module_name)),
-                ) {
-                    let mut module = working_set.get_module(module_id).clone();
-
-                    for (submodule_name, submodule_id) in submodules {
-                        module.add_submodule(submodule_name, submodule_id);
-                    }
-
-                    let module_name = String::from_utf8_lossy(&module.name).to_string();
-
-                    let module_comments =
-                        if let Some(comments) = working_set.get_module_comments(module_id) {
-                            comments.to_vec()
-                        } else {
-                            vec![]
-                        };
-
-                    let new_module_id =
-                        working_set.add_module(&module_name, module, module_comments);
-
-                    Some(new_module_id)
-                } else {
-                    None
-                }
-            } else {
-                let mut module = Module::new(module_name.as_bytes().to_vec());
+            if let Some(module_id) = parse_module_file(
+                working_set,
+                mod_nu_path,
+                path_span,
+                name_override.or(Some(module_name)),
+            ) {
+                let mut module = working_set.get_module(module_id).clone();
 
                 for (submodule_name, submodule_id) in submodules {
                     module.add_submodule(submodule_name, submodule_id);
                 }
 
-                Some(working_set.add_module(&module_name, module, vec![]))
+                let module_name = String::from_utf8_lossy(&module.name).to_string();
+
+                let module_comments =
+                    if let Some(comments) = working_set.get_module_comments(module_id) {
+                        comments.to_vec()
+                    } else {
+                        vec![]
+                    };
+
+                let new_module_id = working_set.add_module(&module_name, module, module_comments);
+
+                Some(new_module_id)
+            } else {
+                None
             }
         } else {
             working_set.error(ParseError::ModuleNotFound(path_span));

crates/nu-protocol/src/parse_error.rs

@@ -193,6 +193,13 @@ pub enum ParseError {
     )]
     ModuleNotFound(#[label = "module not found"] Span),
 
+    #[error("Missing mod.nu file.")]
+    #[diagnostic(
+        code(nu::parser::module_missing_mod_nu_file),
+        help("When importing a directory as a Nushell module, it needs to contain a mod.nu file (can be empty). Alternatively, you can use .nu files in the directory as modules individually.")
+    )]
+    ModuleMissingModNuFile(#[label = "module directory is missing a mod.nu file"] Span),
+
     #[error("Cyclical module import.")]
     #[diagnostic(code(nu::parser::cyclical_module_import), help("{0}"))]
     CyclicalModuleImport(String, #[label = "detected cyclical module import"] Span),
@@ -484,6 +491,7 @@ impl ParseError {
             ParseError::AliasNotValid(s) => *s,
             ParseError::CommandDefNotValid(s) => *s,
             ParseError::ModuleNotFound(s) => *s,
+            ParseError::ModuleMissingModNuFile(s) => *s,
             ParseError::NamedAsModule(_, _, _, s) => *s,
             ParseError::ModuleDoubleMain(_, s) => *s,
             ParseError::InvalidModuleFileName(_, _, s) => *s,

tests/modules/mod.rs

@@ -641,6 +641,27 @@ fn module_dir() {
     assert_eq!(actual.out, "spambaz");
 }
 
+#[test]
+fn module_dir_deep() {
+    let import = "use samples/spam";
+
+    let inp = &[import, "spam bacon"];
+    let actual_repl = nu!(cwd: "tests/modules", pipeline(&inp.join("; ")));
+    assert_eq!(actual_repl.out, "bacon");
+
+    let inp = &[import, "spam bacon foo"];
+    let actual_repl = nu!(cwd: "tests/modules", pipeline(&inp.join("; ")));
+    assert_eq!(actual_repl.out, "bacon foo");
+
+    let inp = &[import, "spam bacon beans"];
+    let actual_repl = nu!(cwd: "tests/modules", pipeline(&inp.join("; ")));
+    assert_eq!(actual_repl.out, "beans");
+
+    let inp = &[import, "spam bacon beans foo"];
+    let actual_repl = nu!(cwd: "tests/modules", pipeline(&inp.join("; ")));
+    assert_eq!(actual_repl.out, "beans foo");
+}
+
 #[test]
 fn module_dir_import_twice_no_panic() {
     let import = "use samples/spam";
@@ -656,6 +677,13 @@ fn not_allowed_submodule_file() {
     assert!(actual.err.contains("invalid_module_file_name"));
 }
 
+#[test]
+fn module_dir_missing_mod_nu() {
+    let inp = &["use samples/missing_mod_nu"];
+    let actual = nu!(cwd: "tests/modules", pipeline(&inp.join("; ")));
+    assert!(actual.err.contains("module_missing_mod_nu_file"));
+}
+
 #[test]
 fn allowed_local_module() {
     let inp = &["module spam { module spam {} }"];

tests/modules/samples/spam/bacon/beans/foo.nu

@@ -0,0 +1 @@
+export def main [] { 'beans foo' }

tests/modules/samples/spam/bacon/beans/mod.nu

@@ -0,0 +1,3 @@
+export def test [] { 'test' }
+
+export def main [] { 'beans' }

tests/modules/samples/spam/bacon/foo.nu

@@ -0,0 +1 @@
+export def main [] { 'bacon foo' }

tests/modules/samples/spam/bacon/mod.nu

@@ -0,0 +1,3 @@
+export def test [] { 'test' }
+
+export def main [] { 'bacon' }