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

String script args with parenthesis are mangled #10908

Closed
pcrock-thmdo opened this issue Nov 1, 2023 · 4 comments · Fixed by #11030
Closed

String script args with parenthesis are mangled #10908

pcrock-thmdo opened this issue Nov 1, 2023 · 4 comments · Fixed by #11030
Labels
🐛 bug Something isn't working quoting/expansion Issues related to string quoting and expansion of variable or glob patterns scripting Related to writing scripts
Milestone

Comments

@pcrock-thmdo
Copy link

Describe the bug

Passing string args with parenthesis to a nushell script can cause the args to be mangled.

How to reproduce

example echo.nu script:

#!/usr/bin/env nu

def main [input: string] {
    print $input
}

here's how it behaves when you run it:

./echo.nu "hi"
# output: hi

./echo.nu "hi(1)"
# output: hi1

Expected behavior

./echo.nu "hi(1)"
# output: hi(1)

Screenshots

No response

Configuration

key value
version 0.86.0
branch
commit_hash
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.72.1 (d5c2e9c34 2023-09-13) (Homebrew)
cargo_version cargo 1.72.1
build_time 2023-10-17 17:08:20 +00:00
build_rust_channel release
allocator mimalloc
features dataframe, default, sqlite, trash, which, zip
installed_plugins

Additional context

I noticed this when my browser downloaded a file twice, and it added the (1) to the end of the file name just before the extension. i.e. the_file(1).log was mangled into the_file1.log. Passing this file path to a script caused the script to fail because the_file1.log doesn't exist.

@pcrock-thmdo pcrock-thmdo added the needs-triage An issue that hasn't had any proper look label Nov 1, 2023
@fdncred fdncred added the 🐛 bug Something isn't working label Nov 1, 2023
@amtoine amtoine added quoting/expansion Issues related to string quoting and expansion of variable or glob patterns scripting Related to writing scripts and removed needs-triage An issue that hasn't had any proper look labels Nov 1, 2023
@amtoine
Copy link
Member

amtoine commented Nov 1, 2023

good catch @pcrock-thmdo

i couldn't reproduce this in the REPL nor as a module, so i'd say it's an issue with Nushell scripts, not custom commands or modules 👌

@MarikaChlebowska
Copy link
Contributor

I experimented with it and interestingly ./echo.nu "hi (1)" works as expected. The difference between calling it directly and in a script from what I see if that the variable is put on stack in the second case and on stack it's already without parentheses. I think it's the way arguments are parsed somewhere in low level code but so far I haven't identified where is the logic that needs to be fixed. And also that's a change which can break some things potentially.

@MarikaChlebowska
Copy link
Contributor

MarikaChlebowska commented Nov 5, 2023

I think I found where things go wrong. Before entering parse_string "1232 (1)" (with space) is implicitly transformed to `1232 (1)` which is considered just string and works fine. The version without space has quotation marks stripped and is just 1232(1) which is parsed as StringInterpolation. The only thing I don't know yet is where does the implicit transformation happen but I spent enough time today on this. Also I'm not sure if StringInterpolation parsing is broken or the conversion shouldn't take place.

@MarikaChlebowska
Copy link
Contributor

I finally found it, it's an issue with deparser which should add backticks to non string interpolation values with brackets. I'll post a PR for it soon.

fdncred pushed a commit that referenced this issue Jan 19, 2024
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
If there were brackets in a string argument of a script it was always
interpreted as interpolation before the change. That lead to unexpected
outputs of such scripts. After this change arguments which are not
intended as interpolation (not starting with $) and containing brackets
will have implicitly added backticks for correct interpretation in the
scripts. This fixes #10908.

To fix other issues mentioned in #11035 I changed the deparsing logic.
Initially we added backticks for multi word variables and double quote
if there was \ or " in the string. My change would add double quotes any
time string starts with $ or contains any of character that might break
parsing. The characters I identified are white space, (, ', `, ",and \.
It's possible other characters should be added to this list.

I tested this solution with few simple scripts using both stand alone
arguments and flags and it seems to work but I would appreciate if
someone with more experience checked it with some more unusual cases I
missed.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->
Erroneous behaviour described  in the issue will no longer happen.

# 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](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `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
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
Added tests for new formatting.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
@hustcer hustcer added this to the v0.90.0 milestone Jan 20, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
If there were brackets in a string argument of a script it was always
interpreted as interpolation before the change. That lead to unexpected
outputs of such scripts. After this change arguments which are not
intended as interpolation (not starting with $) and containing brackets
will have implicitly added backticks for correct interpretation in the
scripts. This fixes nushell#10908.

To fix other issues mentioned in nushell#11035 I changed the deparsing logic.
Initially we added backticks for multi word variables and double quote
if there was \ or " in the string. My change would add double quotes any
time string starts with $ or contains any of character that might break
parsing. The characters I identified are white space, (, ', `, ",and \.
It's possible other characters should be added to this list.

I tested this solution with few simple scripts using both stand alone
arguments and flags and it seems to work but I would appreciate if
someone with more experience checked it with some more unusual cases I
missed.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->
Erroneous behaviour described  in the issue will no longer happen.

# 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](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `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
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
Added tests for new formatting.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working quoting/expansion Issues related to string quoting and expansion of variable or glob patterns scripting Related to writing scripts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants