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

Fix panic on duplicated top-level idents in record destructuring #1324

Merged
merged 1 commit into from
May 26, 2023

Conversation

matthew-healy
Copy link
Contributor

@matthew-healy matthew-healy commented May 26, 2023

Previously Nickel would panic when encountering code like this:

let f = fun { x, x } = x in f { x = 1 }

This commit fixes that by checking each destructured record pattern for duplciated identifiers at parsing time, and returning an error if any are encountered.

Note, however, that in order to preserve backwards compatibility with Nickel 1.0, the following code is still valid (and returns 1):

let f = fun { x = { y }, z = { y } } => y
in f { x = { y = 1 }, z = {  y = 2 } }

Fixes #1318.

Previously Nickel would panic when encountering code like this:

```
let f = fun { x, x } = x in f { x = 1 }
```

This commit fixes that by checking each destructured record pattern for
duplciated identifiers at parsing time, and returning an error if any
are encountered.

Note, however, that in order to preserve backwards compatibility with
Nickel 1.0, the following code is still valid (and returns `1`):

```
let f = fun { x = { y }, z = { y } } => y
in f { x = { y = 1 }, z = {  y = 2 } }
```
@matthew-healy matthew-healy requested review from yannham, vkleen, Radvendii and jneem and removed request for yannham May 26, 2023 08:54
@matthew-healy matthew-healy marked this pull request as ready for review May 26, 2023 08:55
@github-actions github-actions bot temporarily deployed to pull request May 26, 2023 08:58 Inactive
Copy link
Contributor

@vkleen vkleen left a comment

Choose a reason for hiding this comment

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

The duplicate identifiers don't even need to be nested for the previously working example:

fun { foo = x, bar = x } => x

picks out the value of foo.

@matthew-healy
Copy link
Contributor Author

@vkleen Ah, good point! That will also still work fine after this change, though it'd be great to fix it when we next allow breaking changes.

@matthew-healy matthew-healy added this pull request to the merge queue May 26, 2023
@matthew-healy matthew-healy removed this pull request from the merge queue due to a manual request May 26, 2023
@matthew-healy matthew-healy added this pull request to the merge queue May 26, 2023
Merged via the queue into master with commit 9a2e2fc May 26, 2023
@matthew-healy matthew-healy deleted the fix/duplicated-destructuring-panic branch May 26, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when duplicating field in record destructuring
2 participants