parsed_module_files and currently_parsed_pwd are merged into one variable

It seems that CI is failed because of this

Just some small comments. I like the general idea of using a single stack of files to handle both file-relative current working directory and circular parser access (be it sourced file or imported module). With a few touches, I think this can land.

nu_check is failing because you set the parsed file in the command, then is_circular_import() is triggered when parse_module_file() is called. The solution is probably to remove the setting in nu-check for modules because that will be done in parse_module_file().

#12338 paused to extend this ✨

Thanks for all the feedback, and sorry for the late replies. I had something else to tend to, which was why I saved this PR as a draft. I'm back to working on this now.

I'm working on a dedicated FileStack abstraction. Circular import detection happens as part of the push method call, which prevents inconsistent states.

I also wants to enforce the invariant that all paths must be absolute, so that we can safely calculate the current working directory. But it doesn't work with virtual filesystem, which uses a non-existent relative path, so a call to canonicalize() will fail.

Maybe I should just give up the "must be absolute path" part of the invariant? But that would make circular detection unreliable, since two relative paths can resolve to the same file.

Here's what it currently looks like:

/// 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 source/import) results in an error.
///
/// Paths stored in a stack are always in a canonical, absolute form.
pub struct FileStack(Vec<PathBuf>);

impl FileStack {
    /// Creates an empty stack.
    pub fn new() -> Self {
        Self(vec![])
    }

    /// Adds a file to the stack.
    ///
    /// Relative paths are resolved into absolute paths first.
    ///
    /// If the same file is already present in the stack, returns `ParseError::CircularImport`.
    /// If the path doesn't exist or is not a regular file, returns `ParseError::FileNotFound`.
    pub fn push(&mut self, file: &Path, span: Span) -> Result<(), ParseError> {
        // Canonicalize the path.
        let path = file
            .canonicalize()
            .map_err(|_| ParseError::FileNotFound(file.to_string_lossy().to_string(), span))?;

        // Check if the path points to a regular file.
        if !path.is_file() {
            return Err(ParseError::FileNotFound(
                path.to_string_lossy().to_string(),
                span,
            ));
        }

        // 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 current 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.
    pub fn current_working_directory(&self) -> Option<&Path> {
        // We maintain the invariant that paths are always absolute, and always points to a regular file.
        // Therefore, `path.parent()` always succeeds, and the unwrap is safe.
        self.0.last().map(|path| path.parent().unwrap())
    }
}

I'd like to defend my usage of expect. In my opinion, using expect to check invariants and catch bugs is a good thing. If the invariant is ever broken, there's some serious issues with the code, and it's better to just crash than continue with an inconsistent state. Using expect instead of bubbling up a result also has the advantange that the user can send a stacktrace to us, which helps with debugging.

This topic used to be discussed quite a lot, such as this post, which is an interesting read.

It used to be that panics would simply kill our REPL which is unacceptable for a shell. Nowadays, we catch a panic, so it's less of a problem, so I guess it's OK. I'd wrap it into some API, like the FileStack. Having random uncommented freestanding expects/unwraps around the codebase can promote bad hygiene.

I like the FileStack API approach better than hacking directly on the working set stack manually. Maybe the FileStack can use the ParserPath instead of PathBuf? The ParserPath was created specifically to abstract over real and virtual paths. Virtual paths can always be considered "absolute" so that should solve your problem (it could have a helper is_absolute() method for example).

One last thing about the FileStack: Instead of canonicalize(), our own nu-path::canonicalize_with() should be used with a provided cwd. Default canonicalize() implicitly uses std::env::current_dir() to expand the path, which results in wrong semantics because we don't use std::env in Nushell at all. Canonicalizing inside the push(), might not be necessary, though. The paths coming from the parser should be already canonicalized, so we could just check if the path is absolute. Either way is fine to me.

I'd wrap it into some API, like the FileStack.

I would do that if I could, but sometimes only the caller knows whether an expect/unwrap is safe or not (e.g. pushing paths into a newly-created stack is always safe, but the FileStack doesn't know that).

Canonicalizing inside the push(), might not be necessary, though.

I forgot to mention this, but I decided to drop the invariant that paths must be absolute in the final implementation (that code snippet in the comment above is outdated). Refer to the actual commits instead.

Without canonicalization, the same file might be imported twice under different paths, but we'll catch the circular import eventually, so this shouldn't be a problem.

OK, looks good. Thanks!