Run-time pipeline input typechecking tweaks#14922
Merged
132ikl merged 3 commits intonushell:mainfrom Feb 2, 2025
Merged
Conversation
This was referenced Sep 8, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR makes two changes related to run-time pipeline input type checking:
Type::Nothinginput types has been expanded to work with commands with multipleType::Nothinginputs for different outputs. For example,asthas three input/output type pairs, but all of the inputs areType::Nothing:Before this PR, passing a value (which would otherwise be ignored) to
astcaused a run-time type error: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 iscal), but it's more logically consistent.Type::Nothinginput and some other non-nothingTypeinput. This is accomplished by adding aType::Anyinput with the same output as the correspondingType::Nothinginput/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
opencommand is used insort-byclosure:Before this PR (but after the run-time input type checking PR), this error is raised:
While this error is technically correct, we don't actually want to return an error here since
openignores 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::Nothinginput. These commands operate on truenullvalues, rather than ignoring their input. For example,lengthreturns0when passed anullvalue. It's correct, and even desirable, to throw a run-time error whenlengthis passed an unexpected type. For example, a string, which should instead be measured withstr length:We need a more robust way for commands to express how they handle the
Type::Nothinginput case. I think a possible solution here is to allow commands to express that they operate onPipelineData::Empty, rather thanValue::Nothing. Then, a command likeopencould have an empty pipeline input type rather than aType::Nothing, and the parse-time and run-time pipeline input type checks know thatopenwill 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 argumentnu-check: checks input string or filename argumentopen: can take filename as input or argumentpolars when: can be used with input, or can be chained with anotherpolars whenstor insert: data record can be passed as input or argumentstor update: data record can be passed as input or argumentformat date:--listignores input valueinto datetime:--listignores input value (also added aType::Nothinginput 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,cdalways ignores its input. I fixed this in this PR.generategethistory importinterleaveinto boollengthUser-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:
openload-envformat dateinto datetimenu-checkstor insertstor updatepolars whenTests + Formatting
CI became green in the time it took me to type the description 😄
After Submitting
N/A