tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

Getting `record_insert: tried to extend but already exists` when using patterns with duplicate bound variables #1906

Open yannham opened 2 months ago

yannham commented 2 months ago

Describe the bug

When using a pattern with duplicate bound variables, a match expression will fail at runtime with a strange and non-localized error: record_insert: tried to extend a record with the field y, but it already exists. This error leaks implementation details.

To Reproduce

nickel> match { { x = {y}, z = {y}} => 1 } { x.y = 1, z.y = 2}
error: record_insert: tried to extend a record with the field y, but it already exists

Expected behavior One approach would be to forbid such patterns. For example, Rust forbid this and would error out with error[E0416]: identifieryis bound more than once in the same pattern. Same for OCaml: Error: Variable x is bound several times in this matching. This would technically be a breaking change, and this is why it was kept legal for destructuring (see https://github.com/tweag/nickel/pull/1324), but relying on this feature is very dubious.

Another solution would be to use update instead of insert when compiling patterns, so that we can handle several occurrences. But I don't have a good use-case for this, so I'm in favor of just making this illegal.

aspiwack commented 2 months ago

If you bind the same variable twice, you need some notion of equality. This is usually not very reasonable. Alternatively, you could merge the values that are bound to the different occurrences of y. This is a sane semantics, but probably a terribly confusing one.

It's probably quite a bit more reasonable to reject such patterns indeed.

yannham commented 2 months ago

If you bind the same variable twice, you need some notion of equality

The current implementation (in destructuring) is to do shadowing instead. Whether this is reasonable or not, that's another question...