Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong signature parsing in predecl scan #10605

Closed
amtoine opened this issue Oct 4, 2023 · 11 comments · Fixed by #10637 or #10642
Closed

Wrong signature parsing in predecl scan #10605

amtoine opened this issue Oct 4, 2023 · 11 comments · Fixed by #10637 or #10642
Assignees
Labels
🐛 bug Something isn't working parser Issues related to parsing
Milestone

Comments

@amtoine
Copy link
Member

amtoine commented Oct 4, 2023

related to

cc/ @kubouch

Describe the bug

i have a script that broke when upgrading today.
i did bisect recent commits of Nushell and found the culprit 😏

it appears #10566 changed how order of definitions matter in a script 😮

when calling a command that is defined later in the script i get a confusing missing_positional error.
the only fix i found so far was to move the command definition above the command call and then it starts working as before 👌

How to reproduce

  1. save the following script as foo.nu
def main [] {
    foo
}

def foo []: nothing -> nothing { }
  1. run nu foo.nu
Error: nu::parser::missing_positional

  × Missing required positional argument.
   ╭─[foo.nu:1:1]
 1 │ def main [] {
 2 │     foo
   ·        ▲
   ·        ╰── missing ]
 3 │ }
   ╰────
  help: Usage: foo <]>
  1. change the signature of foo to [nothing -> nothing]
  2. run nu foo.nu
Error: nu::parser::missing_positional

  × Missing required positional argument.
   ╭─[foo.nu:1:1]
 1 │ def main [] {
 2 │     foo
   ·        ▲
   ·        ╰── missing nothing
 3 │ }
   ╰────
  help: Usage: foo <nothing> <nothing>

Expected behavior

i expected one of the two following behaviours

Error: nu::shell::external_command

  × External command failed
   ╭─[foo.nu:1:1]
 1 │ def main [] {
 2 │     foo
   ·     ─┬─
   ·      ╰── executable was not found
 3 │ }
   ╰────
  help: No such file or directory (os error 2)

Screenshots

No response

Configuration

key value
version 0.85.1
branch main
commit_hash 8c507dc
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.70.0 (90c541806 2023-05-31)
rust_channel 1.70.0-x86_64-unknown-linux-gnu
cargo_version cargo 1.70.0 (ec8a8a0ca 2023-04-25)
build_time 2023-10-04 18:35:49 +02:00
build_rust_channel release
allocator mimalloc
features default, sqlite, trash, which, zip
installed_plugins gstat, nu_plugin_explore

Additional context

No response

@amtoine amtoine added 🐛 bug Something isn't working needs-triage An issue that hasn't had any proper look labels Oct 4, 2023
@kubouch
Copy link
Contributor

kubouch commented Oct 4, 2023

It seems the parsing got broken somehow, I'll need to take a look.

The ordering should be unchanged. What happens is that if there is an error during parsing in a predecl scan, the definition won't be predeclared, that's why it complains about executable not found because the command was not added. I'll need to look at it because the foo syntax seems correct.

@kubouch kubouch self-assigned this Oct 4, 2023
@kubouch kubouch added parser Issues related to parsing and removed needs-triage An issue that hasn't had any proper look labels Oct 4, 2023
@amtoine
Copy link
Member Author

amtoine commented Oct 4, 2023

that's about what i understood yup, when you have time 😌

@kubouch kubouch changed the title order of definitions matters in scripts now Wrong signature parsing in predecl scan Oct 7, 2023
@hustcer hustcer added this to the v0.86.0 milestone Oct 7, 2023
@amtoine
Copy link
Member Author

amtoine commented Oct 7, 2023

@kubouch
i do not think this issue has been fixed by #10637 😮

with tmux-sessionizer.nu in my $env.PATH, i still get the same error 🤔

