FIX: do not allow *start > end* in `error make` spans by amtoine · Pull Request #8570 · nushell/nushell

amtoine · 2023-03-22T18:39:08Z

This should close #8567.

Description

this PR throws an error when start > end in the most complete branch of ErrorMake::run, i.e. when $.msg, $.label.text, $.label.start and $.label.end are defined.

i've also added a error_start_bigger_than_end_should_fail test to check that it does indeed return the right error.

User-Facing Changes

no more crash when manipulating span bounds and a clear error, e.g.

>_ error make {msg: "msg" label: {text: "text" start: 1010 end: 1000}}
Error:
  × invalid error format.
   ╭─[entry #3:1:1]
 1 │ error make {msg: "msg" label: {text: "text" start: 1010 end: 1000}}
   ·                               ──────────────────┬─────────────────
   ·                                                 ╰── `$.label.start` is stricly bigger than `$.label.end`
   ╰────
  help: 1010 > 1000

or

>_ error make {
:::     msg: "msg"
:::     label: {
:::         text: "text"
:::         start: ($nu.scope.engine_state.source_bytes - 90)
:::         end: ($nu.scope.engine_state.source_bytes - 100)
:::     }
::: }
Error:
  × invalid error format.
   ╭─[entry #4:2:1]
 2 │         msg: "msg"
 3 │ ╭─▶     label: {
 4 │ │           text: "text"
 5 │ │           start: ($nu.scope.engine_state.source_bytes - 90)
 6 │ │           end: ($nu.scope.engine_state.source_bytes - 100)
 7 │ ├─▶     }
   · ╰──── `$.label.start` is stricly bigger than `$.label.end`
 8 │     }
   ╰────
  help: 204525 > 204515

Tests + Formatting

🟢 toolkit fmt
🟢 toolkit clippy
🔴 toolkit test

After Submitting

$nothing

sholderbach

Wow that is a good one! Thanks. still traumatized from span errors....

amtoine · 2023-03-22T18:43:09Z

i just have these failures with toolkit test which look unrelated to this right? 🤔

i've tried merging into latest main but it did not solve the tests 👀

failures:

---- modules::module_import_env_2 stdout ----
=== stderr
Error: nu::shell::file_not_found

  × File not found
   ╭─[source:1:1]
 1 │ source-env main.nu; $env.FOO
   ·            ───┬───
   ·               ╰── file not found
   ╰────


thread 'modules::module_import_env_2' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"foo"`', tests/modules/mod.rs:341:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- modules::module_import_env_1 stdout ----
=== stderr
Error: nu::shell::file_not_found

  × File not found
   ╭─[source:1:1]
 1 │ source-env main.nu; use main.nu foo; foo
   ·            ───┬───
   ·               ╰── file not found
   ╰────


thread 'modules::module_import_env_1' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"foo"`', tests/modules/mod.rs:316:9

---- overlays::overlay_use_dont_cd_overlay stdout ----
=== stderr
Error: nu::shell::file_not_found

  × File not found
   ╭─[source:1:1]
 1 │ source-env test1/test2/spam.nu; $env.PWD | path basename
   ·            ─────────┬─────────
   ·                     ╰── file not found
   ╰────


thread 'overlays::overlay_use_dont_cd_overlay' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"overlay_use_dont_cd_overlay"`', tests/overlays/mod.rs:946:9

---- overlays::overlay_use_do_cd_file_relative stdout ----
=== stderr

thread 'overlays::overlay_use_do_cd_file_relative' panicked at 'assertion failed: `(left == right)`
  left: `"nu-utils"`,
 right: `"test1"`', tests/overlays/mod.rs:919:9

---- parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1 stdout ----
=== stderr
Error: nu::shell::file_not_found

  × File not found
   ╭─[source:1:1]
 1 │ source-env lol/lol.nu; $env.FOO
   ·            ─────┬────
   ·                 ╰── file not found
   ╰────


thread 'parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"good"`', tests/parsing/mod.rs:177:9

---- parsing::parse_file_relative_to_parsed_file_simple stdout ----
=== stderr
Error: nu::shell::file_not_found

  × File not found
   ╭─[source:1:1]
 1 │ source-env lol/lol/lol.nu; $env.LOL
   ·            ───────┬──────
   ·                   ╰── file not found
   ╰────


thread 'parsing::parse_file_relative_to_parsed_file_simple' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"lol"`', tests/parsing/mod.rs:99:9


failures:
    modules::module_import_env_1
    modules::module_import_env_2
    overlays::overlay_use_do_cd_file_relative
    overlays::overlay_use_dont_cd_overlay
    parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1
    parsing::parse_file_relative_to_parsed_file_simple

test result: FAILED. 386 passed; 6 failed; 20 ignored; 0 measured; 0 filtered out; finished in 5.01s

error: test failed, to rerun pass `-p nu --test main`

amtoine · 2023-03-22T18:44:33Z

Wow that is a good one!

do not ask me how i come to this crash, i have no idea myself 😆

Thanks.

my pleasure 😌

still traumatized from span errors....

i love them and i want them as pretty and reliable as possible 😏

This commit should make the CI typo check happy.

amtoine · 2023-03-22T18:47:14Z

i learn how to spell words in d501451 👀

amtoine · 2023-03-22T18:50:28Z

ok that is unfortunate 😬

sholderbach · 2023-03-22T18:54:57Z

crates/nu-cmd-lang/src/core_commands/error_make.rs

                            Some(ShellError::GenericError(
                                "invalid error format.".into(),
-                                "`$.label.start` is stricly bigger than `$.label.end`".into(),
+                                "`$.label.start` is strictly bigger than `$.label.end`".into(),


I would rephrase it in terms of how it shold be but that is a stylistic choice

ooh gotcha 👌

i changed it in 806158f 😉

codecov · 2023-03-22T19:20:59Z

Codecov Report

Merging #8570 (5b79a96) into main (626410b) will increase coverage by 0.37%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8570      +/-   ##
==========================================
+ Coverage   68.14%   68.51%   +0.37%     
==========================================
  Files         624      624              
  Lines      100678   100681       +3     
==========================================
+ Hits        68606    68985     +379     
+ Misses      32072    31696     -376

Impacted Files	Coverage Δ
crates/nu-cmd-lang/src/core_commands/error_make.rs	`62.14% <0.00%> (-3.33%)`	⬇️

... and 14 files with indirect coverage changes

fdncred · 2023-03-23T13:34:16Z

@amtoine if you can fix the type-o, we can probably land this. Thanks!

amtoine · 2023-03-23T17:08:14Z

@amtoine if you can fix the type-o, we can probably land this. Thanks!

~~i do not understand 😕~~
~~i fixed it in d501451 🤔~~

ok sorry, i forgot about the test 👀
fixed in 5b79a96 😌

sholderbach · 2023-03-23T19:30:59Z

Merci beaucoup!

amtoine · 2023-03-24T13:07:46Z

Merci beaucoup!

avec plaisir!

amtoine added 3 commits March 22, 2023 19:21

throw an error when $.label.start > $.label.end

a0da1fd

add a test with start > end that should fail

867a2e1

format the changes

9412d08

sholderbach approved these changes Mar 22, 2023

View reviewed changes

fix a typo in the error label

d501451

This commit should make the CI typo check happy.

amtoine closed this Mar 22, 2023

amtoine reopened this Mar 22, 2023

amtoine changed the title ~~Fix/error span/do not allow start bigger than end~~ FIX: do not allow *start > end* in error make spans Mar 22, 2023

sholderbach reviewed Mar 22, 2023

View reviewed changes

amtoine added 2 commits March 23, 2023 18:15

write a "should" message to make the error clearer

806158f

fix the test and the typo at once

5b79a96

sholderbach merged commit 05ff7a9 into nushell:main Mar 23, 2023

amtoine deleted the fix/error-span/do-not-allow-start-bigger-than-end branch March 24, 2023 13:07

amtoine mentioned this pull request Apr 1, 2023

add a note about better error make nushell/nushell.github.io#849

Merged

Conversation

amtoine commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

amtoine commented Mar 22, 2023

Uh oh!

amtoine commented Mar 22, 2023

Uh oh!

amtoine commented Mar 22, 2023

Uh oh!

amtoine commented Mar 22, 2023

Uh oh!

sholderbach Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fdncred commented Mar 23, 2023

Uh oh!

amtoine commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sholderbach commented Mar 23, 2023

Uh oh!

amtoine commented Mar 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amtoine commented Mar 22, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

amtoine commented Mar 23, 2023 •

edited

Loading