Spread operator in record literals #11144
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ysthakur
commented
Nov 23, 2023
| @@ -30,7 +30,7 @@ pub enum Expr { | |||
| MatchBlock(Vec<(MatchPattern, Expression)>), | |||
| List(Vec<Expression>), | |||
| Table(Vec<Expression>, Vec<Vec<Expression>>), | |||
| Record(Vec<(Expression, Expression)>), | |||
| Record(Vec<RecordItem>), | |||
There was a problem hiding this comment.
It bugs me a little that there's an Expr::Spread variant, but it's not used for records, just lists. Would it be a good idea to make Expr::List also accept instances of some ListItem enum for consistency? It would mean refactoring a lot of code, though. Another possibility is keeping Vec<(Expression, Expression)> and representing spread expressions by pairing them with Nothing, but that's less safe.
ysthakur
commented
Nov 23, 2023
I'm a bit nervous about it (and other places) too, it's not the nicest. Another place that needs to be looked at more closely is the changes to
I'm not 😄. Like you mentioned, the changes to the parser are a bit complicated and it definitely needs to be tested more, but I can't think of more edge cases because I haven't really used records until now, so if you have any suggestions, that'd be very helpful. |
|
Currently, the |
|
sholderbach
added
the
pr:breaking-change
sholderbach
approved these changes
Nov 29, 2023
There was a problem hiding this comment.
I am not an expert in the FlatShape world, but things look sound to me.
The parsing and handling of Record invariants looks good to me. Good call adding the name to the error.
For the release note there is an additional breaking change to mention around how record literals now disallow duplicate column names instead of just ignoring them. I think for literals this is a fine choice to make.
Thanks for the solid amount of tests in the happy path. We may overtime want to expand that with illegal syntax in the spread position or inside the record, to harden ther parser.
| RecordItem::Spread(_, inner) => { | ||
| match eval_expression(engine_state, stack, inner)? { | ||
| Value::Record { val: inner_val, .. } => { | ||
| for (col_name, val) in inner_val { |
There was a problem hiding this comment.
There could be an argument to be had that one might want to have semantics where the spread overrides a local key or you could override a value from the spread by a following key.
But for now this is the clearly definable and easier to implement route even though it may reject some combinations of existing keys and incoming data at eval-time.
Comment on lines
+1620
to
+1622
| } else if second_token.is_some_and(|c| { | ||
| c.len() > 3 && c.starts_with(b"...") && (c[3] == b'$' || c[3] == b'{' || c[3] == b'(') | ||
| }) { |
There was a problem hiding this comment.
This is certainly one of the scarier bits in the existing parse logic.
(Local soundness seems correct, as the bounds are checked)
Comment on lines
+5206
to
+5213
| if curr_tok.starts_with(b"...") | ||
| && curr_tok.len() > 3 | ||
| && (curr_tok[3] == b'$' || curr_tok[3] == b'{' || curr_tok[3] == b'(') | ||
| { | ||
| // Parse spread operator | ||
| let inner = parse_value( | ||
| working_set, | ||
| Span::new(curr_span.start + 3, curr_span.end), |
There was a problem hiding this comment.
This part of the logic looks sane to me!
Comment on lines
-294
to
-296
| // avoid duplicate cols. | ||
| let col_name = value_as_string(eval_constant(working_set, col)?, expr.span)?; | ||
| record.insert(col_name, eval_constant(working_set, val)?); |
There was a problem hiding this comment.
Breaking change pin: This will turn duplication in const contexts into a hard error, which was not the case previously.
amtoine
approved these changes
Nov 29, 2023
There was a problem hiding this comment.
i'll follow @sholderbach and land this PR, let's try it in the real world 😏
thanks for working on this again @ysthakur 🙏
This was referenced Nov 30, 2023
WindSoilder
pushed a commit
that referenced
this pull request
Dec 6, 2023
<!-- 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. --> It turns out that I left a bug in [#11144](#11144), which introduced a spread operator in record literals. When highlighting subexpressions that are spread inside records, the spread operator and the token before it are insert twice. Currently, when you type `{ ...() }`, this is what you'll see:  With the PR, the behavior is as expected:  I'm still not sure how `FlatShape` works, I just copied the existing logic for flattening key-value pairs in records, so it's possible there's still issues, but I haven't found any yet (tried spreading subexpressions, variables, and records). # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> Highlighting for subexpressions spread inside records should no longer be screwed up. # 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 > ``` --> Is there any way to test flattening/syntax highlighting? # 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. -->
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
Goes towards implementing nushell#10598, which asks for a spread operator in lists, in records, and when calling commands (continuation of nushell#11006, which only implements it in lists) # Description This PR is for adding a spread operator that can be used when building records. Additional functionality can be added later. Changes: - Previously, the `Expr::Record` variant held `(Expression, Expression)` pairs. It now holds instances of an enum `RecordItem` (the name isn't amazing) that allows either a key-value mapping or a spread operator. - `...` will be treated as the spread operator when it appears before `$`, `{`, or `(` inside records (no whitespace allowed in between) (not implemented yet) - The error message for duplicate columns now includes the column name itself, because if two spread records are involved in such an error, you can't tell which field was duplicated from the spans alone `...` will still be treated as a normal string outside records, and even in records, it is not treated as a spread operator when not followed immediately by a `$`, `{`, or `(`. # User-Facing Changes Users will be able to use `...` when building records. ``` > let rec = { x: 1, ...{ a: 2 } } > $rec ╭───┬───╮ │ x │ 1 │ │ a │ 2 │ ╰───┴───╯ > { foo: bar, ...$rec, baz: blah } ╭─────┬──────╮ │ foo │ bar │ │ x │ 1 │ │ a │ 2 │ │ baz │ blah │ ╰─────┴──────╯ ``` If you want to update a field of a record, you'll have to use `merge` instead: ``` > { ...$rec, x: 5 } Error: nu::shell::column_defined_twice × Record field or table column used twice: x ╭─[entry nushell#2:1:1] 1 │ { ...$rec, x: 5 } · ──┬─ ┬ · │ ╰── field redefined here · ╰── field first defined here ╰──── > $rec | merge { x: 5 } ╭───┬───╮ │ x │ 5 │ │ a │ 2 │ ╰───┴───╯ ``` # Tests + Formatting # After Submitting
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
<!-- 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. --> It turns out that I left a bug in [nushell#11144](nushell#11144), which introduced a spread operator in record literals. When highlighting subexpressions that are spread inside records, the spread operator and the token before it are insert twice. Currently, when you type `{ ...() }`, this is what you'll see:  With the PR, the behavior is as expected:  I'm still not sure how `FlatShape` works, I just copied the existing logic for flattening key-value pairs in records, so it's possible there's still issues, but I haven't found any yet (tried spreading subexpressions, variables, and records). # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> Highlighting for subexpressions spread inside records should no longer be screwed up. # 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 > ``` --> Is there any way to test flattening/syntax highlighting? # 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. -->
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
Feb 20, 2024
Goes towards implementing nushell#10598, which asks for a spread operator in lists, in records, and when calling commands (continuation of nushell#11006, which only implements it in lists) # Description This PR is for adding a spread operator that can be used when building records. Additional functionality can be added later. Changes: - Previously, the `Expr::Record` variant held `(Expression, Expression)` pairs. It now holds instances of an enum `RecordItem` (the name isn't amazing) that allows either a key-value mapping or a spread operator. - `...` will be treated as the spread operator when it appears before `$`, `{`, or `(` inside records (no whitespace allowed in between) (not implemented yet) - The error message for duplicate columns now includes the column name itself, because if two spread records are involved in such an error, you can't tell which field was duplicated from the spans alone `...` will still be treated as a normal string outside records, and even in records, it is not treated as a spread operator when not followed immediately by a `$`, `{`, or `(`. # User-Facing Changes Users will be able to use `...` when building records. ``` > let rec = { x: 1, ...{ a: 2 } } > $rec ╭───┬───╮ │ x │ 1 │ │ a │ 2 │ ╰───┴───╯ > { foo: bar, ...$rec, baz: blah } ╭─────┬──────╮ │ foo │ bar │ │ x │ 1 │ │ a │ 2 │ │ baz │ blah │ ╰─────┴──────╯ ``` If you want to update a field of a record, you'll have to use `merge` instead: ``` > { ...$rec, x: 5 } Error: nu::shell::column_defined_twice × Record field or table column used twice: x ╭─[entry nushell#2:1:1] 1 │ { ...$rec, x: 5 } · ──┬─ ┬ · │ ╰── field redefined here · ╰── field first defined here ╰──── > $rec | merge { x: 5 } ╭───┬───╮ │ x │ 5 │ │ a │ 2 │ ╰───┴───╯ ``` # Tests + Formatting # After Submitting
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
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. --> It turns out that I left a bug in [nushell#11144](nushell#11144), which introduced a spread operator in record literals. When highlighting subexpressions that are spread inside records, the spread operator and the token before it are insert twice. Currently, when you type `{ ...() }`, this is what you'll see:  With the PR, the behavior is as expected:  I'm still not sure how `FlatShape` works, I just copied the existing logic for flattening key-value pairs in records, so it's possible there's still issues, but I haven't found any yet (tried spreading subexpressions, variables, and records). # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> Highlighting for subexpressions spread inside records should no longer be screwed up. # 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 > ``` --> Is there any way to test flattening/syntax highlighting? # 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goes towards implementing #10598, which asks for a spread operator in lists, in records, and when calling commands (continuation of #11006, which only implements it in lists)
Description
This PR is for adding a spread operator that can be used when building records. Additional functionality can be added later.
Changes:
Expr::Recordvariant held(Expression, Expression)pairs. It now holds instances of an enumRecordItem(the name isn't amazing) that allows either a key-value mapping or a spread operator....will be treated as the spread operator when it appears before$,{, or(inside records (no whitespace allowed in between) (not implemented yet)...will still be treated as a normal string outside records, and even in records, it is not treated as a spread operator when not followed immediately by a$,{, or(.User-Facing Changes
Users will be able to use
...when building records.If you want to update a field of a record, you'll have to use
mergeinstead:Tests + Formatting
After Submitting