> tmux-sessionizer.nu list-sessions
Error: nu::parser::missing_positional

  × Missing required positional argument.
    ╭─[/home/amtoine/.local/share/nupm/scripts/tmux-sessionizer.nu:20:1]
 20 
 21      if $session.name not-in (list-sessions | get name) {
    ·                                           
    ·                                           ╰── missing nothing
 22          log debug $"creating session ($session.name) at ($session.path)"
    ╰────
  help: Usage: list-sessions {flags} <nothing> <table> <nothing> <table>

my nu-goat-scripts have been installed with nupm and the script is in $env.NUPM_HOME | path join "scripts" 👍

@amtoine amtoine reopened this Oct 7, 2023
@amtoine
Copy link
Member Author

amtoine commented Oct 7, 2023

and nuls gives me two diagnostics

Diagnostics:
1. Missing required positional argument.

on lines 21 and 58 of tmux-sessionizer.nu which are the two only calls to list-sessions that are above the definition of the command itself 😏

amtoine added a commit to amtoine/scripts that referenced this issue Oct 7, 2023
@kubouch
Copy link
Contributor

kubouch commented Oct 7, 2023

Could you create a minimal example that reproduces the error? The script is quite large and I also don't use tmux on this computer, I can't test it.

@amtoine
Copy link
Member Author

amtoine commented Oct 8, 2023

Could you create a minimal example that reproduces the error? The script is quite large and I also don't use tmux on this computer, I can't test it.

yes of course, my bad 😌

i brought it down to the following script 😏

#!/usr/bin/env nu
def main [] {
    foo
}

def foo []: [nothing -> table, nothing -> table] {
    []
}

which gives me

Error: nu::parser::missing_positional

  × Missing required positional argument.
   ╭─[/home/amtoine/foo.nu:2:1]
 2 │ def main [] {
 3 │     foo
   ·        ▲
   ·        ╰── missing nothing
 4 │ }
   ╰────
  help: Usage: foo {flags} <nothing> <table> <nothing> <table>

@amtoine
Copy link
Member Author

amtoine commented Oct 8, 2023

looks like it's the multi signature which gives an unhelpful error 😮

#!/usr/bin/env nu
def main [] {
    foo
}

def foo []: nothing -> table {
    []
}

returns an empty list as expected 🤔

@amtoine
Copy link
Member Author

amtoine commented Oct 8, 2023

even if i change the signature to something with different types, still the same error

#!/usr/bin/env nu
def main [] {
    foo
}

def foo []: [nothing -> table, string -> list] {
    []
}

@amtoine
Copy link
Member Author

amtoine commented Oct 8, 2023

and for each one of these examples, putting foo before main always solves the issue 👍

@kubouch
Copy link
Contributor

kubouch commented Oct 8, 2023

OK, thanks. The ordering matters in this case because before parsing def foo, foo is called whose signature is parsed by the predeclaration scan. If you put def foo to the top, it's parsed as a regular declaration which works fine.

The original error was present even before the def --env --wrapped PR btw.

@amtoine
Copy link
Member Author

amtoine commented Oct 8, 2023

OK, thanks. The ordering matters in this case because before parsing def foo, foo is called whose signature is parsed by the predeclaration scan. If you put def foo to the top, it's parsed as a regular declaration which works fine.

ooh i see 👍

The original error was present even before the def --env --wrapped PR btw.

strange because i use this script each time i open a terminal every day and never had this issue 🤔
and Git bisecting Nushell gave me #10566 😮

amtoine pushed a commit that referenced this issue Oct 8, 2023
# Description
Fixes #10605 (again).

The loop looking for `[` to determine signature position didn't stop
early enough, so it thought the second `[` denoting the inp/out types
marks the beginning of the signature.

# User-Facing Changes

# Tests + Formatting
adds a new `predecl_signature_multiple_inp_out_types` test

# After Submitting
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description
Fixes nushell#10605 (again).

The loop looking for `[` to determine signature position didn't stop
early enough, so it thought the second `[` denoting the inp/out types
marks the beginning of the signature.

# User-Facing Changes

# Tests + Formatting
adds a new `predecl_signature_multiple_inp_out_types` test

# After Submitting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working parser Issues related to parsing
Projects
None yet
3 participants