Fix panic on too few arguments for custom function by anka-213 · Pull Request #10395 · nushell/nushell

anka-213 · 2023-09-16T19:11:50Z

Description

Old code was comparing remaining positional arguments with total number
of arguments, where it should've compared remaining positional with
with remaining arguments of any kind. This means that if a function was given too few arguments, calculate_end_span would believe that it actually had too many arguments, since after parsing the first few arguments, the number of remaining arguments needed were fewer than the total number of arguments, of which we had used several.

Fixes #9072
Fixes: #13930
Fixes: #12069
Fixes: #8385

Extracted from #10381

Bonus

It also improves the error handling on missing positional arguments before keywords (no longer crashing since #9851). Instead of just giving the keyword to the parser for the missing positional, we give an explicit error about a missing positional argument. I would like better descriptions than "missing var_name" though, but I'm not sure if that's available without

Old error

Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry #1:1:1]
 1 │ let = if foo
   ·     ┬
   ·     ╰── expected valid variable name
   ╰────

New error

Error: nu::parser::missing_positional

  × Missing required positional argument.
   ╭─[entry #18:1:1]
 1 │ let = foo
   ·    ┬
   ·    ╰── missing var_name
   ╰────
  help: Usage: let <var_name> = <initial_value>

User-Facing Changes

The program alias = = = is no longer accepted by the parser

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 to check that you're using the standard code style
cargo test --workspace to check that all tests pass (on Windows make sure to enable developer mode)
cargo run -- -c "use std testing; testing run-tests --path crates/nu-std" to run the tests for the standard library

Note
from nushell you can also use the toolkit as follows
use toolkit.nu  # or use an `env_change` hook to activate it automatically
toolkit check pr

-->

After Submitting

anka-213 · 2023-09-19T17:12:08Z

If the changes in error message for missing arguments before keywords is controversial, I can remove/extract it to a separate PR, since the actual fix to the crash is just the first commit with a single line changed + added tests.

amtoine

not an expert on the parser at all, but from what i'm seeing in the tests and the PR description

the bug appears to be fixed
tests look good
i think we can live without this cursed alias = = = 😆

thanks for tackling the parser @anka-213 🙏

anka-213 · 2024-06-29T12:23:29Z

Is there anything (other than the merge conflict in the tests) that is preventing this fix from being merged?

WindSoilder

Hi, really thank you for the pr.
I'm sorry that it takes too much time to review it. I left a single question, and some comment.

WindSoilder · 2024-09-26T16:10:16Z

crates/nu-parser/src/parser.rs


            let positionals_between = kw_pos - positional_idx - 1;
-            if positionals_between > (kw_idx - spans_idx) {
+            if positionals_between >= (kw_idx - spans_idx) {


Why >= is required?

I should have documented it when I wrote this change, as I don't remember. 😅
The commit this change comes from says:

Handle multiple missing args before a keyword
calculate_end_span should only return spans_idx when the current
span is a keyword, but we require more positional before the keyword

But I'll have to think a bit more to remember why that meant that we want >=. I should've also added another testcase for that specific change, since it's a separate fix from the previous ones.

Yeah I have the question because I tried something like let = 3 without this change, it also reports new error message.

Yeah, I unfortunately still don't remember. I believe I had some reason for it, but it was a year ago and I didn't write enough down. I tried changing it to panic on == and running the fuzzer to see when the equals case would happen with no success. My commit message unfortunately wasn't clear enough for me to understand it, since I no longer remember how the parser works.

I would be fine with either reverting that change or keeping it. The test cases succeed in both cases, so it only makes a difference in some untested case (if ever).

The case of equality did trigger in a few tests, but for all of those, positionals_between was equal to 0, which means that the result of the two variants are identical.

It could be the case that the change only matters if there is a command with more than one required argument before a keyword argument, which there are currently none. But after trying out adding such a command, those seem to be broken enough to not even trigger this code path at all (I should ensure that there is no regression for that in this patch, even though no such command currently exist). That turned out to just be an issue with special-casing of the keyword "let". I should've tried a custom keyword instead.

Ah ok. Thank you for the detailed information. If you believe you had some reason for it, I think we can keep it.

It could be the case that the change only matters if there is a command with more than one required argument before a keyword argument, which there are currently none. But after trying out adding such a command, those seem to be broken enough to not even trigger this code path at all (I should ensure that there is no regression for that in this patch, even though no such command currently exist).

Yeah I believe the parser had been changed a lot from the pr you created before. So it might be checked and broken elsewhere.

crates/nu-parser/src/parser.rs

src/tests/test_parser.rs

anka-213 · 2024-09-27T07:09:05Z

This PR could be split into two PRs. one for the two first commits (the second commit is just a refactoring on top of the first). and another for the final two commits. I don't know if that would've made reviewing easier? (but the later changes depend on the earlier, so this was the easiest for me)

Old code was comparing remaining positional arguments with total number of arguments, where it should've compared remaining positional with with remaining arguments of any kind Fixes nushell#9072

All the branching can be reduced to saturated subtraction

calculate_end_span should only return spans_idx when the current span is a keyword, but we require more positional before the keyword

WindSoilder · 2024-09-27T07:58:38Z

This PR could be split into two PRs. one for the two first commits (the second commit is just a refactoring on top of the first). and another for the final two commits. I don't know if that would've made reviewing easier? (but the later changes depend on the earlier, so this was the easiest for me)

I think it's ok to be one PR. One is because the new error message seems fine to me, and the change is small enough, I can review them together.

WindSoilder

Really appreciate for the pr! It looks good to me :-)

anka-213 mentioned this pull request Sep 16, 2023

Reduce use of unchecked indexing in parser code #10381

Closed

5 tasks

anka-213 changed the title ~~Fix logic error in calculate_end_span in parser~~ Fix panic on too few arguments for custom function Sep 18, 2023

amtoine reviewed Sep 23, 2023

View reviewed changes

fdncred mentioned this pull request Sep 25, 2024

fix(nu-parser): REPL panics when function has exactly 6 arguments #13934

Closed

7 tasks

WindSoilder requested changes Sep 26, 2024

View reviewed changes

anka-213 commented Sep 27, 2024

View reviewed changes

src/tests/test_parser.rs Show resolved Hide resolved

anka-213 added 6 commits September 27, 2024 09:39

Fix logic error in calculate_end_span

93de5a0

Old code was comparing remaining positional arguments with total number of arguments, where it should've compared remaining positional with with remaining arguments of any kind Fixes nushell#9072

Simplify logic in calculate_end_span

980494a

All the branching can be reduced to saturated subtraction

Handle multiple missing args before a keyword

bc561b4

calculate_end_span should only return spans_idx when the current span is a keyword, but we require more positional before the keyword

Update tests to new error messages

9b69855

Update code to latest version

07f8c6f

Remove now redundant function

4e2e125

anka-213 force-pushed the fix-9072 branch from f489ae0 to 4e2e125 Compare September 27, 2024 08:35

WindSoilder approved these changes Sep 27, 2024

View reviewed changes

WindSoilder merged commit 8200831 into nushell:main Sep 27, 2024

hustcer added this to the v0.99.0 milestone Sep 28, 2024

Conversation

anka-213 commented Sep 16, 2023 • edited by WindSoilder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Bonus

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

anka-213 commented Sep 19, 2023

Uh oh!

amtoine left a comment

Choose a reason for hiding this comment

Uh oh!

anka-213 commented Jun 29, 2024

Uh oh!

WindSoilder left a comment

Choose a reason for hiding this comment

Uh oh!

WindSoilder Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

anka-213 Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

WindSoilder Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

anka-213 Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WindSoilder Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anka-213 commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WindSoilder commented Sep 27, 2024

Uh oh!

WindSoilder left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anka-213 commented Sep 16, 2023 •

edited by WindSoilder

Loading

anka-213 Sep 27, 2024 •

edited

Loading

anka-213 commented Sep 27, 2024 •

edited

Loading