Strings are parsed inconsistently for external commands

[YizhePKU]

Describe the bug

When passing string literals to external commands, any surrounding quotes are included in the resulting Expr::String. However, when passing a list of string literals using the spread operator, any surrounding quotes are NOT included in the resulting Expr::String.

How to reproduce

1. Add a dbg!(call) to the run() function in crates\nu-command\src\system\run_external.rs.
2. Run ^ls "foo" ...[ "bar" ]

Here's the output:

[crates\nu-command\src\system\run_external.rs:50:9] call = Call {
    decl_id: 0,
    head: Span {
        start: 123432,
        end: 123434,
    },
    arguments: [
        Positional(
            Expression {
                expr: String(
                    "ls",
                ),
                span: Span {
                    start: 123432,
                    end: 123434,
                },
                ty: String,
                custom_completion: None,
            },
        ),
        Positional(
            Expression {
                expr: String(
                    "\"foo\"",        <----- quotes kept
                ),
                span: Span {
                    start: 123435,
                    end: 123440,
                },
                ty: String,
                custom_completion: None,
            },
        ),
        Spread(
            Expression {
                expr: List(
                    [
                        Item(
                            Expression {
                                expr: String(
                                    "bar",      <---- quotes removed
                                ),
                                span: Span {
                                    start: 123446,
                                    end: 123451,
                                },
                                ty: String,
                                custom_completion: None,
                            },
                        ),
                    ],
                ),
                span: Span {
                    start: 123444,
                    end: 123453,
                },
                ty: List(
                    String,
                ),
                custom_completion: None,
            },
        ),
    ],
    parser_info: {},
}

Expected behavior

Currently run_external gets special treatment so that strings retain their surrounding quotes. The quotes are then trimmed within run_external so it can use the quotes to decide whether to perform glob/tilde expansions or not.

I think this is not a good solution, for several reason:

• It duplicates the quote-striping logic. The parser can strip quotes just fine; we shouldn't need to do it again.
• It provides more information than needed. run_external only needs to know one bit of information: whether the string was evaluated from an unquoted/backtick-quoted string literal. That information can be provided as a boolean.
• It introduces invalid states. "Make invalid states impossible to represent" is generally a good design criteria. Keeping surrounding quotes means that we allow unbalanced quotes to be represented.
• run_external is not the only command that needs to handle expansions. We should cover the use cases of other commands too, like ls and rm.

---

My ideal solution would be to change Value::String into something like this:

enum Value {
   // ...
   String { val: String, bare: bool }
   // ...
}

Various eval() functions should set bare to true if and only if it was evaluated from an unquoted/backtick-quoted string literal. This way, commands that want to handle expansions can do it without ever touching the syntax tree.

While we're at it, let's also remove the Value::Glob variant. The proposed Value::String variant is sufficient to cover all use cases.

---

I'm currently doing a full-rewrite of the run_external module to improve readability (Issue #6011). I'll finish the rewrite regardless of whether we change anything or not, but it is a good opportunity to revisit some of our design decisions.

Screenshots

No response

Configuration

key	value
version	0.93.1
major	0
minor	93
patch	1
branch	set-cwd
commit_hash	bde8255
build_os	windows-x86_64
build_target	x86_64-pc-windows-msvc
rust_version	rustc 1.77.2 (25ef9e3d8 2024-04-09)
rust_channel	1.77.2-x86_64-pc-windows-msvc
cargo_version	cargo 1.77.2 (e52e36006 2024-03-26)
build_time	2024-05-06 19:53:16 +08:00
build_rust_channel	release
allocator	mimalloc
features	default, sqlite, system-clipboard, trash, which
installed_plugins

Additional context

No response

[fdncred]

It seems like we need to be consistent, but we also need to be able to tell when to expand a glob or ~.

[YizhePKU]

To be fair, we don't have to be consistent, but our current implementation feels more like a hack than a proper solution. So this is technically not a bug, just something I want to bring into attention.

[fdncred]

Maybe I don't understand what you're saying because I'd argue that we indeed have to be consistent. If the user is going to opt-in to globbing or expansion, they have to know how to do it and that means it needs to have some level of consistency, otherwise it's a confusing mess.

[YizhePKU]

The current behavior is consistent for the user (bare-strings/backticks = globbing/expansion). It's only inconsistent for the developer (i.e. the current me), who has to handle strings differently based on where they came from.

[YizhePKU]

I'll close this for now. Maybe we can revisit this later.