Skip to content

Rework operator type errors#14429

Merged
IanManske merged 23 commits intonushell:mainfrom
IanManske:operator-parser-error
Feb 13, 2025
Merged

Rework operator type errors#14429
IanManske merged 23 commits intonushell:mainfrom
IanManske:operator-parser-error

Conversation

@IanManske
Copy link
Copy Markdown
Member

@IanManske IanManske commented Nov 24, 2024

Description

This PR adds two new ParseError and ShellError cases for type errors relating to operators.

  • OperatorUnsupportedType is used when a type is not supported by an operator in any way, shape, or form. E.g., + does not support bool.
  • OperatorIncompatibleTypes is used when a operator is used with types it supports, but the combination of types provided cannot be used together. E.g., filesize + duration is not a valid combination.

The other preexisting error cases related to operators have been removed and replaced with the new ones above. Namely:

  • ShellError::OperatorMismatch
  • ShellError::UnsupportedOperator
  • ParseError::UnsupportedOperationLHS
  • ParseError::UnsupportedOperationRHS
  • ParseError::UnsupportedOperationTernary

User-Facing Changes

  • help operators now lists the precedence of not as 55 instead of 0 (above the other boolean operators). Fixes 'not' operator precedence displayed wrong #13675.
  • math median and math mode now ignore NaN values so that [NaN NaN] | math median and [NaN NaN] | math mode no longer trigger a type error. Instead, it's now an empty input error. Fixing this in earnest can be left for a future PR.
  • Comparisons with nan now return false instead of causing an error. E.g., 1 == nan is now false.
  • All the operator type errors have been standardized and reworked. In particular, they can now have a help message, which is currently used for types errors relating to ++.
