Merge branch 'main' into no_export_env

crates/nu-command/src/core_commands/overlay/list.rs

@@ -4,8 +4,6 @@ use nu_protocol::{
     Category, Example, IntoPipelineData, PipelineData, ShellError, Signature, Span, Value,
 };
 
-use log::trace;
-
 #[derive(Clone)]
 pub struct OverlayList;
 
@@ -28,41 +26,17 @@ impl Command for OverlayList {
 
     fn run(
         &self,
-        engine_state: &EngineState,
+        _engine_state: &EngineState,
         stack: &mut Stack,
         call: &Call,
         _input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
-        let active_overlays_parser: Vec<Value> = engine_state
-            .active_overlay_names(&[])
-            .iter()
-            .map(|s| Value::string(String::from_utf8_lossy(s), call.head))
-            .collect();
-
         let active_overlays_engine: Vec<Value> = stack
             .active_overlays
             .iter()
             .map(|s| Value::string(s, call.head))
             .collect();
 
-        // Check if the overlays in the engine match the overlays in the parser
-        if (active_overlays_parser.len() != active_overlays_engine.len())
-            || active_overlays_parser
-                .iter()
-                .zip(active_overlays_engine.iter())
-                .any(|(op, oe)| op != oe)
-        {
-            trace!("parser overlays: {:?}", active_overlays_parser);
-            trace!("engine overlays: {:?}", active_overlays_engine);
-
-            return Err(ShellError::NushellFailedSpannedHelp(
-                "Overlay mismatch".into(),
-                "Active overlays do not match between the engine and the parser.".into(),
-                call.head,
-                "Run Nushell with --log-level=trace to see what went wrong.".into(),
-            ));
-        }
-
         Ok(Value::List {
             vals: active_overlays_engine,
             span: call.head,

crates/nu-command/src/core_commands/overlay/use_.rs

@@ -1,5 +1,5 @@
 use nu_engine::{eval_block, find_in_dirs_env, redirect_env, CallExt};
-use nu_protocol::ast::Call;
+use nu_protocol::ast::{Call, Expr};
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::{
     Category, Example, PipelineData, ShellError, Signature, Spanned, SyntaxShape, Value,
@@ -57,12 +57,29 @@ impl Command for OverlayUse {
     ) -> Result<PipelineData, ShellError> {
         let name_arg: Spanned<String> = call.req(engine_state, caller_stack, 0)?;
 
-        let (overlay_name, overlay_name_span) = if let Some(kw_expression) = call.positional_nth(1)
+        let origin_module_id = if let Some(overlay_expr) = call.positional_nth(0) {
-        {
+            if let Expr::Overlay(module_id) = overlay_expr.expr {
+                module_id
+            } else {
+                return Err(ShellError::NushellFailedSpanned(
+                    "Not an overlay".to_string(),
+                    "requires an overlay (path or a string)".to_string(),
+                    overlay_expr.span,
+                ));
+            }
+        } else {
+            return Err(ShellError::NushellFailedSpanned(
+                "Missing positional".to_string(),
+                "missing required overlay".to_string(),
+                call.head,
+            ));
+        };
+
+        let overlay_name = if let Some(kw_expression) = call.positional_nth(1) {
             // If renamed via the 'as' keyword, use the new name as the overlay name
             if let Some(new_name_expression) = kw_expression.as_keyword() {
                 if let Some(new_name) = new_name_expression.as_string() {
-                    (new_name, new_name_expression.span)
+                    new_name
                 } else {
                     return Err(ShellError::NushellFailedSpanned(
                         "Wrong keyword type".to_string(),
@@ -81,10 +98,10 @@ impl Command for OverlayUse {
             .find_overlay(name_arg.item.as_bytes())
             .is_some()
         {
-            (name_arg.item.clone(), name_arg.span)
+            name_arg.item.clone()
         } else if let Some(os_str) = Path::new(&name_arg.item).file_stem() {
             if let Some(name) = os_str.to_str() {
-                (name.to_string(), name_arg.span)
+                name.to_string()
             } else {
                 return Err(ShellError::NonUtf8(name_arg.span));
             }
@@ -95,87 +112,74 @@ impl Command for OverlayUse {
             ));
         };
 
-        if let Some(overlay_id) = engine_state.find_overlay(overlay_name.as_bytes()) {
+        caller_stack.add_overlay(overlay_name);
-            let old_module_id = engine_state.get_overlay(overlay_id).origin;
 
-            caller_stack.add_overlay(overlay_name.clone());
+        if let Some(module_id) = origin_module_id {
+            // Add environment variables only if:
+            // a) adding a new overlay
+            // b) refreshing an active overlay (the origin module changed)
 
-            if let Some(new_module_id) = engine_state.find_module(overlay_name.as_bytes(), &[]) {
+            let module = engine_state.get_module(module_id);
-                if !caller_stack.has_env_overlay(&overlay_name, engine_state)
-                    || (old_module_id != new_module_id)
-                {
-                    // Add environment variables only if:
-                    // a) adding a new overlay
-                    // b) refreshing an active overlay (the origin module changed)
-                    let module = engine_state.get_module(new_module_id);
 
-                    for (name, block_id) in module.env_vars() {
+            for (name, block_id) in module.env_vars() {
-                        let name = if let Ok(s) = String::from_utf8(name.clone()) {
+                let name = if let Ok(s) = String::from_utf8(name.clone()) {
-                            s
+                    s
-                        } else {
+                } else {
-                            return Err(ShellError::NonUtf8(call.head));
+                    return Err(ShellError::NonUtf8(call.head));
-                        };
+                };
 
-                        let block = engine_state.get_block(block_id);
+                let block = engine_state.get_block(block_id);
 
-                        let val = eval_block(
+                let val = eval_block(
-                            engine_state,
+                    engine_state,
-                            caller_stack,
+                    caller_stack,
-                            block,
+                    block,
-                            PipelineData::new(call.head),
+                    PipelineData::new(call.head),
-                            false,
+                    false,
-                            true,
+                    true,
-                        )?
+                )?
-                        .into_value(call.head);
+                .into_value(call.head);
 
-                        caller_stack.add_env_var(name, val);
+                caller_stack.add_env_var(name, val);
-                    }
+            }
 
-                    // Evaluate the export-env block (if any) and keep its environment
+            // Evaluate the export-env block (if any) and keep its environment
-                    if let Some(block_id) = module.env_block {
+            if let Some(block_id) = module.env_block {
-                        let maybe_path =
+                let maybe_path = find_in_dirs_env(&name_arg.item, engine_state, caller_stack)?;
-                            find_in_dirs_env(&name_arg.item, engine_state, caller_stack)?;
 
-                        if let Some(path) = &maybe_path {
+                if let Some(path) = &maybe_path {
-                            // Set the currently evaluated directory, if the argument is a valid path
+                    // Set the currently evaluated directory, if the argument is a valid path
-                            let mut parent = path.clone();
+                    let mut parent = path.clone();
-                            parent.pop();
+                    parent.pop();
 
-                            let file_pwd = Value::String {
+                    let file_pwd = Value::String {
-                                val: parent.to_string_lossy().to_string(),
+                        val: parent.to_string_lossy().to_string(),
-                                span: call.head,
+                        span: call.head,
-                            };
+                    };
 
-                            caller_stack.add_env_var("FILE_PWD".to_string(), file_pwd);
+                    caller_stack.add_env_var("FILE_PWD".to_string(), file_pwd);
-                        }
+                }
 
-                        let block = engine_state.get_block(block_id);
+                let block = engine_state.get_block(block_id);
-                        let mut callee_stack = caller_stack.gather_captures(&block.captures);
+                let mut callee_stack = caller_stack.gather_captures(&block.captures);
 
-                        let _ = eval_block(
+                let _ = eval_block(
-                            engine_state,
+                    engine_state,
-                            &mut callee_stack,
+                    &mut callee_stack,
-                            block,
+                    block,
-                            input,
+                    input,
-                            call.redirect_stdout,
+                    call.redirect_stdout,
-                            call.redirect_stderr,
+                    call.redirect_stderr,
-                        );
+                );
 
-                        // Merge the block's environment to the current stack
+                // Merge the block's environment to the current stack
-                        redirect_env(engine_state, caller_stack, &callee_stack);
+                redirect_env(engine_state, caller_stack, &callee_stack);
 
-                        if maybe_path.is_some() {
+                if maybe_path.is_some() {
-                            // Remove the file-relative PWD, if the argument is a valid path
+                    // Remove the file-relative PWD, if the argument is a valid path
-                            caller_stack.remove_env_var(engine_state, "FILE_PWD");
+                    caller_stack.remove_env_var(engine_state, "FILE_PWD");
-                        }
-                    }
                 }
             }
-        } else {
-            return Err(ShellError::OverlayNotFoundAtRuntime(
-                overlay_name,
-                overlay_name_span,
-            ));
         }
 
         Ok(PipelineData::new(call.head))

crates/nu-command/src/formats/from/nuon.rs

@@ -265,6 +265,12 @@ fn convert_to_value(
             "imports not supported in nuon".into(),
             expr.span,
         )),
+        Expr::Overlay(..) => Err(ShellError::OutsideSpannedLabeledError(
+            original_text.to_string(),
+            "Error when loading".into(),
+            "overlays not supported in nuon".into(),
+            expr.span,
+        )),
         Expr::Int(val) => Ok(Value::Int { val, span }),
         Expr::Keyword(kw, ..) => Err(ShellError::OutsideSpannedLabeledError(
             original_text.to_string(),

crates/nu-command/tests/commands/source_env.rs

@@ -1,4 +1,5 @@
-use nu_test_support::fs::{AbsolutePath, Stub::FileWithContent};
+use nu_test_support::fs::AbsolutePath;
+use nu_test_support::fs::Stub::{FileWithContent, FileWithContentToBeTrimmed};
 use nu_test_support::nu;
 use nu_test_support::pipeline;
 use nu_test_support::playground::Playground;
@@ -141,3 +142,166 @@ fn sources_unicode_file_in_unicode_dir_with_spaces_2() {
 fn sources_unicode_file_in_non_utf8_dir() {
     // How do I create non-UTF-8 path???
 }
+
+#[test]
+fn can_source_dynamic_path() {
+    Playground::setup("can_source_dynamic_path", |dirs, sandbox| {
+        let foo_file = "foo.nu";
+
+        sandbox.with_files(vec![FileWithContent(&foo_file, "echo foo")]);
+
+        let cmd = format!("let file = `{}`; source-env $file", foo_file);
+        let actual = nu!(cwd: dirs.test(), &cmd);
+
+        assert_eq!(actual.out, "foo");
+    });
+}
+
+#[test]
+fn source_env_eval_export_env() {
+    Playground::setup("source_env_eval_export_env", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "spam.nu",
+            r#"
+                export-env { let-env FOO = 'foo' }
+            "#,
+        )]);
+
+        let inp = &[r#"source-env spam.nu"#, r#"$env.FOO"#];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert_eq!(actual.out, "foo");
+    })
+}
+
+#[test]
+fn source_env_eval_export_env_hide() {
+    Playground::setup("source_env_eval_export_env", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "spam.nu",
+            r#"
+                export-env { hide-env FOO }
+            "#,
+        )]);
+
+        let inp = &[
+            r#"let-env FOO = 'foo'"#,
+            r#"source-env spam.nu"#,
+            r#"$env.FOO"#,
+        ];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert!(actual.err.contains("did you mean"));
+    })
+}
+
+#[test]
+fn source_env_do_cd() {
+    Playground::setup("source_env_do_cd", |dirs, sandbox| {
+        sandbox
+            .mkdir("test1/test2")
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "test1/test2/spam.nu",
+                r#"
+                    cd test1/test2
+                "#,
+            )]);
+
+        let inp = &[
+            r#"source-env test1/test2/spam.nu"#,
+            r#"$env.PWD | path basename"#,
+        ];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert_eq!(actual.out, "test2");
+    })
+}
+
+#[test]
+fn source_env_do_cd_file_relative() {
+    Playground::setup("source_env_do_cd_file_relative", |dirs, sandbox| {
+        sandbox
+            .mkdir("test1/test2")
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "test1/test2/spam.nu",
+                r#"
+                    cd ($env.FILE_PWD | path join '..')
+                "#,
+            )]);
+
+        let inp = &[
+            r#"source-env test1/test2/spam.nu"#,
+            r#"$env.PWD | path basename"#,
+        ];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert_eq!(actual.out, "test1");
+    })
+}
+
+#[test]
+fn source_env_dont_cd_overlay() {
+    Playground::setup("source_env_dont_cd_overlay", |dirs, sandbox| {
+        sandbox
+            .mkdir("test1/test2")
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "test1/test2/spam.nu",
+                r#"
+                    overlay new spam
+                    cd test1/test2
+                    overlay hide spam
+                "#,
+            )]);
+
+        let inp = &[
+            r#"source-env test1/test2/spam.nu"#,
+            r#"$env.PWD | path basename"#,
+        ];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert_eq!(actual.out, "source_env_dont_cd_overlay");
+    })
+}
+
+#[test]
+fn source_env_nice_parse_error() {
+    Playground::setup("source_env_nice_parse_error", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "spam.nu",
+            r#"
+                let x
+            "#,
+        )]);
+
+        let inp = &[r#"source-env spam.nu"#];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert!(actual.err.contains("cannot parse this file"));
+        assert!(actual.err.contains("───"));
+    })
+}
+
+#[test]
+fn source_env_nice_shell_error() {
+    Playground::setup("source_env_nice_shell_error", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "spam.nu",
+            r#"
+                let-env FILE_PWD = 'foo'
+            "#,
+        )]);
+
+        let inp = &[r#"source-env spam.nu"#];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert!(actual.err.contains("cannot evaluate this file"));
+        assert!(actual.err.contains("───"));
+    })
+}

