Run-time pipeline input typechecking tweaks (#14922)

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

# Description
<!--
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.
-->

This PR makes two changes related to [run-time pipeline input type
checking](https://github.com/nushell/nushell/pull/14741):

1. The check which bypasses type checking for commands with only
`Type::Nothing` input types has been expanded to work with commands with
multiple `Type::Nothing` inputs for different outputs. For example,
`ast` has three input/output type pairs, but all of the inputs are
`Type::Nothing`:
  ```
  ╭───┬─────────┬────────╮
  │ # │  input  │ output │
  ├───┼─────────┼────────┤
  │ 0 │ nothing │ table  │
  │ 1 │ nothing │ record │
  │ 2 │ nothing │ string │
  ╰───┴─────────┴────────╯
  ```
Before this PR, passing a value (which would otherwise be ignored) to
`ast` caused a run-time type error:
  ```
    Error: nu:🐚:only_supports_this_input_type
  
    × Input type not supported.
     ╭─[entry #1:1:6]
   1 │ echo 123 | ast -j -f "hi" 
     ·      ─┬─   ─┬─
· │ ╰── only nothing, nothing, and nothing input data is supported
     ·       ╰── input type: int
     ╰────
  
  ```

  After this PR, no error is raised.

This doesn't really matter for `ast` (the only other built-in command
with a similar input/output type signature is `cal`), but it's more
logically consistent.

2. Bypasses input type-checking (parse-time ***and*** run-time) for some
(not all, see below) commands which have both a `Type::Nothing` input
and some other non-nothing `Type` input. This is accomplished by adding
a `Type::Any` input with the same output as the corresponding
`Type::Nothing` input/output pair.
  &nbsp;
This is necessary because some commands are intended to operate on an
argument with empty pipeline input, or operate on an empty pipeline
input with no argument. This causes issues when a value is implicitly
passed to one of these commands. I [discovered this
issue](https://discord.com/channels/601130461678272522/615962413203718156/1329945784346611712)
when working with an example where the `open` command is used in
`sort-by` closure:
```nushell
ls | sort-by { open -r $in.name | lines | length }
```

Before this PR (but after the run-time input type checking PR), this
error is raised:

```
Error: nu:🐚:only_supports_this_input_type

  × Input type not supported.
   ╭─[entry #1:1:1]
 1 │ ls | sort-by { open -r $in.name | lines | length }
   · ─┬             ──┬─
   ·  │               ╰── only nothing and string input data is supported
   ·  ╰── input type: record<name: string, type: string, size: filesize, modified: date>
   ╰────
```

While this error is technically correct, we don't actually want to
return an error here since `open` ignores its pipeline input when an
argument is passed. This would be a parse-time error as well if the
parser was able to infer that the closure input type was a record, but
our type inference isn't that robust currently, so this technically
incorrect form snuck by type checking until #14741.

However, there are some commands with the same kind of type signature
where this behavior is actually desirable. This means we can't just
bypass type-checking for any command with a `Type::Nothing` input. These
commands operate on true `null` values, rather than ignoring their
input. For example, `length` returns `0` when passed a `null` value.
It's correct, and even desirable, to throw a run-time error when
`length` is passed an unexpected type. For example, a string, which
should instead be measured with `str length`:

```nushell
["hello" "world"] | sort-by { length }
# => Error: nu:🐚:only_supports_this_input_type
# => 
# =>   × Input type not supported.
# =>    ╭─[entry #32:1:10]
# =>  1 │ ["hello" "world"] | sort-by { length }
# =>    ·          ───┬───              ───┬──
# =>    ·             │                    ╰── only list<any>, binary, and nothing input data is supported
# =>    ·             ╰── input type: string
# =>    ╰────
```

We need a more robust way for commands to express how they handle the
`Type::Nothing` input case. I think a possible solution here is to allow
commands to express that they operate on `PipelineData::Empty`, rather
than `Value::Nothing`. Then, a command like `open` could have an empty
pipeline input type rather than a `Type::Nothing`, and the parse-time
and run-time pipeline input type checks know that `open` will safely
ignore an incorrectly typed input.

That being said, we have a release coming up and the above solution
might take a while to implement, so while unfortunate, bypassing input
type-checking for these problematic commands serves as a workaround to
avoid breaking changes in the release until a more robust solution is
implemented.

This PR bypasses input type-checking for the following commands:
* `load-env`: can take record of envvars as input or argument
* `nu-check`: checks input string or filename argument 
* `open`: can take filename as input or argument
* `polars when`: can be used with input, or can be chained with another
`polars when`
* `stor insert`: data record can be passed as input or argument
* `stor update`: data record can be passed as input or argument
* `format date`: `--list` ignores input value
* `into datetime`: `--list` ignores input value (also added a
`Type::Nothing` input which was missing from this command)

These commands have a similar input/output signature to the above
commands, but are working as intended:
* `cd`: The input/output signature was actually incorrect, `cd` always
ignores its input. I fixed this in this PR.
* `generate`
* `get`
* `history import` 
* `interleave`
* `into bool`
* `length`

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

As a temporary workaround, pipeline input type-checking for the
following commands has been bypassed to avoid undesirable run-time input
type checking errors which were previously not caught at parse-time:
* `open`
* `load-env`
* `format date`
* `into datetime`
* `nu-check`
* `stor insert`
* `stor update`
* `polars when`

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
CI became green in the time it took me to type the description 😄 

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->

N/A
This commit is contained in:
132ikl 2025-02-02 15:51:47 -05:00 committed by GitHub
parent 339c5b7c83
commit 13d5a15f75
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 33 additions and 10 deletions

View File

@ -65,6 +65,11 @@ impl Command for SubCommand {
(Type::List(Box::new(Type::String)), Type::List(Box::new(Type::Date))),
(Type::table(), Type::table()),
(Type::record(), Type::record()),
(Type::Nothing, Type::table()),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
// only applicable for --list flag
(Type::Any, Type::table()),
])
.allow_variants_without_examples(true)
.named(

View File

@ -17,6 +17,9 @@ impl Command for LoadEnv {
.input_output_types(vec![
(Type::record(), Type::Nothing),
(Type::Nothing, Type::Nothing),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
(Type::Any, Type::Nothing),
])
.allow_variants_without_examples(true)
.optional(

View File

@ -25,10 +25,6 @@ impl Command for Cd {
.input_output_types(vec![(Type::Nothing, Type::Nothing)])
.switch("physical", "use the physical directory structure; resolve symbolic links before processing instances of ..", Some('P'))
.optional("path", SyntaxShape::Directory, "The path to change to.")
.input_output_types(vec![
(Type::Nothing, Type::Nothing),
(Type::String, Type::Nothing),
])
.allow_variants_without_examples(true)
.category(Category::FileSystem)
}

View File

@ -35,7 +35,13 @@ impl Command for Open {
fn signature(&self) -> nu_protocol::Signature {
Signature::build("open")
.input_output_types(vec![(Type::Nothing, Type::Any), (Type::String, Type::Any)])
.input_output_types(vec![
(Type::Nothing, Type::Any),
(Type::String, Type::Any),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
(Type::Any, Type::Any),
])
.rest(
"files",
SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::String]),

View File

@ -17,6 +17,9 @@ impl Command for StorInsert {
(Type::Nothing, Type::table()),
(Type::record(), Type::table()),
(Type::table(), Type::table()),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
(Type::Any, Type::table()),
])
.required_named(
"table-name",

View File

@ -16,6 +16,9 @@ impl Command for StorUpdate {
.input_output_types(vec![
(Type::Nothing, Type::table()),
(Type::record(), Type::table()),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
(Type::Any, Type::table()),
])
.required_named(
"table-name",

View File

@ -19,6 +19,10 @@ impl Command for FormatDate {
(Type::Date, Type::String),
(Type::String, Type::String),
(Type::Nothing, Type::table()),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
// only applicable for --list flag
(Type::Any, Type::table()),
])
.allow_variants_without_examples(true) // https://github.com/nushell/nushell/issues/7032
.switch("list", "lists strftime cheatsheet", Some('l'))

View File

@ -20,6 +20,9 @@ impl Command for NuCheck {
(Type::Nothing, Type::Bool),
(Type::String, Type::Bool),
(Type::List(Box::new(Type::Any)), Type::Bool),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
(Type::Any, Type::Bool),
])
// type is string to avoid automatically canonicalizing the path
.optional("path", SyntaxShape::String, "File path to parse.")

View File

@ -1277,11 +1277,8 @@ fn check_input_types(
}
// If a command only has a nothing input type, then allow any input data
match io_types.first() {
Some((Type::Nothing, _)) if io_types.len() == 1 => {
return Ok(());
}
_ => (),
if io_types.iter().all(|(intype, _)| intype == &Type::Nothing) {
return Ok(());
}
match input {

View File

@ -41,6 +41,9 @@ impl PluginCommand for ExprWhen {
Type::Custom("expression".into()),
Type::Custom("expression".into()),
),
// FIXME Type::Any input added to disable pipeline input type checking, as run-time checks can raise undesirable type errors
// which aren't caught by the parser. see https://github.com/nushell/nushell/pull/14922 for more details
(Type::Any, Type::Custom("expression".into())),
])
.category(Category::Custom("expression".into()))
}