[1] ++ 2
Error: nu::parser::operator_unsupported_type

  × The '++' operator does not work on values of type 'int'.
   ╭─[entry #1:1:5]
 1 │ [1] ++ 2
   ·     ─┬ ┬
   ·      │ ╰── int
   ·      ╰── does not support 'int'
   ╰────
  help: if you meant to append a value to a list or a record to a table, use the `append` command or wrap the value in a list. For example: `$list ++ $value` should be
        `$list ++ [$value]` or `$list | append $value`.

@IanManske IanManske added notes:fixes Include the release notes summary in the "Bug fixes" section deprecated:pr-errors Deprecated: use A:error-handling, A:error-unhelpful, or A:error-silent-fail instead labels Nov 24, 2024
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Nov 24, 2024

Correct me if I'm wrong, but the stdlib help operators needs to be updated too.

def get-all-operators [] { return [
[type, operator, name, description, precedence];
[Assignment, =, Assign, "Assigns a value to a variable.", 10]
[Assignment, +=, PlusAssign, "Adds a value to a variable.", 10]
[Assignment, ++=, ConcatAssign, "Concatenate two lists, two strings, or two binary values.", 10]
[Assignment, -=, MinusAssign, "Subtracts a value from a variable.", 10]
[Assignment, *=, MultiplyAssign, "Multiplies a variable by a value.", 10]
[Assignment, /=, DivideAssign, "Divides a variable by a value.", 10]
[Comparison, ==, Equal, "Checks if two values are equal.", 80]
[Comparison, !=, NotEqual, "Checks if two values are not equal.", 80]
[Comparison, <, LessThan, "Checks if a value is less than another.", 80]
[Comparison, <=, LessThanOrEqual, "Checks if a value is less than or equal to another.", 80]
[Comparison, >, GreaterThan, "Checks if a value is greater than another.", 80]
[Comparison, >=, GreaterThanOrEqual, "Checks if a value is greater than or equal to another.", 80]
[Comparison, '=~ or like', RegexMatch, "Checks if a value matches a regular expression.", 80]
[Comparison, '!~ or not-like', NotRegexMatch, "Checks if a value does not match a regular expression.", 80]
[Comparison, in, In, "Checks if a value is in a list or string.", 80]
[Comparison, not-in, NotIn, "Checks if a value is not in a list or string.", 80]
[Comparison, starts-with, StartsWith, "Checks if a string starts with another.", 80]
[Comparison, ends-with, EndsWith, "Checks if a string ends with another.", 80]
[Comparison, not, UnaryNot, "Negates a value or expression.", 0]
[Math, +, Plus, "Adds two values.", 90]
[Math, ++, Concat, "Concatenate two lists, two strings, or two binary values.", 80]
[Math, -, Minus, "Subtracts two values.", 90]
[Math, *, Multiply, "Multiplies two values.", 95]
[Math, /, Divide, "Divides two values.", 95]
[Math, //, FloorDivision, "Divides two values and floors the result.", 95]
[Math, mod, Modulo, "Divides two values and returns the remainder.", 95]
[Math, **, "Pow ", "Raises one value to the power of another.", 100]
[Bitwise, bit-or, BitOr, "Performs a bitwise OR on two values.", 60]
[Bitwise, bit-xor, BitXor, "Performs a bitwise XOR on two values.", 70]
[Bitwise, bit-and, BitAnd, "Performs a bitwise AND on two values.", 75]
[Bitwise, bit-shl, ShiftLeft, "Shifts a value left by another.", 85]
[Bitwise, bit-shr, ShiftRight, "Shifts a value right by another.", 85]
[Boolean, and, And, "Checks if two values are true.", 50]
[Boolean, or, Or, "Checks if either value is true.", 40]
[Boolean, xor, Xor, "Checks if one value is true and the other is false.", 45]
]}

@IanManske
Copy link
Copy Markdown
Member Author

IanManske commented Nov 24, 2024

Oh yeah, nice catch. Do we still need that part of the std lib? help operators has the same output structure.

Edit: it looks like it's not necessary?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Nov 24, 2024

The point of help operators and the rest of the help commands in the stdlib is to test the efficacy of using stdlib for these "low-impact" type of commands. I think it should be updated.

@WindSoilder
Copy link
Copy Markdown
Contributor

WindSoilder commented Dec 5, 2024

TBH I'm a confused, most of changes involved renaming. I think it's a little hard to review, maybe it's better to separate renaming stuff in another pr, what do you think?

@IanManske
Copy link
Copy Markdown
Member Author

TBH I'm a confused, most of changes involved renaming. I think it's a little hard to review, maybe it's better to separate renaming stuff in another pr, what do you think?

You can review by commit if that helps, the renaming should be limited to one commit.

@132ikl
Copy link
Copy Markdown
Member

132ikl commented Jan 24, 2025

Finally got around to looking at this, looks great! I have a couple thoughts:

  • I'd like to take a closer look at some of the tricky operator edge cases, hopefully in the next couple days, but this isn't a blocker for merging. thanks for marking all of these with TODOs, makes it much easier to see where our edge cases are
  • You didn't change this, but I noticed when reading through this that we're using type_compatible from type_check.rs for operations, and not Type::is_subtype_of. This is out of scope for the PR, but maybe we should think about unifying these, or at least making the difference more clear? Type::is_subtype_of has a notion of "type A is a subtype of type B", I'm not sure if that covers every case that type_compatible is used for.
  • This is really minor, but I'm a little confused why you moved Boolean::And below Boolean::Xor? The rest of the things you moved look great, I'm just a bit puzzled on this one 😅

@IanManske
Copy link
Copy Markdown
Member Author

I'd like to take a closer look at some of the tricky operator edge cases, hopefully in the next couple days, but this isn't a blocker for merging. thanks for marking all of these with TODOs, makes it much easier to see where our edge cases are

Thanks! One thing that I didn't fix or note is that we are too liberal when encountering Type::Any. If an operator doesn't support the type of one of the operands, but the other operand has type Any, then we don't provide a type error. (This is due to the (Type::Any, _) match arms.)

You didn't change this, but I noticed when reading through this that we're using type_compatible from type_check.rs for operations, and not Type::is_subtype_of. This is out of scope for the PR, but maybe we should think about unifying these, or at least making the difference more clear? Type::is_subtype_of has a notion of "type A is a subtype of type B", I'm not sure if that covers every case that type_compatible is used for.

Oh yeah, that sounds great! Feel free to unify those.

This is really minor, but I'm a little confused why you moved Boolean::And below Boolean::Xor? The rest of the things you moved look great, I'm just a bit puzzled on this one 😅

Honestly can't remember 😆 I think I may have been trying to match the order in the Bits operators. Or, I was sorting the operators by precedence?

@IanManske IanManske merged commit 62e56d3 into nushell:main Feb 13, 2025
@github-actions github-actions bot added this to the v0.103.0 milestone Feb 13, 2025
@132ikl
Copy link
Copy Markdown
Member

132ikl commented Feb 13, 2025

thanks!

ysthakur pushed a commit that referenced this pull request Jul 4, 2025
<!--
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.
-->

Rel: #14429, #16079

Finishes up a TODO in the assignment type checking. 

- For regular assignment operations (only applies to `mut`), type
checking is now done using `type_compatible` (which is what `let` uses)
- This allows some mutable assignments to work which weren't allowed
before

Before:
```nushell
let x: glob = "" 
# => ok, no error
mut x: glob = ""; $x = ""
# => Error: nu::parser::operator_incompatible_types
# => 
# =>   × Types 'glob' and 'string' are not compatible for the '=' operator.
# =>    ╭─[entry #6:1:19]
# =>  1 │ mut x: glob = ""; $x = ""
# =>    ·                   ─┬ ┬ ─┬
# =>    ·                    │ │  ╰── string
# =>    ·                    │ ╰── does not operate between 'glob' and 'string'
# =>    ·                    ╰── glob
# =>    ╰────

let x: number = 1
# ok, no error
mut x: number = 1; $x = 2
# => Error: nu::parser::operator_incompatible_types
# => 
# =>   × Types 'number' and 'int' are not compatible for the '=' operator.
# =>    ╭─[source:1:20]
# =>  1 │ mut x: number = 1; $x = 2
# =>    ·                    ─┬ ┬ ┬
# =>    ·                     │ │ ╰── int
# =>    ·                     │ ╰── does not operate between 'number' and 'int'
# =>    ·                     ╰── number
# =>    ╰────
```

After:
```nushell
let x: glob = ""
# ok, no error (same as before)
mut x: glob = ""; $x = ""
# ok, no error

let x: number = 1
# ok, no error (same as before)
mut x: number = 1; $x = 2
# ok, no error
```

- Properly type check compound operations. First checks if the operation
(eg. `+` for `+=`) type checks successfully, and then checks if the
assignment type checks successfully (also using `type_compatible`)
- This fixes some issues where the "long version" of a compound
assignment operator would error, but the compound assignment operator
itself would not

Before:
```nushell
mut x = 1; $x = $x / 2
# => Error: nu::parser::operator_incompatible_types
# => 
# =>   × Types 'int' and 'float' are not compatible for the '=' operator.
# =>    ╭─[entry #15:1:12]
# =>  1 │ mut x = 1; $x = $x / 2
# =>    ·            ─┬ ┬ ───┬──
# =>    ·             │ │    ╰── float
# =>    ·             │ ╰── does not operate between 'int' and 'float'
# =>    ·             ╰── int
# =>    ╰────

mut x = 1; $x /= 2
# uh oh, no error...

mut x = (date now); $x = $x - 2019-05-10
# => Error: nu::parser::operator_incompatible_types
# => 
# =>   × Types 'datetime' and 'duration' are not compatible for the '=' operator.
# =>    ╭─[entry #1:1:21]
# =>  1 │ mut x = (date now); $x = $x - 2019-05-10
# =>    ·                     ─┬ ┬ ───────┬───────
# =>    ·                      │ │        ╰── duration
# =>    ·                      │ ╰── does not operate between 'datetime' and 'duration'
# =>    ·                      ╰── datetime
# =>    ╰────

mut x = (date now); $x -= 2019-05-10
# uh oh, no error... (the result of this is a duration, not a datetime)
```

After:
```nushell
mut x = 1; $x = $x / 2
# => Error: nu::parser::operator_incompatible_types
# => 
# =>   × Types 'int' and 'float' are not compatible for the '=' operator.
# =>    ╭─[entry #5:1:12]
# =>  1 │ mut x = 1; $x = $x / 2
# =>    ·            ─┬ ┬ ───┬──
# =>    ·             │ │    ╰── float
# =>    ·             │ ╰── does not operate between 'int' and 'float'
# =>    ·             ╰── int
# =>    ╰────

mut x = (date now); $x -= 2019-05-10
# => Error: nu::parser::operator_incompatible_types
# => 
# =>   × Types 'datetime' and 'datetime' are not compatible for the '-=' operator.
# =>    ╭─[entry #11:1:21]
# =>  1 │ mut x = (date now); $x -= 2019-05-10
# =>    ·                     ─┬ ─┬ ─────┬────
# =>    ·                      │  │      ╰── datetime
# =>    ·                      │  ╰── does not operate between 'datetime' and 'datetime'
# =>    ·                      ╰── datetime
# =>    ╰────
# =>   help: The result type of this operation is not compatible with the type of the variable.
```

This is technically a breaking change if you relied on the old behavior
(for example, there was a test that broke after this change because it
relied on `/=` improperly type checking)

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->
* Mutable assignment operations now use the same type checking rules as
normal assignments
* For example, `$x = 123` now uses the same type checking rules as `let
x = 123` or `mut x = 123`
* Compound assignment operations now type check using the same rules as
the operation they use
* Assignment errors will also now highlight the invalid assignment
operator in red


# 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
> ```
-->
Adds some tests for the examples given above

# 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.
-->
N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated:pr-errors Deprecated: use A:error-handling, A:error-unhelpful, or A:error-silent-fail instead 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.

'not' operator precedence displayed wrong

4 participants