Standardise the use of ShellError::UnsupportedInput and ShellError::TypeMismatch and add spans to every instance of the former by webbedspace · Pull Request #7217 · nushell/nushell

webbedspace · 2022-11-23T18:07:55Z

Description

I was dismayed to discover recently that UnsupportedInput and TypeMismatch are used extremely inconsistently across the codebase. UnsupportedInput is sometimes used for input type-checks (as per the name!!), but also used for argument type-checks. TypeMismatch is also used for both.
I thus devised the following standard: input type-checking only uses UnsupportedInput, and argument type-checking only uses TypeMismatch. Moreover, to differentiate them, UnsupportedInput now has two error arrows (spans), one pointing at the command and the other at the input origin, while TypeMismatch only has the one (because the command should always be nearby)
In order to apply that standard, a very large number of UnsupportedInput uses were changed so that the input's span could be retrieved and delivered to it.
Additionally, I noticed many places where errors are not propagated correctly: there are lots of match sites which take a Value::Error, then throw it away and replace it with a new Value::Error with less/misleading information (such as reporting the error as an "incorrect type"). I believe that the earliest errors are the most important, and should always be propagated where possible.
Also, to standardise one broad subset of UnsupportedInput error messages, who all used slightly different wordings of "expected <type>, got <type>", I created OnlySupportsThisInputType as a variant of it.
Finally, a bunch of error sites that had "repeated spans" - i.e. where an error expected two spans, but call.head was given for both - were fixed to use different spans.

Example

BEFORE

