Skip to content

Parse time type checking for range#13595

Merged
WindSoilder merged 5 commits intonushell:mainfrom
ysthakur:range-typecheck
Aug 13, 2024
Merged

Parse time type checking for range#13595
WindSoilder merged 5 commits intonushell:mainfrom
ysthakur:range-typecheck

Conversation

@ysthakur
Copy link
Copy Markdown
Member

@ysthakur ysthakur commented Aug 12, 2024

Description

As part of fixing #13586, this PR checks the types of the operands when creating a range. Stuff like 0..(glob .) will be rejected at parse time. Additionally, 0..$x will be treated as a range and rejected if x is not defined, rather than being treated as a string. A separate PR will need to be made to do reject streams at runtime, so that stuff like 0..(open /dev/random) doesn't hang.

Internally, this PR adds a ParseError::UnsupportedOperationTernary variant, for when you have a range like 1..2..(glob .).

User-Facing Changes

Users will now receive an error if any of the operands in the ranges they construct have types that aren't compatible with Type::Number.

Additionally, if a piece of code looks like a range but some parse error is encountered while parsing it, that piece of code will still be treated as a range and the user will be shown the parse error. This means that a piece of code like 0..$x will be treated as a range no matter what. Previously, if x weren't the expression would've been treated as a string "0..$x". I feel like it makes the language less complicated if we make it less context-sensitive.

Here's an example of the error you get:

> 0..(glob .)
Error: nu::parser::unsupported_operation

  × range is not supported between int and any.
   ╭─[entry #1:1:1]
 1 │ 0..(glob .)
   · ─────┬─────┬┬
   ·      │     │╰── any
   ·      │     ╰── int
   ·      ╰── doesn't support these values
   ╰────

And as an image:
image

Note: I made the operands themselves (above, (glob .)) be garbage, rather than the .. operator itself. This doesn't match the behavior of the math operators (if you do 1 + "foo", + gets highlighted red). This is because with ranges, the range operators aren't Expressions themselves, so they can't be turned into garbage. I felt like here, it makes more sense to highlight the individual operand anyway.

Tests + Formatting

Added some test cases for making sure that an unsupported operation parse error is triggered.

After Submitting


if token.starts_with("...") {
working_set.error(ParseError::Expected(
"range operator ('..'), got spread ('...')",
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.

Good catch! I think it's better to add a test for the behavior

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's actually covered by the test here: https://github.com/nushell/nushell/blob/main/tests/repl/test_spread.rs#L191-L194. Without this if statement, ...$rest is parsed a string "...$rest". With this change, it's parsed as a call to an external ...$rest and produces a command not found error.

Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I left some comments above

@WindSoilder WindSoilder added the notes:fixes Include the release notes summary in the "Bug fixes" section label Aug 12, 2024
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me now

@WindSoilder WindSoilder merged commit d5946a9 into nushell:main Aug 13, 2024
@ysthakur ysthakur deleted the range-typecheck branch August 13, 2024 09:36
@hustcer hustcer added this to the v0.97.0 milestone Aug 13, 2024
maxim-uvarov pushed a commit to maxim-uvarov/nushell that referenced this pull request Aug 14, 2024
# Description

As part of fixing nushell#13586, this
PR checks the types of the operands when creating a range. Stuff like
`0..(glob .)` will be rejected at parse time. Additionally, `0..$x` will
be treated as a range and rejected if `x` is not defined, rather than
being treated as a string. A separate PR will need to be made to do
reject streams at runtime, so that stuff like `0..(open /dev/random)`
doesn't hang.

Internally, this PR adds a `ParseError::UnsupportedOperationTernary`
variant, for when you have a range like `1..2..(glob .)`.

# User-Facing Changes

Users will now receive an error if any of the operands in the ranges
they construct have types that aren't compatible with `Type::Number`.

Additionally, if a piece of code looks like a range but some parse error
is encountered while parsing it, that piece of code will still be
treated as a range and the user will be shown the parse error. This
means that a piece of code like `0..$x` will be treated as a range no
matter what. Previously, if `x` weren't the expression would've been
treated as a string `"0..$x"`. I feel like it makes the language less
complicated if we make it less context-sensitive.

Here's an example of the error you get:
```
> 0..(glob .)
Error: nu::parser::unsupported_operation

  × range is not supported between int and any.
   ╭─[entry #1:1:1]
 1 │ 0..(glob .)
   · ─────┬─────┬┬
   ·      │     │╰── any
   ·      │     ╰── int
   ·      ╰── doesn't support these values
   ╰────
```

And as an image:

![image](https://github.com/user-attachments/assets/5c76168d-27db-481b-b541-861dac899dbf)

Note: I made the operands themselves (above, `(glob .)`) be garbage,
rather than the `..` operator itself. This doesn't match the behavior of
the math operators (if you do `1 + "foo"`, `+` gets highlighted red).
This is because with ranges, the range operators aren't `Expression`s
themselves, so they can't be turned into garbage. I felt like here, it
makes more sense to highlight the individual operand anyway.
@devyn
Copy link
Copy Markdown
Contributor

devyn commented Aug 21, 2024

Hi @ysthakur,

I think this inadvertently broke command paths containing ..:

../foo/bar/baz

should run a command, but now returns a string instead.

devyn pushed a commit that referenced this pull request Aug 21, 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.
-->

@devyn found that #13595, which
made ranges be type-checked at parse time, introduced a bug that caused
`../foo` to be parsed as a string rather than a command call. This was
caused by `parse_range` returning a `Some` despite there being parse
errors (`/foo` doesn't match `SyntaxShape::Number`). To go back to the
old behavior, `parse_range` now returns `None` anytime there's any parse
errors met while parsing the range.

Unfortunately, this means that something like `..$foo` will be parsed as
a string if `$foo` isn't defined and as a range if it is defined. That
was the behavior before #13595, and it should probably be fixed at some
point, but I'm just trying to quickly fix the bug.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

Things should go back to the way they were before #13595, except the
type-checking stuff from that PR is still here.

# 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 toolkit.nu; toolkit test stdlib"` 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 a test. Reverted another test that tests that `0..<$day` is parsed
successfully as a string if the variable isn't defined.

# 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

notes:fixes Include the release notes summary in the "Bug fixes" section

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants