From 13d5a15f7562fc3ade6cb77512806c1651eab05f Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Sun, 2 Feb 2025 15:51:47 -0500 Subject: [PATCH] Run-time pipeline input typechecking tweaks (#14922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description 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::shell::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.   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::shell::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 ╰──── ``` 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::shell::only_supports_this_input_type # => # => × Input type not supported. # => ╭─[entry #32:1:10] # => 1 │ ["hello" "world"] | sort-by { length } # => · ───┬─── ───┬── # => · │ ╰── only list, 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 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 CI became green in the time it took me to type the description :smile: # After Submitting N/A --- crates/nu-command/src/conversions/into/datetime.rs | 5 +++++ crates/nu-command/src/env/load_env.rs | 3 +++ crates/nu-command/src/filesystem/cd.rs | 4 ---- crates/nu-command/src/filesystem/open.rs | 8 +++++++- crates/nu-command/src/stor/insert.rs | 3 +++ crates/nu-command/src/stor/update.rs | 3 +++ crates/nu-command/src/strings/format/date.rs | 4 ++++ crates/nu-command/src/system/nu_check.rs | 3 +++ crates/nu-engine/src/eval_ir.rs | 7 ++----- .../src/dataframe/command/boolean/when.rs | 3 +++ 10 files changed, 33 insertions(+), 10 deletions(-) diff --git a/crates/nu-command/src/conversions/into/datetime.rs b/crates/nu-command/src/conversions/into/datetime.rs index 2b274c3ea5..0fc9725bcf 100644 --- a/crates/nu-command/src/conversions/into/datetime.rs +++ b/crates/nu-command/src/conversions/into/datetime.rs @@ -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( diff --git a/crates/nu-command/src/env/load_env.rs b/crates/nu-command/src/env/load_env.rs index f35b1d08ea..b508aa6f3b 100644 --- a/crates/nu-command/src/env/load_env.rs +++ b/crates/nu-command/src/env/load_env.rs @@ -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( diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index 3ba438746d..c4f540010e 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -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) } diff --git a/crates/nu-command/src/filesystem/open.rs b/crates/nu-command/src/filesystem/open.rs index a372d7e79e..a2a63f180b 100644 --- a/crates/nu-command/src/filesystem/open.rs +++ b/crates/nu-command/src/filesystem/open.rs @@ -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]), diff --git a/crates/nu-command/src/stor/insert.rs b/crates/nu-command/src/stor/insert.rs index 4a2ebf4928..1d0f5ae6c0 100644 --- a/crates/nu-command/src/stor/insert.rs +++ b/crates/nu-command/src/stor/insert.rs @@ -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", diff --git a/crates/nu-command/src/stor/update.rs b/crates/nu-command/src/stor/update.rs index 8a77f41d51..98652bae2a 100644 --- a/crates/nu-command/src/stor/update.rs +++ b/crates/nu-command/src/stor/update.rs @@ -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", diff --git a/crates/nu-command/src/strings/format/date.rs b/crates/nu-command/src/strings/format/date.rs index 658823aa6a..fa7911361c 100644 --- a/crates/nu-command/src/strings/format/date.rs +++ b/crates/nu-command/src/strings/format/date.rs @@ -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')) diff --git a/crates/nu-command/src/system/nu_check.rs b/crates/nu-command/src/system/nu_check.rs index ca0d09965a..f3ecdd004d 100644 --- a/crates/nu-command/src/system/nu_check.rs +++ b/crates/nu-command/src/system/nu_check.rs @@ -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.") diff --git a/crates/nu-engine/src/eval_ir.rs b/crates/nu-engine/src/eval_ir.rs index 4cc8cef782..ecdebbac51 100644 --- a/crates/nu-engine/src/eval_ir.rs +++ b/crates/nu-engine/src/eval_ir.rs @@ -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 { diff --git a/crates/nu_plugin_polars/src/dataframe/command/boolean/when.rs b/crates/nu_plugin_polars/src/dataframe/command/boolean/when.rs index 9c6976aa54..e6c70ec73d 100644 --- a/crates/nu_plugin_polars/src/dataframe/command/boolean/when.rs +++ b/crates/nu_plugin_polars/src/dataframe/command/boolean/when.rs @@ -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())) }