Skip to content

Run-time pipeline input typechecking tweaks#14922

Merged
132ikl merged 3 commits intonushell:mainfrom
132ikl:input-typecheck-tweaks
Feb 2, 2025
Merged

Run-time pipeline input typechecking tweaks#14922
132ikl merged 3 commits intonushell:mainfrom
132ikl:input-typecheck-tweaks

Conversation

@132ikl
Copy link
Copy Markdown
Member

@132ikl 132ikl commented Jan 27, 2025

Description

This PR makes two changes related to run-time pipeline input type checking:

  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.

  1. 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 when working with an example where the open command is used in sort-by closure:
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<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:

["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<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

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 😄

After Submitting

N/A

@132ikl 132ikl changed the title Pipeline input typechecking tweaks Run-time pipeline input typechecking tweaks Jan 27, 2025
@132ikl 132ikl marked this pull request as ready for review January 27, 2025 03:05
@132ikl 132ikl merged commit 13d5a15 into nushell:main Feb 2, 2025
@github-actions github-actions bot added this to the v0.102.0 milestone Feb 2, 2025
@fdncred fdncred added the A:type-system Problems or features related to nushell's type system label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:type-system Problems or features related to nushell's type system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants