Skip to content

Prevent each from swallowing errors when eval_block returns a ListStream#12412

Merged
fdncred merged 2 commits intonushell:mainfrom
merelymyself:patch-1
May 1, 2024
Merged

Prevent each from swallowing errors when eval_block returns a ListStream#12412
fdncred merged 2 commits intonushell:mainfrom
merelymyself:patch-1

Conversation

@merelymyself
Copy link
Copy Markdown
Contributor

Description

Prior, it seemed that nested errors would not get detected and shown. This PR fixes that.

Resolves #10176:

~/CodingProjects/nushell> [[1,2]] | each {|x| $x | each {|y| error make {msg: "oh noes"} } }                        05/04/2024 21:34:08
Error: nu::shell::eval_block_with_input

  × Eval block failed with pipeline input
   ╭─[entry #1:1:3]
 1 │ [[1,2]] | each {|x| $x | each {|y| error make {msg: "oh noes"} } }
   ·   ┬
   ·   ╰── source value
   ╰────

Error:   × oh noes
   ╭─[entry #1:1:36]
 1 │ [[1,2]] | each {|x| $x | each {|y| error make {msg: "oh noes"} } }
   ·                                    ─────┬────
   ·                                         ╰── originates from here
   ╰────

Resolves #11224:

~/CodingProjects/nushell> [0] | each { |_|                                                                          05/04/2024 21:35:40
:::     [0] | each { |_|
:::         non-existent-command
:::     }
::: }
Error: nu::shell::eval_block_with_input

  × Eval block failed with pipeline input
   ╭─[entry #1:2:6]
 1 │ [0] | each { |_|
 2 │     [0] | each { |_|
   ·      ┬
   ·      ╰── source value
 3 │         non-existent-command
   ╰────

Error: nu::shell::external_command

  × External command failed
   ╭─[entry #1:3:9]
 2 │     [0] | each { |_|
 3 │         non-existent-command
   ·         ──────────┬─────────
   ·                   ╰── executable was not found
 4 │     }
   ╰────
  help: No such file or directory (os error 2)

User-Facing Changes

Tests + Formatting

After Submitting

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 6, 2024

Have you done any performance testing to see if this decreases the performance of table/each?

@merelymyself
Copy link
Copy Markdown
Contributor Author

Yes, there is a noticeable decrease in the performance of table. Drafting until resolved.

@merelymyself merelymyself marked this pull request as draft April 8, 2024 05:48
@merelymyself merelymyself force-pushed the patch-1 branch 2 times, most recently from 4721f45 to 0649d5b Compare April 12, 2024 16:44
@merelymyself merelymyself changed the title Prevent tables (and, by extension, each) from swallowing errors Prevent each from swallowing errors when eval_block returns a ListStream Apr 12, 2024
@merelymyself merelymyself marked this pull request as ready for review April 13, 2024 05:25
@merelymyself
Copy link
Copy Markdown
Contributor Author

With this change, the performance decrease is minimised - only in the specific case where a ListStream is fed into each is there a slight dip, but it shouldn't be very significant.

return Some(v.clone());
}
}
Some(Value::list(vals, span))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...Maybe collect s first is not really good. What about this?

let mut vals = vec![];
for v in s {
    if let Value::Error { .. } = v { return Some(v) }
    else { vals.push(v) }
}
Some(Value::list(vals, span))

@merelymyself
Copy link
Copy Markdown
Contributor Author

Rebased and accepted the suggestion. @fdncred @WindSoilder, take a look?

@WindSoilder
Copy link
Copy Markdown
Contributor

Thanks! It looks good to me. Since nushell will have a release in 5 days, I'd like to defer it after released

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 1, 2024

Thanks

@fdncred fdncred added notes:fixes Include the release notes summary in the "Bug fixes" section deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way labels May 1, 2024
@hustcer hustcer added this to the v0.94.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way notes:fixes Include the release notes summary in the "Bug fixes" section status:wait-until-after-nushell-release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error is not propagated from a nested each Nested each swallows errors

4 participants