Fix incorrect handling of boolean flags for builtin commands

[NotLebedev]

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

After:

Another example:
Before:

After:

Tests + Formatting

Added test reproducing some variants of original issue.

[kubouch]

The fix seems good. Two points:

• Can you add a test to make sure that Buitlin commands such as ls, cp do not respect --flag=false #11456 is fixed?
• Why is has_flag() now in CallExt instead of Call?

[kubouch]

Also, since you essentially renamed the old has_flag() to has_named(), shouldn't has_named() be removed because it has a faulty logic? What is the reason for keeping it?

[NotLebedev]

1. Added test for some flags of ls command but I am not sure how to make a more general test (and if it is needed)
2. Call is part of nu_protocol::ast and its methods don't know anything about nu_protocol::engine and dont call eval_expression method. The trait CallExt is a collection of methods that can evaluate arguments of call
3. Actually the reason was me. I misunderstood what it did in parse_keyword.rs and though that the old logic was necessary there. But actually it should be the same, so I replaced all remaining usages of has_named (also some in commands) and deleted it.

[kubouch]

OK, the test looks good enough.

One thing that I also noticed is that the parser shouldn't depend on the engine. If you check, nu-engine is used only in one place in the parser and that case should be removed in some other PR. So, I think the has_flag_const() should be moved to Call in nu-protocol. This should be possible as you only need the StateWorkingSet, you don't need the engine's Stack.

[NotLebedev]

If you check, nu-engine is used only in one place in the parser and that case should be removed in some other PR. So, I think the has_flag_const() should be moved to Call in nu-protocol. This should be possible as you only need the StateWorkingSet, you don't need the engine's Stack.

That's fair. I think get_flag_const should also be moved to Call for consistency. I will take a look at this and submit a PR

[kubouch]

(retroactively added the label to merge after the dust settles around the new release)

OK, could you then make a new PR moving the xxx_const() from CallExt to Call and while at it, you could also remove nu-engine dependency from the parser altogether if you don't mind. The move from CallExt to Call can also be a part of this PR, that would make me feel better not adding CallExt import to the parser 😆.

[NotLebedev]

Done. Moved all _const methods from CallExt to Call. Also moved an auxiliary method rest_iter_flattened that was used by const mehtods (and in general had no place being part of CallExt)

[kubouch]

OK, thanks, let's try it out!

[kik4444]

Hi, @NotLebedev, it seems that b90fe82 introduced a regression. When I build the latest nushell:main branch and try to run something like touch -r deps build I get an error

Error: nu::shell::cant_convert

  × Can't convert to bool.
   ╭─[entry #3:1:1]
 1 │ touch -r deps build
   ·          ──┬─
   ·            ╰── can't convert string to bool
   ╰────

On commits prior to that there's no issues. Could you check what went wrong?