〉20b | str starts-with 'a'
Error: nu::shell::unsupported_input (link)

  × Unsupported input
   ╭─[entry #31:1:1]
 1 │ 20b | str starts-with 'a'
   ·   ┬
   ·   ╰── Input's type is filesize. This command only works with strings.
   ╰────

〉'a' | math cos
Error: nu::shell::unsupported_input (link)

  × Unsupported input
   ╭─[entry #33:1:1]
 1 │ 'a' | math cos
   · ─┬─
   ·  ╰── Only numerical values are supported, input type: String
   ╰────

〉0x[12] | encode utf8
Error: nu::shell::unsupported_input (link)

  × Unsupported input
   ╭─[entry #38:1:1]
 1 │ 0x[12] | encode utf8
   ·          ───┬──
   ·             ╰── non-string input
   ╰────

AFTER

〉20b | str starts-with 'a'
Error: nu::shell::pipeline_mismatch (link)

  × Pipeline mismatch.
   ╭─[entry #1:1:1]
 1 │ 20b | str starts-with 'a'
   ·   ┬   ───────┬───────
   ·   │          ╰── only string input data is supported
   ·   ╰── input type: filesize
   ╰────

〉'a' | math cos
Error: nu::shell::pipeline_mismatch (link)

  × Pipeline mismatch.
   ╭─[entry #2:1:1]
 1 │ 'a' | math cos
   · ─┬─   ────┬───
   ·  │        ╰── only numeric input data is supported
   ·  ╰── input type: string
   ╰────

〉0x[12] | encode utf8
Error: nu::shell::pipeline_mismatch (link)

  × Pipeline mismatch.
   ╭─[entry #3:1:1]
 1 │ 0x[12] | encode utf8
   · ───┬──   ───┬──
   ·    │        ╰── only string input data is supported
   ·    ╰── input type: binary
   ╰────

User-Facing Changes

Various error messages suddenly make more sense (i.e. have two arrows instead of one).

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 -A clippy::needless_collect to check that you're using the standard code style
cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

sholderbach · 2022-12-01T12:48:16Z

Good stuff!

My only concern is that we also have places where ShellError::UnsupportedInput is used to indicate a problem with the particular input's value and we don't have a specific ShellError variant like NeedsPositiveValue/InvalidProbability. (I recently chose to use UnsupportedInput in #7258 as I found that it was the most fitting.)

If I understand your change correctly the additional variant you added is for indicating the only valid types. Maybe we should also split up UnsupportedInput into two variants for TypeError and ValueError for better handling.

EDIT: Totallly missed that we kind of already have a variant for a input type error: PipelineMismatch
The naming doesn't seem to be too great if we don't use it consistently.

webbedspace · 2022-12-01T13:02:47Z

Totallly missed that we kind of already have a variant for a input type error: [PipelineMismatch](https://docs.rs/nu-protocol/latest/nu_protocol/enum.ShellError.html#variant.PipelineMismatch)

Hmm… maybe I should change my new UnsupportedInput uses to use that… actually, it's so similar to what I've edited UnsupportedInput to, that maybe I'll convert all PipelineMismatch uses to UnsupportedInput, then rename it in entirety to PipelineMismatch (i.e. eliminate UnsupportedInput due to both its cases being covered by -Mismatch variants)… Then again, you are right that people seem to "like" the name UnsupportedInput more…

sholderbach · 2022-12-01T13:05:53Z

We still need a variant to signal that the type was fine but the values in the input won't work with the command.

webbedspace · 2022-12-18T15:37:35Z

OK, so… in the end I just used a bunch of .expect()s for unwrapping known non-Error Values' spans, due to realising that the issue was Clippy and not the type system itself.

Also, I added ShellError::PipelineEmpty as another variant that was needed for date format. Then, I changed several conversion commands (namely the into, str, bytes, bits, path subcommands) to throw ShellError::PipelineEmpty when given Pipeline::Empty (but not Value::Nothing) for consistency.

Regarding PipelineMismatch versus UnsupportedInput: what I did was change OnlySupportsThisInputType to produce a PipelineMismatch instead of an UnsupportedInput. As such, all UnsupportedInputs now correspond to value errors rather than type errors.

It now passes all the tests, so it's where I want it to be.

rgwood · 2022-12-20T05:53:21Z

Nice, thanks @webbedspace! This looks like a big improvement to our error messages.

I will take a closer look at this tomorrow but I don't expect any issues. Once the remaining conflicts are fixed, should be able to merge this after the v0.73 release (which is happening tomorrow).

webbedspace · 2022-12-21T08:15:19Z

Random question, but why does the CI run nu-fmt-clippy on three platforms? Isn't that just a code style test?

sholderbach · 2022-12-21T19:50:05Z

Random question, but why does the CI run nu-fmt-clippy on three platforms? Isn't that just a code style test?

IIRC clippy can only check the code that is the result of conditional compilation so we have to excercise the different cfg() paths separately.

Thanks for all the meticulous work on this PR!

crates/nu-command/src/conversions/into/cellpath.rs

crates/nu-command/src/charting/histogram.rs

* Change various .span().expect() into a special .expect_span() call. * Correct histogram error to CantFindColumn * Fix places where PipelineData::Empty wasn't matched

webbedspace · 2022-12-23T05:31:15Z

I changed the "bunch of .expect()s" into a bunch of .expect_span() calls (new method), that puts the expect string in a single place (this doesn't change any semantics).

rgwood

This PR took a very long time to read through, so I can only imagine how much work it was to implement.

Incredible work, @webbedspace - this is a big step forward for Nu's errors. Thank you!

I've left a few small comments but I don't think they should hold up merging this. I think it's ready to go.

rgwood · 2022-12-23T06:14:05Z

crates/nu-command/src/conversions/into/filesize.rs

+        // Propagate existing errors
+        input.clone()


I'm a little iffy on this assumption that no span will always mean an error - feels like the kind of thing that might cause trouble in the future. Not a big enough deal to be a blocker though.

rgwood · 2022-12-23T06:24:00Z

crates/nu-command/src/formats/from/eml.rs

    head: Span,
 ) -> Result<PipelineData, ShellError> {
-    let (value, metadata) = input.collect_string_strict(head)?;
+    let (value, _span, metadata, ..) = input.collect_string_strict(head)?;


Don't think this .. is needed?

rgwood · 2022-12-23T06:33:49Z

crates/nu-command/src/input_handler.rs

-        None => input.map(move |v| cmd(&v, &arg, v.span().unwrap_or(span)), ctrlc),
+        None => input.map(
+            move |v| {
+                match v {
+                    // Propagate errors inside the input
+                    Value::Error { .. } => v,
+                    _ => cmd(&v, &arg, span),
+                }
+            },
+            ctrlc,
+        ),


I don't have any reason to believe that this will cause problems, but it's an interesting change that affects a lot of commands. Something to take a look at if anything breaks unexpectedly.

webbedspace force-pushed the dont_unwrap branch from 4c6e1fe to 4e95807 Compare December 18, 2022 15:32

webbedspace marked this pull request as ready for review December 18, 2022 15:40

webbedspace force-pushed the dont_unwrap branch from ee8539a to b0b2243 Compare December 20, 2022 06:04

sholderbach reviewed Dec 21, 2022

View reviewed changes

crates/nu-command/src/conversions/into/cellpath.rs Outdated Show resolved Hide resolved

webbedspace added 9 commits December 22, 2022 15:33

Initial commit: standardise use of UnsupportedValue() and TypeMismatch()

fb071c9

Apply changes to new commands

210113b

Further cleanup

f777d66

Fix several test failures

ee00d0b

Add comment

cd81a80

Remove invalid file

bdbf29a

Revert Cargo.lock

038f8ed

Clippy

b495a35

Remove cellpath.rs

247311d

webbedspace force-pushed the dont_unwrap branch from 5563db0 to 247311d Compare December 22, 2022 05:54

rgwood reviewed Dec 22, 2022

View reviewed changes

crates/nu-command/src/charting/histogram.rs Outdated Show resolved Hide resolved

webbedspace added 2 commits December 23, 2022 15:24

Some fixes

e5f8e76

* Change various .span().expect() into a special .expect_span() call. * Correct histogram error to CantFindColumn * Fix places where PipelineData::Empty wasn't matched

Clippy

0e57e3f

rgwood approved these changes Dec 23, 2022

View reviewed changes

rgwood merged commit dd7b731 into nushell:main Dec 23, 2022

webbedspace deleted the dont_unwrap branch December 23, 2022 07:05

sholderbach mentioned this pull request Dec 28, 2022

Piping records into from nuon or from json produces bizarre errors #6852

Closed

Conversation

webbedspace commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

sholderbach commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webbedspace commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sholderbach commented Dec 1, 2022

Uh oh!

webbedspace commented Dec 18, 2022

Uh oh!

rgwood commented Dec 20, 2022

Uh oh!

webbedspace commented Dec 21, 2022

Uh oh!

sholderbach commented Dec 21, 2022

Uh oh!

Uh oh!

Uh oh!

webbedspace commented Dec 23, 2022

Uh oh!

rgwood left a comment

Choose a reason for hiding this comment

Uh oh!

rgwood Dec 23, 2022

Choose a reason for hiding this comment

Uh oh!

rgwood Dec 23, 2022

Choose a reason for hiding this comment

Uh oh!

rgwood Dec 23, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

webbedspace commented Nov 23, 2022 •

edited

Loading

sholderbach commented Dec 1, 2022 •

edited

Loading

webbedspace commented Dec 1, 2022 •

edited

Loading