Skip to content

raise ParseError if assign to a non-variable or non-mutable-variable #14405

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.

Sign up for GitHub

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

Jump to bottom
Merged
sholderbach merged 3 commits into from
Nov 29, 2024
Merged

raise ParseError if assign to a non-variable or non-mutable-variable #14405

sholderbach merged 3 commits into from
Nov 29, 2024

Conversation

@WindSoilder
Copy link
Contributor

@WindSoilder WindSoilder commented Nov 21, 2024

Description

While reviewing #14388, I think we can make some improvement on parser.

For the following code:

let a = 3
a = 10   # should be error
$a = 10 # another error

I think they can raise ParseError, so nushell doesn't need to move forward compiling IR block.

User-Facing Changes

let a = 3
a = 10

Will raise parse error instead of compile error.

Tests + Formatting

Added 1 test.

@fdncred
Copy link
Contributor

fdncred commented Nov 21, 2024

At one point we were trying to decide whether a or $a was appropriate and decided to leave them both. I think it's better to allow only $a like you've demonstrated above.

@WindSoilder
Copy link
Contributor Author

WindSoilder commented Nov 21, 2024 •
edited
Loading

The real issue here is not the usage of variable a through a or $a, but try to mutate a variable which is not mutable.

devyn
devyn requested changes Nov 23, 2024
View reviewed changes
Copy link
Contributor

@devyn devyn left a comment

Choose a reason for hiding this comment

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

Just one small change :) I think it's a good idea

crates/nu-protocol/src/errors/parse_error.rs
#[error("Assignment to an immutable variable.")]
#[diagnostic(
code(nu::parser::assignment_requires_mutable_variable),
help("declare the variable with `mut`, or shadow it again with `let`")
Copy link
Contributor

@devyn devyn Nov 23, 2024

Choose a reason for hiding this comment

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

Nice, I like the help

@WindSoilder WindSoilder requested a review from devyn November 26, 2024 02:22
@sholderbach
Copy link
Member

sholderbach commented Nov 29, 2024

Great error messages!

@sholderbach sholderbach merged commit 8178309 into nushell:main Nov 29, 2024
13 checks passed
@github-actions github-actions bot added this to the v0.101.0 milestone Nov 29, 2024
@fdncred fdncred added the deprecated:pr-errors Deprecated: use A:error-handling, A:error-unhelpful, or A:error-silent-fail instead label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sholderbach sholderbach sholderbach approved these changes

@devyn devyn Awaiting requested review from devyn

Assignees

No one assigned

Labels

deprecated:pr-errors Deprecated: use A:error-handling, A:error-unhelpful, or A:error-silent-fail instead

Projects

None yet

Milestone

v0.101.0

Development

Successfully merging this pull request may close these issues.

4 participants

@WindSoilder @fdncred @sholderbach @devyn