Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buitlin commands such as ls, cp do not respect --flag=false #11456

Closed
crabdancing opened this issue Dec 31, 2023 · 4 comments
Closed

Buitlin commands such as ls, cp do not respect --flag=false #11456

crabdancing opened this issue Dec 31, 2023 · 4 comments
Labels
needs-triage An issue that hasn't had any proper look
Milestone

Comments

@crabdancing
Copy link

Describe the bug

Awhile ago, --flag=true / --flag=false was added to fix the combinatorial explosion when making wrappers. This works great on user-defined scripts. Although the parser appears to also understand this syntax with respect to builtin commands, it doesn't actually seem to respect what the syntax is saying.

How to reproduce

  1. ls --long=false

  2. *ls is long* :c

name type target readonly mode num_links inode user group size created accessed modified
bar file false rw-r--r-- 1 24217229 root root 0 B 2023-12-31 08:45:31 (Sun, Dec) -0700 2023-12-31 08:45:31 (Sun, Dec) -0700 2023-12-31 08:45:31 (Sun, Dec) -0700
foo file false rw-r--r-- 1 24217228 root root 0 B 2023-12-31 08:45:31 (Sun, Dec) -0700 2023-12-31 08:45:31 (Sun, Dec) -0700 2023-12-31 08:45:31 (Sun, Dec) -0700

Expected behavior

--flag=false should disable flag for the command.

Screenshots

No response

Configuration

key value
version 0.88.1
branch
commit_hash
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.74.0 (79e9716c9 2023-11-13) (built from a source tarball)
cargo_version cargo 1.74.0
build_time 1980-01-01 00:00:00 +00:00
build_rust_channel release
allocator mimalloc
features dataframe, default, extra, sqlite, trash, which, zip
installed_plugins from eml, from ics, from ini, from vcf, gstat, net, query, query json, query web, query xml, regex

Additional context

No response

@crabdancing crabdancing added the needs-triage An issue that hasn't had any proper look label Dec 31, 2023
@NotLebedev
Copy link
Contributor

The issue is the has_flag method. It is used in commands to check if flag is set, but in reality it checks if a named argument is present (without evaluating its argument if it exists)

I think I can fix it. I will rename existing has_flag method to has_named and replace usages in parser with has_named. I will move the has_flag command to CallExt and change body to something like this:

    fn has_flag(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        flag_name: &str,
    ) -> Result<bool, ShellError> {
        for name in self.named_iter() {
            if flag_name == name.0.item {
                return if let Some(expr) = &name.2 {
                    // Check --flag=false
                    let result = eval_expression(engine_state, stack, expr)?;
                    match result {
                        Value::Bool { val, .. } => Ok(val),
                        _ => Err(ShellError::CantConvert {
                            to_type: "bool".into(),
                            from_type: result.get_type().to_string(),
                            span: result.span(),
                            help: Some("".into()),
                        }),
                    }
                } else {
                    Ok(true)
                };
            }
        }

        Ok(false)
    }

which will additionally evaluate expression of flag if it exists and check if it is true. Unlike get_flag it will return Result<bool, ShellError> and produce an error if result of expression evaluation is not a bool.
I tried it in ls only and it seems to work so I'll try replacing it in all commands and see if it works

kubouch pushed a commit that referenced this issue Jan 11, 2024
# Description
Possible fix of #11456
This PR fixes a bug where builtin commands did not respect the logic of
dynamically passed boolean flags. The reason is
[has_flag](https://github.com/nushell/nushell/blob/6f59abaf4310487f7a6319437be6ec61abcbc3b9/crates/nu-protocol/src/ast/call.rs#L204C5-L212C6)
method did not evaluate and take into consideration expression used with
flag.

To address this issue a solution is proposed:
1. `has_flag` method is moved to `CallExt` and new logic to evaluate
expression and check if it is a boolean value is added
2. `has_flag_const` method is added to `CallExt` which is a constant
version of `has_flag`
3. `has_named` method is added to `Call` which is basically the old
logic of `has_flag`
4. All usages of `has_flag` in code are updated, mostly to pass
`engine_state` and `stack` to new `has_flag`. In `run_const` commands it
is replaced with `has_flag_const`. And in a few select places: parser,
`to nuon` and `into string` old logic via `has_named` is used.

# User-Facing Changes
Explicit values of boolean flags are now respected in builtin commands.
Before:

![image](https://github.com/nushell/nushell/assets/17511668/f9fbabb2-3cfd-43f9-ba9e-ece76d80043c)
After:

![image](https://github.com/nushell/nushell/assets/17511668/21867596-2075-437f-9c85-45563ac70083)

Another example:
Before:

![image](https://github.com/nushell/nushell/assets/17511668/efdbc5ca-5227-45a4-ac5b-532cdc2bbf5f)
After:

![image](https://github.com/nushell/nushell/assets/17511668/2907d5c5-aa93-404d-af1c-21cdc3d44646)


# Tests + Formatting
Added test reproducing some variants of original issue.
@WindSoilder
Copy link
Collaborator

Is it resolved? @NotLebedev

@NotLebedev
Copy link
Contributor

Is it resolved? @NotLebedev

Yes

@WindSoilder
Copy link
Collaborator

Thanks for resolving it! It highly improves user experience when using builtin commands :)

@hustcer hustcer added this to the v0.90.0 milestone Jan 15, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
…#11492)

# Description
Possible fix of nushell#11456
This PR fixes a bug where builtin commands did not respect the logic of
dynamically passed boolean flags. The reason is
[has_flag](https://github.com/nushell/nushell/blob/6f59abaf4310487f7a6319437be6ec61abcbc3b9/crates/nu-protocol/src/ast/call.rs#L204C5-L212C6)
method did not evaluate and take into consideration expression used with
flag.

To address this issue a solution is proposed:
1. `has_flag` method is moved to `CallExt` and new logic to evaluate
expression and check if it is a boolean value is added
2. `has_flag_const` method is added to `CallExt` which is a constant
version of `has_flag`
3. `has_named` method is added to `Call` which is basically the old
logic of `has_flag`
4. All usages of `has_flag` in code are updated, mostly to pass
`engine_state` and `stack` to new `has_flag`. In `run_const` commands it
is replaced with `has_flag_const`. And in a few select places: parser,
`to nuon` and `into string` old logic via `has_named` is used.

# User-Facing Changes
Explicit values of boolean flags are now respected in builtin commands.
Before:

![image](https://github.com/nushell/nushell/assets/17511668/f9fbabb2-3cfd-43f9-ba9e-ece76d80043c)
After:

![image](https://github.com/nushell/nushell/assets/17511668/21867596-2075-437f-9c85-45563ac70083)

Another example:
Before:

![image](https://github.com/nushell/nushell/assets/17511668/efdbc5ca-5227-45a4-ac5b-532cdc2bbf5f)
After:

![image](https://github.com/nushell/nushell/assets/17511668/2907d5c5-aa93-404d-af1c-21cdc3d44646)


# Tests + Formatting
Added test reproducing some variants of original issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage An issue that hasn't had any proper look
Projects
None yet
Development

No branches or pull requests

4 participants