Fix scoped overlay use not finding a module (#6474)

* Add source-env test for dynamic path

* Use correct module ID for env overlay imports

* Remove parser check from "overlay list"

It would cause unnecessary errors from some inner scope if some
overlay module was also defined in some inner scope.

* Restore Cargo.lock back

* Remove comments

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,73 @@ impl Command for OverlayUse {
             ));
         };
 
-        if let Some(overlay_id) = engine_state.find_overlay(overlay_name.as_bytes()) {
-            let old_module_id = engine_state.get_overlay(overlay_id).origin;
+        caller_stack.add_overlay(overlay_name);
 
-            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)
+            let module = engine_state.get_module(module_id);
 
-            if let Some(new_module_id) = engine_state.find_module(overlay_name.as_bytes(), &[]) {
-                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() {
+                let name = if let Ok(s) = String::from_utf8(name.clone()) {
+                    s
+                } else {
+                    return Err(ShellError::NonUtf8(call.head));
+                };
 
-                    for (name, block_id) in module.env_vars() {
-                        let name = if let Ok(s) = String::from_utf8(name.clone()) {
-                            s
-                        } else {
-                            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(
+                    engine_state,
+                    caller_stack,
+                    block,
+                    PipelineData::new(call.head),
+                    false,
+                    true,
+                )?
+                .into_value(call.head);
 
-                        let val = eval_block(
-                            engine_state,
-                            caller_stack,
-                            block,
-                            PipelineData::new(call.head),
-                            false,
-                            true,
-                        )?
-                        .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
+            if let Some(block_id) = module.env_block {
+                let maybe_path = find_in_dirs_env(&name_arg.item, engine_state, caller_stack)?;
 
-                    // Evaluate the export-env block (if any) and keep its environment
-                    if let Some(block_id) = module.env_block {
-                        let maybe_path =
-                            find_in_dirs_env(&name_arg.item, engine_state, caller_stack)?;
+                if let Some(path) = &maybe_path {
+                    // Set the currently evaluated directory, if the argument is a valid path
+                    let mut parent = path.clone();
+                    parent.pop();
 
-                        if let Some(path) = &maybe_path {
-                            // Set the currently evaluated directory, if the argument is a valid path
-                            let mut parent = path.clone();
-                            parent.pop();
+                    let file_pwd = Value::String {
+                        val: parent.to_string_lossy().to_string(),
+                        span: call.head,
+                    };
 
-                            let file_pwd = Value::String {
-                                val: parent.to_string_lossy().to_string(),
-                                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 mut callee_stack = caller_stack.gather_captures(&block.captures);
 
-                        let block = engine_state.get_block(block_id);
-                        let mut callee_stack = caller_stack.gather_captures(&block.captures);
+                let _ = eval_block(
+                    engine_state,
+                    &mut callee_stack,
+                    block,
+                    input,
+                    call.redirect_stdout,
+                    call.redirect_stderr,
+                );
 
-                        let _ = eval_block(
-                            engine_state,
-                            &mut callee_stack,
-                            block,
-                            input,
-                            call.redirect_stdout,
-                            call.redirect_stderr,
-                        );
+                // Merge the block's environment to the current stack
+                redirect_env(engine_state, caller_stack, &callee_stack);
 
-                        // Merge the block's environment to the current stack
-                        redirect_env(engine_state, caller_stack, &callee_stack);
-
-                        if maybe_path.is_some() {
-                            // Remove the file-relative PWD, if the argument is a valid path
-                            caller_stack.remove_env_var(engine_state, "FILE_PWD");
-                        }
-                    }
+                if maybe_path.is_some() {
+                    // Remove the file-relative PWD, if the argument is a valid path
+                    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

@@ -141,3 +141,17 @@ 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");
+    });
+}

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/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

@@ -2422,7 +2422,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,
@@ -2432,7 +2432,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,
@@ -2467,22 +2474,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
@@ -2490,11 +2497,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) =
@@ -2541,11 +2549,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,
@@ -2553,8 +2562,10 @@ pub fn parse_overlay_use(
                         );
                     }
                 } else {
-                    error = error.or(Some(ParseError::ModuleOrOverlayNotFound(overlay_name_span)));
-                    None
+                    return (
+                        pipeline,
+                        Some(ParseError::ModuleOrOverlayNotFound(overlay_name_span)),
+                    );
                 }
             } else {
                 return (garbage_pipeline(spans), Some(ParseError::NonUtf8(spans[1])));
@@ -2562,24 +2573,39 @@ pub fn parse_overlay_use(
         }
     };
 
-    if let Some((name, module, module_id)) = result {
-        let (decls_to_lay, aliases_to_lay) = if has_prefix {
-            (
-                module.decls_with_head(name.as_bytes()),
-                module.aliases_with_head(name.as_bytes()),
-            )
-        } else {
-            (module.decls(), module.aliases())
-        };
+    let (decls_to_lay, aliases_to_lay) = if has_prefix {
+        (
+            origin_module.decls_with_head(final_overlay_name.as_bytes()),
+            origin_module.aliases_with_head(final_overlay_name.as_bytes()),
+        )
+    } else {
+        (origin_module.decls(), origin_module.aliases())
+    };
 
-        working_set.add_overlay(
-            name.as_bytes().to_vec(),
-            module_id,
-            decls_to_lay,
-            aliases_to_lay,
-            has_prefix,
-        );
-    }
+    working_set.add_overlay(
+        final_overlay_name.as_bytes().to_vec(),
+        origin_module_id,
+        decls_to_lay,
+        aliases_to_lay,
+        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(_) => {}

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,20 @@ 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_repl = nu!(cwd: "tests/overlays", nu_repl_code(inp));
+        let actual = nu!(cwd: dirs.test(), inp);
 
-        assert_eq!(actual.out, "foo");
-        assert_eq!(actual_repl.out, "foo");
+        assert_eq!(actual.out, "spam");
     })
 }