Fix circular source causing Nushell to crash (#12262)

# Description

EngineState now tracks the script currently running, instead of the
parent directory of the script. This also provides an easy way to expose
the current running script to the user (Issue #12195).

Similarly, StateWorkingSet now tracks scripts instead of directories.
`parsed_module_files` and `currently_parsed_pwd` are merged into one
variable, `scripts`, which acts like a stack for tracking the current
running script (which is on the top of the stack).

Circular import check is added for `source` operations, in addition to
module import. A simple testcase is added for circular source.

<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->


<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

It shouldn't have any user facing changes.

crates/nu-cli/src/config_files.rs

@@ -108,9 +108,9 @@ pub fn eval_config_contents(
         let config_filename = config_path.to_string_lossy();
 
         if let Ok(contents) = std::fs::read(&config_path) {
-            // Change currently parsed directory
+            // Set the current active file to the config file.
-            let prev_currently_parsed_cwd = engine_state.currently_parsed_cwd.clone();
+            let prev_file = engine_state.file.take();
-            engine_state.start_in_file(Some(&config_filename));
+            engine_state.file = Some(config_path.clone());
 
             eval_source(
                 engine_state,
@@ -121,8 +121,8 @@ pub fn eval_config_contents(
                 false,
             );
 
-            // Restore the currently parsed directory back
+            // Restore the current active file.
-            engine_state.currently_parsed_cwd = prev_currently_parsed_cwd;
+            engine_state.file = prev_file;
 
             // Merge the environment in case env vars changed in the config
             match nu_engine::env::current_dir(engine_state, stack) {

crates/nu-cli/src/eval_file.rs

@@ -13,7 +13,10 @@ use nu_protocol::{
 use nu_utils::stdout_write_all_and_flush;
 use std::sync::Arc;
 
-/// Main function used when a file path is found as argument for nu
+/// Entry point for evaluating a file.
+///
+/// If the file contains a main command, it is invoked with `args` and the pipeline data from `input`;
+/// otherwise, the pipeline data is forwarded to the first command in the file, and `args` are ignored.
 pub fn evaluate_file(
     path: String,
     args: &[String],
@@ -21,7 +24,7 @@ pub fn evaluate_file(
     stack: &mut Stack,
     input: PipelineData,
 ) -> Result<()> {
-    // Translate environment variables from Strings to Values
+    // Convert environment variables from Strings to Values and store them in the engine state.
     if let Some(e) = convert_env_values(engine_state, stack) {
         let working_set = StateWorkingSet::new(engine_state);
         report_error(&working_set, &e);
@@ -74,8 +77,7 @@ pub fn evaluate_file(
             );
             std::process::exit(1);
         });
-
+    engine_state.file = Some(file_path.clone());
-    engine_state.start_in_file(Some(file_path_str));
 
     let parent = file_path.parent().unwrap_or_else(|| {
         let working_set = StateWorkingSet::new(engine_state);
@@ -104,17 +106,19 @@ pub fn evaluate_file(
 
     let source_filename = file_path
         .file_name()
-        .expect("internal error: script missing filename");
+        .expect("internal error: missing filename");
 
     let mut working_set = StateWorkingSet::new(engine_state);
     trace!("parsing file: {}", file_path_str);
     let block = parse(&mut working_set, Some(file_path_str), &file, false);
 
+    // If any parse errors were found, report the first error and exit.
     if let Some(err) = working_set.parse_errors.first() {
         report_error(&working_set, err);
         std::process::exit(1);
     }
 
+    // Look for blocks whose name starts with "main" and replace it with the filename.
     for block in working_set.delta.blocks.iter_mut().map(Arc::make_mut) {
         if block.signature.name == "main" {
             block.signature.name = source_filename.to_string_lossy().to_string();
@@ -124,19 +128,21 @@ pub fn evaluate_file(
         }
     }
 
-    let _ = engine_state.merge_delta(working_set.delta);
+    // Merge the changes into the engine state.
+    engine_state
+        .merge_delta(working_set.delta)
+        .expect("merging delta into engine_state should succeed");
 
+    // Check if the file contains a main command.
     if engine_state.find_decl(b"main", &[]).is_some() {
-        let args = format!("main {}", args.join(" "));
+        // Evaluate the file, but don't run main yet.
-
         let pipeline_data =
             eval_block::<WithoutDebug>(engine_state, stack, &block, PipelineData::empty());
         let pipeline_data = match pipeline_data {
             Err(ShellError::Return { .. }) => {
-                // allows early exists before `main` is run.
+                // Allow early return before main is run.
                 return Ok(());
             }
-
             x => x,
         }
         .unwrap_or_else(|e| {
@@ -145,12 +151,12 @@ pub fn evaluate_file(
             std::process::exit(1);
         });
 
+        // Print the pipeline output of the file.
+        // The pipeline output of a file is the pipeline output of its last command.
         let result = pipeline_data.print(engine_state, stack, true, false);
-
         match result {
             Err(err) => {
                 let working_set = StateWorkingSet::new(engine_state);
-
                 report_error(&working_set, &err);
                 std::process::exit(1);
             }
@@ -161,6 +167,9 @@ pub fn evaluate_file(
             }
         }
 
+        // Invoke the main command with arguments.
+        // Arguments with whitespaces are quoted, thus can be safely concatenated by whitespace.
+        let args = format!("main {}", args.join(" "));
         if !eval_source(
             engine_state,
             stack,

crates/nu-command/src/system/nu_check.rs

@@ -1,6 +1,6 @@
 use nu_engine::{command_prelude::*, env::get_config, find_in_dirs_env, get_dirs_var_from_call};
 use nu_parser::{parse, parse_module_block, parse_module_file_or_dir, unescape_unquote_string};
-use nu_protocol::engine::StateWorkingSet;
+use nu_protocol::engine::{FileStack, StateWorkingSet};
 use std::path::Path;
 
 #[derive(Clone)]
@@ -112,15 +112,6 @@ impl Command for NuCheck {
                         Err(error) => return Err(error),
                     };
 
-                    // Change currently parsed directory
-                    let prev_currently_parsed_cwd = if let Some(parent) = path.parent() {
-                        let prev = working_set.currently_parsed_cwd.clone();
-                        working_set.currently_parsed_cwd = Some(parent.into());
-                        prev
-                    } else {
-                        working_set.currently_parsed_cwd.clone()
-                    };
-
                     let result = if as_module || path.is_dir() {
                         parse_file_or_dir_module(
                             path.to_string_lossy().as_bytes(),
@@ -130,12 +121,13 @@ impl Command for NuCheck {
                             call.head,
                         )
                     } else {
+                        // Unlike `parse_file_or_dir_module`, `parse_file_script` parses the content directly,
+                        // without adding the file to the stack. Therefore we need to handle this manually.
+                        working_set.files = FileStack::with_file(path.clone());
                         parse_file_script(&path, &mut working_set, is_debug, path_span, call.head)
+                        // The working set is not merged, so no need to pop the file from the stack.
                     };
 
-                    // Restore the currently parsed directory back
-                    working_set.currently_parsed_cwd = prev_currently_parsed_cwd;
-
                     result
                 } else {
                     Err(ShellError::GenericError {

crates/nu-lsp/src/lib.rs

@@ -250,8 +250,7 @@ impl LanguageServer {
     ) -> Option<(&Rope, &PathBuf, StateWorkingSet<'a>)> {
         let (file, path) = self.rope(file_url)?;
 
-        // TODO: AsPath thingy
+        engine_state.file = Some(path.to_owned());
-        engine_state.start_in_file(Some(&path.to_string_lossy()));
 
         let working_set = StateWorkingSet::new(engine_state);
 

crates/nu-parser/src/parse_keywords.rs

@@ -1871,32 +1871,16 @@ pub fn parse_module_block(
     (block, module, module_comments)
 }
 
+/// Parse a module from a file.
+///
+/// The module name is inferred from the stem of the file, unless specified in `name_override`.
 fn parse_module_file(
     working_set: &mut StateWorkingSet,
     path: ParserPath,
     path_span: Span,
     name_override: Option<String>,
 ) -> Option<ModuleId> {
-    if let Some(i) = working_set
+    // Infer the module name from the stem of the file, unless overridden.
-        .parsed_module_files
-        .iter()
-        .rposition(|p| p == path.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(path.path().to_string_lossy().to_string());
-
-        let msg = files.join("\nuses ");
-
-        working_set.error(ParseError::CyclicalModuleImport(msg, path_span));
-        return None;
-    }
-
     let module_name = if let Some(name) = name_override {
         name
     } else if let Some(stem) = path.file_stem() {
@@ -1909,6 +1893,7 @@ fn parse_module_file(
         return None;
     };
 
+    // Read the content of the module.
     let contents = if let Some(contents) = path.read(working_set) {
         contents
     } else {
@@ -1922,29 +1907,23 @@ fn parse_module_file(
     let file_id = working_set.add_file(path.path().to_string_lossy().to_string(), &contents);
     let new_span = working_set.get_span_for_file(file_id);
 
+    // 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);
     }
 
-    // Change the currently parsed directory
+    // Add the file to the stack of files being processed.
-    let prev_currently_parsed_cwd = if let Some(parent) = path.parent() {
+    if let Err(e) = working_set.files.push(path.path_buf(), path_span) {
-        working_set.currently_parsed_cwd.replace(parent.into())
+        working_set.error(e);
-    } else {
+        return None;
-        working_set.currently_parsed_cwd.clone()
+    }
-    };
-
-    // Add the file to the stack of parsed module files
-    working_set.parsed_module_files.push(path.path_buf());
 
     // Parse the module
     let (block, module, module_comments) =
         parse_module_block(working_set, new_span, module_name.as_bytes());
 
-    // Remove the file from the stack of parsed module files
+    // Remove the file from the stack of files being processed.
-    working_set.parsed_module_files.pop();
+    working_set.files.pop();
-
-    // Restore the currently parsed directory back
-    working_set.currently_parsed_cwd = prev_currently_parsed_cwd;
 
     let _ = working_set.add_block(Arc::new(block));
     let module_id = working_set.add_module(&module_name, module, module_comments);
@@ -3425,12 +3404,11 @@ pub fn parse_source(working_set: &mut StateWorkingSet, lite_command: &LiteComman
 
                 if let Some(path) = find_in_dirs(&filename, working_set, &cwd, LIB_DIRS_VAR) {
                     if let Some(contents) = path.read(working_set) {
-                        // Change currently parsed directory
+                        // Add the file to the stack of files being processed.
-                        let prev_currently_parsed_cwd = if let Some(parent) = path.parent() {
+                        if let Err(e) = working_set.files.push(path.clone().path_buf(), spans[1]) {
-                            working_set.currently_parsed_cwd.replace(parent.into())
+                            working_set.error(e);
-                        } else {
+                            return garbage_pipeline(spans);
-                            working_set.currently_parsed_cwd.clone()
+                        }
-                        };
 
                         // This will load the defs from the file into the
                         // working set, if it was a successful parse.
@@ -3441,8 +3419,8 @@ pub fn parse_source(working_set: &mut StateWorkingSet, lite_command: &LiteComman
                             scoped,
                         );
 
-                        // Restore the currently parsed directory back
+                        // Remove the file from the stack of files being processed.
-                        working_set.currently_parsed_cwd = prev_currently_parsed_cwd;
+                        working_set.files.pop();
 
                         // Save the block into the working set
                         let block_id = working_set.add_block(block);
@@ -3831,11 +3809,10 @@ pub fn find_in_dirs(
         dirs_var_name: &str,
     ) -> Option<ParserPath> {
         // Choose whether to use file-relative or PWD-relative path
-        let actual_cwd = if let Some(currently_parsed_cwd) = &working_set.currently_parsed_cwd {
+        let actual_cwd = working_set
-            currently_parsed_cwd.as_path()
+            .files
-        } else {
+            .current_working_directory()
-            Path::new(cwd)
+            .unwrap_or(Path::new(cwd));
-        };
 
         // Try if we have an existing virtual path
         if let Some(virtual_path) = working_set.find_virtual_path(filename) {
@@ -3895,11 +3872,10 @@ pub fn find_in_dirs(
         dirs_env: &str,
     ) -> Option<PathBuf> {
         // Choose whether to use file-relative or PWD-relative path
-        let actual_cwd = if let Some(currently_parsed_cwd) = &working_set.currently_parsed_cwd {
+        let actual_cwd = working_set
-            currently_parsed_cwd.as_path()
+            .files
-        } else {
+            .current_working_directory()
-            Path::new(cwd)
+            .unwrap_or(Path::new(cwd));
-        };
 
         if let Ok(p) = canonicalize_with(filename, actual_cwd) {
             Some(p)

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

@@ -98,8 +98,8 @@ pub struct EngineState {
     config_path: HashMap<String, PathBuf>,
     pub history_enabled: bool,
     pub history_session_id: i64,
-    // If Nushell was started, e.g., with `nu spam.nu`, the file's parent is stored here
+    // Path to the file Nushell is currently evaluating, or None if we're in an interactive session.
-    pub currently_parsed_cwd: Option<PathBuf>,
+    pub file: Option<PathBuf>,
     pub regex_cache: Arc<Mutex<LruCache<String, Regex>>>,
     pub is_interactive: bool,
     pub is_login: bool,
@@ -161,7 +161,7 @@ impl EngineState {
             config_path: HashMap::new(),
             history_enabled: true,
             history_session_id: 0,
-            currently_parsed_cwd: None,
+            file: None,
             regex_cache: Arc::new(Mutex::new(LruCache::new(
                 NonZeroUsize::new(REGEX_CACHE_SIZE).expect("tried to create cache of size zero"),
             ))),
@@ -322,15 +322,6 @@ impl EngineState {
         Ok(())
     }
 
-    /// Mark a starting point if it is a script (e.g., nu spam.nu)
-    pub fn start_in_file(&mut self, file_path: Option<&str>) {
-        self.currently_parsed_cwd = if let Some(path) = file_path {
-            Path::new(path).parent().map(PathBuf::from)
-        } else {
-            None
-        };
-    }
-
     pub fn has_overlay(&self, name: &[u8]) -> bool {
         self.scope
             .overlays

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

@@ -10,7 +10,7 @@ use crate::{
 use core::panic;
 use std::{
     collections::{HashMap, HashSet},
-    path::PathBuf,
+    path::{Path, PathBuf},
     sync::Arc,
 };
 
@@ -26,10 +26,7 @@ pub struct StateWorkingSet<'a> {
     pub permanent_state: &'a EngineState,
     pub delta: StateDelta,
     pub external_commands: Vec<Vec<u8>>,
-    /// Current working directory relative to the file being parsed right now
+    pub files: FileStack,
-    pub currently_parsed_cwd: Option<PathBuf>,
-    /// All previously parsed module files. Used to protect against circular imports.
-    pub parsed_module_files: Vec<PathBuf>,
     /// Whether or not predeclarations are searched when looking up a command (used with aliases)
     pub search_predecls: bool,
     pub parse_errors: Vec<ParseError>,
@@ -38,12 +35,18 @@ pub struct StateWorkingSet<'a> {
 
 impl<'a> StateWorkingSet<'a> {
     pub fn new(permanent_state: &'a EngineState) -> Self {
+        // Initialize the file stack with the top-level file.
+        let files = if let Some(file) = permanent_state.file.clone() {
+            FileStack::with_file(file)
+        } else {
+            FileStack::new()
+        };
+
         Self {
             delta: StateDelta::new(permanent_state),
             permanent_state,
             external_commands: vec![],
-            currently_parsed_cwd: permanent_state.currently_parsed_cwd.clone(),
+            files,
-            parsed_module_files: vec![],
             search_predecls: true,
             parse_errors: vec![],
             parse_warnings: vec![],
@@ -1100,3 +1103,65 @@ impl<'a> miette::SourceCode for &StateWorkingSet<'a> {
         Err(miette::MietteError::OutOfBounds)
     }
 }
+
+/// Files being evaluated, arranged as a stack.
+///
+/// The current active file is on the top of the stack.
+/// When a file source/import another file, the new file is pushed onto the stack.
+/// Attempting to add files that are already in the stack (circular import) results in an error.
+///
+/// Note that file paths are compared without canonicalization, so the same
+/// physical file may still appear multiple times under different paths.
+/// This doesn't affect circular import detection though.
+#[derive(Debug, Default)]
+pub struct FileStack(Vec<PathBuf>);
+
+impl FileStack {
+    /// Creates an empty stack.
+    pub fn new() -> Self {
+        Self(vec![])
+    }
+
+    /// Creates a stack with a single file on top.
+    ///
+    /// This is a convenience method that creates an empty stack, then pushes the file onto it.
+    /// It skips the circular import check and always succeeds.
+    pub fn with_file(path: PathBuf) -> Self {
+        Self(vec![path])
+    }
+
+    /// Adds a file to the stack.
+    ///
+    /// If the same file is already present in the stack, returns `ParseError::CircularImport`.
+    pub fn push(&mut self, path: PathBuf, span: Span) -> Result<(), ParseError> {
+        // Check for circular import.
+        if let Some(i) = self.0.iter().rposition(|p| p == &path) {
+            let filenames: Vec<String> = self.0[i..]
+                .iter()
+                .chain(std::iter::once(&path))
+                .map(|p| p.to_string_lossy().to_string())
+                .collect();
+            let msg = filenames.join("\nuses ");
+            return Err(ParseError::CircularImport(msg, span));
+        }
+
+        self.0.push(path);
+        Ok(())
+    }
+
+    /// Removes a file from the stack and returns its path, or None if the stack is empty.
+    pub fn pop(&mut self) -> Option<PathBuf> {
+        self.0.pop()
+    }
+
+    /// Returns the active file (that is, the file on the top of the stack), or None if the stack is empty.
+    pub fn top(&self) -> Option<&Path> {
+        self.0.last().map(PathBuf::as_path)
+    }
+
+    /// Returns the parent directory of the active file, or None if the stack is empty
+    /// or the active file doesn't have a parent directory as part of its path.
+    pub fn current_working_directory(&self) -> Option<&Path> {
+        self.0.last().and_then(|path| path.parent())
+    }
+}

crates/nu-protocol/src/errors/parse_error.rs

@@ -227,9 +227,9 @@ pub enum ParseError {
         #[label = "module directory is missing a mod.nu file"] Span,
     ),
 
-    #[error("Cyclical module import.")]
+    #[error("Circular import.")]
-    #[diagnostic(code(nu::parser::cyclical_module_import), help("{0}"))]
+    #[diagnostic(code(nu::parser::circular_import), help("{0}"))]
-    CyclicalModuleImport(String, #[label = "detected cyclical module import"] Span),
+    CircularImport(String, #[label = "detected circular import"] Span),
 
     #[error("Can't export {0} named same as the module.")]
     #[diagnostic(
@@ -506,7 +506,7 @@ impl ParseError {
             ParseError::NamedAsModule(_, _, _, s) => *s,
             ParseError::ModuleDoubleMain(_, s) => *s,
             ParseError::ExportMainAliasNotAllowed(s) => *s,
-            ParseError::CyclicalModuleImport(_, s) => *s,
+            ParseError::CircularImport(_, s) => *s,
             ParseError::ModuleOrOverlayNotFound(s) => *s,
             ParseError::ActiveOverlayNotFound(s) => *s,
             ParseError::OverlayPrefixMismatch(_, _, s) => *s,

crates/nu-protocol/src/eval_const.rs

@@ -11,6 +11,7 @@ use std::{
     path::{Path, PathBuf},
 };
 
+/// Create a Value for `$nu`.
 pub fn create_nu_constant(engine_state: &EngineState, span: Span) -> Result<Value, ShellError> {
     fn canonicalize_path(engine_state: &EngineState, path: &Path) -> PathBuf {
         let cwd = engine_state.current_work_dir();

crates/nu-std/src/lib.rs

@@ -3,7 +3,7 @@ use nu_engine::{env::current_dir, eval_block};
 use nu_parser::parse;
 use nu_protocol::{
     debugger::WithoutDebug,
-    engine::{Stack, StateWorkingSet, VirtualPath},
+    engine::{FileStack, Stack, StateWorkingSet, VirtualPath},
     report_error, PipelineData,
 };
 use std::path::PathBuf;
@@ -50,10 +50,9 @@ pub fn load_standard_library(
         }
 
         let std_dir = std_dir.to_string_lossy().to_string();
-        let source = format!(
+        let source = r#"
-            r#"
 # Define the `std` module
-module {std_dir}
+module std
 
 # Prelude
 use std dirs [
@@ -65,14 +64,14 @@ use std dirs [
     dexit
 ]
 use std pwd
-"#
+"#;
-        );
 
         let _ = working_set.add_virtual_path(std_dir, VirtualPath::Dir(std_virt_paths));
 
-        // Change the currently parsed directory
+        // Add a placeholder file to the stack of files being evaluated.
-        let prev_currently_parsed_cwd = working_set.currently_parsed_cwd.clone();
+        // The name of this file doesn't matter; it's only there to set the current working directory to NU_STDLIB_VIRTUAL_DIR.
-        working_set.currently_parsed_cwd = Some(PathBuf::from(NU_STDLIB_VIRTUAL_DIR));
+        let placeholder = PathBuf::from(NU_STDLIB_VIRTUAL_DIR).join("loading stdlib");
+        working_set.files = FileStack::with_file(placeholder);
 
         let block = parse(
             &mut working_set,
@@ -81,13 +80,13 @@ use std pwd
             false,
         );
 
+        // Remove the placeholder file from the stack of files being evaluated.
+        working_set.files.pop();
+
         if let Some(err) = working_set.parse_errors.first() {
             report_error(&working_set, err);
         }
 
-        // Restore the currently parsed directory back
-        working_set.currently_parsed_cwd = prev_currently_parsed_cwd;
-
         (block, working_set.render())
     };
 

src/ide.rs

@@ -8,7 +8,7 @@ use nu_protocol::{
 };
 use reedline::Completer;
 use serde_json::{json, Value as JsonValue};
-use std::sync::Arc;
+use std::{path::PathBuf, sync::Arc};
 
 #[derive(Debug)]
 enum Id {
@@ -67,7 +67,7 @@ fn read_in_file<'a>(
             std::process::exit(1);
         });
 
-    engine_state.start_in_file(Some(file_path));
+    engine_state.file = Some(PathBuf::from(file_path));
 
     let working_set = StateWorkingSet::new(engine_state);
 

tests/parsing/mod.rs

@@ -33,6 +33,15 @@ fn source_const_file() {
     assert_eq!(actual.out, "5");
 }
 
+#[test]
+fn source_circular() {
+    let actual = nu!(cwd: "tests/parsing/samples", "
+        nu source_circular_1.nu
+        ");
+
+    assert!(actual.err.contains("nu::parser::circular_import"));
+}
+
 #[test]
 fn run_nu_script_single_line() {
     let actual = nu!(cwd: "tests/parsing/samples", "

tests/parsing/samples/source_circular_1.nu

@@ -0,0 +1 @@
+source source_circular_2.nu

tests/parsing/samples/source_circular_2.nu

@@ -0,0 +1 @@
+source source_circular_1.nu