crates/nu-engine/src/eval.rs

@@ -355,6 +355,15 @@ pub fn eval_expression(
             value.follow_cell_path(&cell_path.tail, false)
         }
         Expr::ImportPattern(_) => Ok(Value::Nothing { span: expr.span }),
+        Expr::Overlay(_) => {
+            let name =
+                String::from_utf8_lossy(engine_state.get_span_contents(&expr.span)).to_string();
+
+            Ok(Value::String {
+                val: name,
+                span: expr.span,
+            })
+        }
         Expr::Call(call) => {
             // FIXME: protect this collect with ctrl-c
             Ok(

crates/nu-parser/src/errors.rs

@@ -120,6 +120,10 @@ pub enum ParseError {
     )]
     ModuleNotFound(#[label = "module not found"] Span),
 
+    #[error("Cyclical module import.")]
+    #[diagnostic(code(nu::parser::cyclical_module_import), url(docsrs), help("{0}"))]
+    CyclicalModuleImport(String, #[label = "detected cyclical module import"] Span),
+
     #[error("Active overlay not found.")]
     #[diagnostic(code(nu::parser::active_overlay_not_found), url(docsrs))]
     ActiveOverlayNotFound(#[label = "not an active overlay"] Span),
@@ -341,6 +345,7 @@ impl ParseError {
             ParseError::VariableNotFound(s) => *s,
             ParseError::VariableNotValid(s) => *s,
             ParseError::ModuleNotFound(s) => *s,
+            ParseError::CyclicalModuleImport(_, s) => *s,
             ParseError::ModuleOrOverlayNotFound(s) => *s,
             ParseError::ActiveOverlayNotFound(s) => *s,
             ParseError::OverlayPrefixMismatch(_, _, s) => *s,

crates/nu-parser/src/flatten.rs

@@ -260,6 +260,9 @@ pub fn flatten_expression(
 
             output
         }
+        Expr::Overlay(_) => {
+            vec![(expr.span, FlatShape::String)]
+        }
         Expr::Range(from, next, to, op) => {
             let mut output = vec![];
             if let Some(f) = from {

crates/nu-parser/src/parse_keywords.rs

@@ -1571,8 +1571,8 @@ pub fn parse_use(
         if let Some(module_id) = working_set.find_module(&import_pattern.head.name) {
             (import_pattern, working_set.get_module(module_id).clone())
         } else {
-            // TODO: Do not close over when loading module from file?
             // It could be a file
+            // TODO: Do not close over when loading module from file?
 
             let (module_filename, err) =
                 unescape_unquote_string(&import_pattern.head.name, import_pattern.head.span);
@@ -1581,6 +1581,37 @@ pub fn parse_use(
                 if let Some(module_path) =
                     find_in_dirs(&module_filename, working_set, &cwd, LIB_DIRS_ENV)
                 {
+                    if let Some(i) = working_set
+                        .parsed_module_files
+                        .iter()
+                        .rposition(|p| p == &module_path)
+                    {
+                        let mut files: Vec<String> = working_set
+                            .parsed_module_files
+                            .split_off(i)
+                            .iter()
+                            .map(|p| p.to_string_lossy().to_string())
+                            .collect();
+
+                        files.push(module_path.to_string_lossy().to_string());
+
+                        let msg = files.join("\nuses ");
+
+                        return (
+                            Pipeline::from_vec(vec![Expression {
+                                expr: Expr::Call(call),
+                                span: call_span,
+                                ty: Type::Any,
+                                custom_completion: None,
+                            }]),
+                            vec![],
+                            Some(ParseError::CyclicalModuleImport(
+                                msg,
+                                import_pattern.head.span,
+                            )),
+                        );
+                    }
+
                     let module_name = if let Some(stem) = module_path.file_stem() {
                         stem.to_string_lossy().to_string()
                     } else {
@@ -1601,7 +1632,7 @@ pub fn parse_use(
                         working_set.add_file(module_filename, &contents);
                         let span_end = working_set.next_span_start();
 
-                        // Change currently parsed directory
+                        // Change the currently parsed directory
                         let prev_currently_parsed_cwd = if let Some(parent) = module_path.parent() {
                             let prev = working_set.currently_parsed_cwd.clone();
 
@@ -1612,6 +1643,10 @@ pub fn parse_use(
                             working_set.currently_parsed_cwd.clone()
                         };
 
+                        // Add the file to the stack of parsed module files
+                        working_set.parsed_module_files.push(module_path);
+
+                        // Parse the module
                         let (block, module, err) = parse_module_block(
                             working_set,
                             Span::new(span_start, span_end),
@@ -1619,6 +1654,9 @@ pub fn parse_use(
                         );
                         error = error.or(err);
 
+                        // Remove the file from the stack of parsed module files
+                        working_set.parsed_module_files.pop();
+
                         // Restore the currently parsed directory back
                         working_set.currently_parsed_cwd = prev_currently_parsed_cwd;
 
@@ -2333,7 +2371,7 @@ pub fn parse_overlay_use(
     let has_prefix = call.has_flag("prefix");
 
     let pipeline = Pipeline::from_vec(vec![Expression {
-        expr: Expr::Call(call),
+        expr: Expr::Call(call.clone()),
         span: span(spans),
         ty: Type::Any,
         custom_completion: None,
@@ -2343,7 +2381,14 @@ pub fn parse_overlay_use(
 
     let mut error = None;
 
-    let result = if let Some(overlay_frame) = working_set.find_overlay(overlay_name.as_bytes()) {
+    let (final_overlay_name, origin_module, origin_module_id, is_module_updated) = if let Some(
+        overlay_frame,
+    ) =
+        working_set.find_overlay(overlay_name.as_bytes())
+    {
+        // Activate existing overlay
+
+        // First, check for errors
         if has_prefix && !overlay_frame.prefixed {
             return (
                 pipeline,
@@ -2378,22 +2423,22 @@ pub fn parse_overlay_use(
             }
         }
 
-        // Activate existing overlay
         let module_id = overlay_frame.origin;
 
         if let Some(new_module_id) = working_set.find_module(overlay_name.as_bytes()) {
             if module_id == new_module_id {
-                Some((overlay_name, Module::new(), module_id))
+                (overlay_name, Module::new(), module_id, false)
             } else {
                 // The origin module of an overlay changed => update it
-                Some((
+                (
                     overlay_name,
                     working_set.get_module(new_module_id).clone(),
                     new_module_id,
-                ))
+                    true,
+                )
             }
         } else {
-            Some((overlay_name, Module::new(), module_id))
+            (overlay_name, Module::new(), module_id, true)
         }
     } else {
         // Create a new overlay from a module
@@ -2401,11 +2446,12 @@ pub fn parse_overlay_use(
             // the name is a module
             working_set.find_module(overlay_name.as_bytes())
         {
-            Some((
+            (
                 new_name.map(|spanned| spanned.item).unwrap_or(overlay_name),
                 working_set.get_module(module_id).clone(),
                 module_id,
-            ))
+                true,
+            )
         } else {
             // try if the name is a file
             if let Ok(module_filename) =
@@ -2452,11 +2498,12 @@ pub fn parse_overlay_use(
                         let _ = working_set.add_block(block);
                         let module_id = working_set.add_module(&overlay_name, module.clone());
 
-                        Some((
+                        (
                             new_name.map(|spanned| spanned.item).unwrap_or(overlay_name),
                             module,
                             module_id,
-                        ))
+                            true,
+                        )
                     } else {
                         return (
                             pipeline,
@@ -2464,8 +2511,10 @@ pub fn parse_overlay_use(
                         );
                     }
                 } else {
-                    error = error.or(Some(ParseError::ModuleOrOverlayNotFound(overlay_name_span)));
+                    return (
-                    None
+                        pipeline,
+                        Some(ParseError::ModuleOrOverlayNotFound(overlay_name_span)),
+                    );
                 }
             } else {
                 return (garbage_pipeline(spans), Some(ParseError::NonUtf8(spans[1])));
@@ -2473,24 +2522,39 @@ pub fn parse_overlay_use(
         }
     };
 
-    if let Some((name, module, module_id)) = result {
+    let (decls_to_lay, aliases_to_lay) = if has_prefix {
-        let (decls_to_lay, aliases_to_lay) = if has_prefix {
+        (
-            (
+            origin_module.decls_with_head(final_overlay_name.as_bytes()),
-                module.decls_with_head(name.as_bytes()),
+            origin_module.aliases_with_head(final_overlay_name.as_bytes()),
-                module.aliases_with_head(name.as_bytes()),
+        )
-            )
+    } else {
-        } else {
+        (origin_module.decls(), origin_module.aliases())
-            (module.decls(), module.aliases())
+    };
-        };
 
-        working_set.add_overlay(
+    working_set.add_overlay(
-            name.as_bytes().to_vec(),
+        final_overlay_name.as_bytes().to_vec(),
-            module_id,
+        origin_module_id,
-            decls_to_lay,
+        decls_to_lay,
-            aliases_to_lay,
+        aliases_to_lay,
-            has_prefix,
+        has_prefix,
-        );
+    );
-    }
+
+    // Change the call argument to include the Overlay expression with the module ID
+    let mut call = call;
+    if let Some(overlay_expr) = call.positional_nth_mut(0) {
+        overlay_expr.expr = Expr::Overlay(if is_module_updated {
+            Some(origin_module_id)
+        } else {
+            None
+        });
+    } // no need to check for else since it was already checked
+
+    let pipeline = Pipeline::from_vec(vec![Expression {
+        expr: Expr::Call(call),
+        span: span(spans),
+        ty: Type::Any,
+        custom_completion: None,
+    }]);
 
     (pipeline, error)
 }

crates/nu-parser/src/parser.rs

@@ -5221,6 +5221,7 @@ pub fn discover_captures_in_expr(
             output.extend(&result);
         }
         Expr::ImportPattern(_) => {}
+        Expr::Overlay(_) => {}
         Expr::Garbage => {}
         Expr::Nothing => {}
         Expr::GlobPattern(_) => {}

crates/nu-protocol/src/ast/call.rs

@@ -82,6 +82,10 @@ impl Call {
         self.positional_iter().nth(i)
     }
 
+    pub fn positional_nth_mut(&mut self, i: usize) -> Option<&mut Expression> {
+        self.positional_iter_mut().nth(i)
+    }
+
     pub fn positional_len(&self) -> usize {
         self.positional_iter().count()
     }

crates/nu-protocol/src/ast/expr.rs

@@ -39,6 +39,7 @@ pub enum Expr {
     CellPath(CellPath),
     FullCellPath(Box<FullCellPath>),
     ImportPattern(ImportPattern),
+    Overlay(Option<BlockId>), // block ID of the overlay's origin module
     Signature(Box<Signature>),
     StringInterpolation(Vec<Expression>),
     Nothing,

crates/nu-protocol/src/ast/expression.rs

@@ -169,6 +169,7 @@ impl Expression {
                 false
             }
             Expr::ImportPattern(_) => false,
+            Expr::Overlay(_) => false,
             Expr::Filepath(_) => false,
             Expr::Directory(_) => false,
             Expr::Float(_) => false,
@@ -337,6 +338,7 @@ impl Expression {
                     .replace_in_variable(working_set, new_var_id);
             }
             Expr::ImportPattern(_) => {}
+            Expr::Overlay(_) => {}
             Expr::Garbage => {}
             Expr::Nothing => {}
             Expr::GlobPattern(_) => {}
@@ -485,6 +487,7 @@ impl Expression {
                     .replace_span(working_set, replaced, new_span);
             }
             Expr::ImportPattern(_) => {}
+            Expr::Overlay(_) => {}
             Expr::Garbage => {}
             Expr::Nothing => {}
             Expr::GlobPattern(_) => {}

crates/nu-protocol/src/engine/engine_state.rs

@@ -777,6 +777,8 @@ pub struct StateWorkingSet<'a> {
     pub type_scope: TypeScope,
     /// Current working directory relative to the file being parsed right now
     pub currently_parsed_cwd: Option<PathBuf>,
+    /// All previously parsed module files. Used to protect against circular imports.
+    pub parsed_module_files: Vec<PathBuf>,
 }
 
 /// A temporary placeholder for expression types. It is used to keep track of the input types
@@ -952,6 +954,7 @@ impl<'a> StateWorkingSet<'a> {
             external_commands: vec![],
             type_scope: TypeScope::default(),
             currently_parsed_cwd: None,
+            parsed_module_files: vec![],
         }
     }
 

crates/nu-protocol/src/engine/stack.rs

@@ -379,8 +379,6 @@ impl Stack {
     }
 
     pub fn add_overlay(&mut self, name: String) {
-        self.env_hidden.remove(&name);
-
         self.active_overlays.retain(|o| o != &name);
         self.active_overlays.push(name);
     }

tests/modules/mod.rs

@@ -341,3 +341,95 @@ fn module_import_env_2() {
         assert_eq!(actual.out, "foo");
     })
 }
+
+#[test]
+fn module_cyclical_imports_0() {
+    Playground::setup("module_cyclical_imports_0", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "spam.nu",
+            r#"
+                    use eggs.nu
+                "#,
+        )]);
+
+        let inp = &[r#"module eggs { use spam.nu }"#];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert!(actual.err.contains("module not found"));
+    })
+}
+
+#[test]
+fn module_cyclical_imports_1() {
+    Playground::setup("module_cyclical_imports_1", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "spam.nu",
+            r#"
+                    use spam.nu
+                "#,
+        )]);
+
+        let inp = &[r#"use spam.nu"#];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert!(actual.err.contains("cyclical"));
+    })
+}
+
+#[test]
+fn module_cyclical_imports_2() {
+    Playground::setup("module_cyclical_imports_2", |dirs, sandbox| {
+        sandbox
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "spam.nu",
+                r#"
+                    use eggs.nu
+                "#,
+            )])
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "eggs.nu",
+                r#"
+                    use spam.nu
+                "#,
+            )]);
+
+        let inp = &[r#"use spam.nu"#];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert!(actual.err.contains("cyclical"));
+    })
+}
+
+#[test]
+fn module_cyclical_imports_3() {
+    Playground::setup("module_cyclical_imports_3", |dirs, sandbox| {
+        sandbox
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "spam.nu",
+                r#"
+                    use eggs.nu
+                "#,
+            )])
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "eggs.nu",
+                r#"
+                    use bacon.nu
+                "#,
+            )])
+            .with_files(vec![FileWithContentToBeTrimmed(
+                "bacon.nu",
+                r#"
+                    use spam.nu
+                "#,
+            )]);
+
+        let inp = &[r#"use spam.nu"#];
+
+        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+
+        assert!(actual.err.contains("cyclical"));
+    })
+}

tests/overlays/mod.rs

@@ -238,6 +238,24 @@ fn update_overlay_from_module_env() {
     assert_eq!(actual_repl.out, "bar");
 }
 
+#[test]
+fn overlay_use_do_not_eval_twice() {
+    let inp = &[
+        r#"module spam { export env FOO { "foo" } }"#,
+        r#"overlay use spam"#,
+        r#"let-env FOO = "bar""#,
+        r#"overlay hide spam"#,
+        r#"overlay use spam"#,
+        r#"$env.FOO"#,
+    ];
+
+    let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; ")));
+    let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
+
+    assert_eq!(actual.out, "bar");
+    assert_eq!(actual_repl.out, "bar");
+}
+
 #[test]
 fn remove_overlay() {
     let inp = &[
@@ -843,23 +861,120 @@ fn overlay_use_dont_cd_overlay() {
     })
 }
 
-#[ignore]
 #[test]
-fn overlay_use_find_module_scoped() {
+fn overlay_use_find_scoped_module() {
     Playground::setup("overlay_use_find_module_scoped", |dirs, _| {
-        let inp = &[r#"
+        let inp = r#"
                 do {
-                    module spam { export def foo [] { 'foo' } }
+                    module spam { }
 
                     overlay use spam
-                    foo
+                    overlay list | last
                 }
-            "#];
+            "#;
 
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
+        let actual = nu!(cwd: dirs.test(), inp);
-        let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
 
-        assert_eq!(actual.out, "foo");
+        assert_eq!(actual.out, "spam");
-        assert_eq!(actual_repl.out, "foo");
     })
 }
+
+#[test]
+fn overlay_preserve_hidden_env_1() {
+    let inp = &[
+        r#"overlay new spam"#,
+        r#"let-env FOO = 'foo'"#,
+        r#"overlay new eggs"#,
+        r#"let-env FOO = 'bar'"#,
+        r#"hide-env FOO"#,
+        r#"overlay use eggs"#,
+        r#"$env.FOO"#,
+    ];
+
+    let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; ")));
+    let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
+
+    assert_eq!(actual.out, "foo");
+    assert_eq!(actual_repl.out, "foo");
+}
+
+#[test]
+fn overlay_preserve_hidden_env_2() {
+    let inp = &[
+        r#"overlay new spam"#,
+        r#"let-env FOO = 'foo'"#,
+        r#"overlay hide spam"#,
+        r#"overlay new eggs"#,
+        r#"let-env FOO = 'bar'"#,
+        r#"hide-env FOO"#,
+        r#"overlay hide eggs"#,
+        r#"overlay use spam"#,
+        r#"overlay use eggs"#,
+        r#"$env.FOO"#,
+    ];
+
+    let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; ")));
+    let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
+
+    assert_eq!(actual.out, "foo");
+    assert_eq!(actual_repl.out, "foo");
+}
+
+#[test]
+fn overlay_reset_hidden_env() {
+    let inp = &[
+        r#"overlay new spam"#,
+        r#"let-env FOO = 'foo'"#,
+        r#"overlay new eggs"#,
+        r#"let-env FOO = 'bar'"#,
+        r#"hide-env FOO"#,
+        r#"module eggs { export-env { let-env FOO = 'bar' } }"#,
+        r#"overlay use eggs"#,
+        r#"$env.FOO"#,
+    ];
+
+    let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; ")));
+    let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
+
+    assert_eq!(actual.out, "bar");
+    assert_eq!(actual_repl.out, "bar");
+}
+
+#[ignore = "TODO: For this to work, we'd need to make predecls respect overlays"]
+#[test]
+fn overlay_preserve_hidden_decl() {
+    let inp = &[
+        r#"overlay new spam"#,
+        r#"def foo [] { 'foo' }"#,
+        r#"overlay new eggs"#,
+        r#"def foo [] { 'bar' }"#,
+        r#"hide foo"#,
+        r#"overlay use eggs"#,
+        r#"foo"#,
+    ];
+
+    let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; ")));
+    let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
+
+    assert_eq!(actual.out, "foo");
+    assert_eq!(actual_repl.out, "foo");
+}
+
+#[test]
+fn overlay_preserve_hidden_alias() {
+    let inp = &[
+        r#"overlay new spam"#,
+        r#"alias foo = 'foo'"#,
+        r#"overlay new eggs"#,
+        r#"alias foo = 'bar'"#,
+        r#"hide foo"#,
+        r#"overlay use eggs"#,
+        r#"foo"#,
+    ];
+
+    let actual = nu!(cwd: "tests/overlays", pipeline(&inp.join("; ")));
+    let actual_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
+
+    assert_eq!(actual.out, "foo");
+    assert_eq!(actual_repl.out, "foo");
+}

tests/shell/environment/mod.rs

@@ -1,5 +1,4 @@
 mod env;
-mod source_env;
 
 // FIXME: nu_env tests depend on autoenv which hasn't been ported yet
 // mod nu_env;

tests/shell/environment/source_env.rs

@@ -1,152 +0,0 @@
-use nu_test_support::fs::Stub::FileWithContentToBeTrimmed;
-use nu_test_support::playground::Playground;
-use nu_test_support::{nu, pipeline};
-
-#[test]
-fn source_env_eval_export_env() {
-    Playground::setup("source_env_eval_export_env", |dirs, sandbox| {
-        sandbox.with_files(vec![FileWithContentToBeTrimmed(
-            "spam.nu",
-            r#"
-                export-env { let-env FOO = 'foo' }
-            "#,
-        )]);
-
-        let inp = &[r#"source-env spam.nu"#, r#"$env.FOO"#];
-
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
-
-        assert_eq!(actual.out, "foo");
-    })
-}
-
-#[test]
-fn source_env_eval_export_env_hide() {
-    Playground::setup("source_env_eval_export_env", |dirs, sandbox| {
-        sandbox.with_files(vec![FileWithContentToBeTrimmed(
-            "spam.nu",
-            r#"
-                export-env { hide-env FOO }
-            "#,
-        )]);
-
-        let inp = &[
-            r#"let-env FOO = 'foo'"#,
-            r#"source-env spam.nu"#,
-            r#"$env.FOO"#,
-        ];
-
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
-
-        assert!(actual.err.contains("did you mean"));
-    })
-}
-
-#[test]
-fn source_env_do_cd() {
-    Playground::setup("source_env_do_cd", |dirs, sandbox| {
-        sandbox
-            .mkdir("test1/test2")
-            .with_files(vec![FileWithContentToBeTrimmed(
-                "test1/test2/spam.nu",
-                r#"
-                    cd test1/test2
-                "#,
-            )]);
-
-        let inp = &[
-            r#"source-env test1/test2/spam.nu"#,
-            r#"$env.PWD | path basename"#,
-        ];
-
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
-
-        assert_eq!(actual.out, "test2");
-    })
-}
-
-#[test]
-fn source_env_do_cd_file_relative() {
-    Playground::setup("source_env_do_cd_file_relative", |dirs, sandbox| {
-        sandbox
-            .mkdir("test1/test2")
-            .with_files(vec![FileWithContentToBeTrimmed(
-                "test1/test2/spam.nu",
-                r#"
-                    cd ($env.FILE_PWD | path join '..')
-                "#,
-            )]);
-
-        let inp = &[
-            r#"source-env test1/test2/spam.nu"#,
-            r#"$env.PWD | path basename"#,
-        ];
-
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
-
-        assert_eq!(actual.out, "test1");
-    })
-}
-
-#[test]
-fn source_env_dont_cd_overlay() {
-    Playground::setup("source_env_dont_cd_overlay", |dirs, sandbox| {
-        sandbox
-            .mkdir("test1/test2")
-            .with_files(vec![FileWithContentToBeTrimmed(
-                "test1/test2/spam.nu",
-                r#"
-                    overlay new spam
-                    cd test1/test2
-                    overlay hide spam
-                "#,
-            )]);
-
-        let inp = &[
-            r#"source-env test1/test2/spam.nu"#,
-            r#"$env.PWD | path basename"#,
-        ];
-
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
-
-        assert_eq!(actual.out, "source_env_dont_cd_overlay");
-    })
-}
-
-#[test]
-fn source_env_nice_parse_error() {
-    Playground::setup("source_env_nice_parse_error", |dirs, sandbox| {
-        sandbox.with_files(vec![FileWithContentToBeTrimmed(
-            "spam.nu",
-            r#"
-                let x
-            "#,
-        )]);
-
-        let inp = &[r#"source-env spam.nu"#];
-
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
-
-        assert!(actual.err.contains("cannot parse this file"));
-        assert!(actual.err.contains("───"));
-    })
-}
-
-#[test]
-fn source_env_nice_shell_error() {
-    Playground::setup("source_env_nice_shell_error", |dirs, sandbox| {
-        sandbox.with_files(vec![FileWithContentToBeTrimmed(
-            "spam.nu",
-            r#"
-                let-env FILE_PWD = 'foo'
-            "#,
-        )]);
-
-        let inp = &[r#"source-env spam.nu"#];
-
-        let actual = nu!(cwd: dirs.test(), pipeline(&inp.join("; ")));
-
-        assert!(actual.err.contains("cannot evaluate this file"));
-        assert!(actual.err.contains("───"));
-    })
-}