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

Entry of which key is same with an element index #361

Open
taichi-ishitani opened this issue Mar 25, 2024 · 6 comments
Open

Entry of which key is same with an element index #361

taichi-ishitani opened this issue Mar 25, 2024 · 6 comments

Comments

@taichi-ishitani
Copy link
Contributor

Hi,

As shown example below, entries of which key matches with an element index will be deleted.

foo { [0] = 2; 3 }
bar = foo[0]
$ ./pkl eval foo.pkl
foo {
  3
}
bar = 3

On the other hand, entries of which key does not match with an element index will not be deleted.

foo { [1] = 2; 3 }
bar = foo[0]
$ ./pkl eval foo.pkl
foo {
  [1] = 2
  3
}
bar = 3

Which behavior is wrong?

@bioball
Copy link
Contributor

bioball commented Mar 25, 2024

Currently, there is a bug when mixing entries whose keys are integers with elements. So, the first snippet is incorrect, and is a bug.

This happens because Pkl mistakenly interprets [0] = 2 as "assign to element with index 0", instead of "declare an entry with key 0".

@taichi-ishitani
Copy link
Contributor Author

Thank you and I understood.
For the first example, what is the expected value of foo[0]? Should it return the element with index 0?

@odenix
Copy link
Contributor

odenix commented Mar 26, 2024

Currently, there is a bug when mixing entries whose keys are integers with elements. So, the first snippet is incorrect, and is a bug.

What does [n] = x mean for Dynamic? Does it mean the same as for Listing, the same as for Mapping, or something else?

@bioball
Copy link
Contributor

bioball commented Mar 27, 2024

Currently, the behavior is: if there is an element at index n, [n] = x means: overwrite element at that index. And foo[0] means either "get element with index 0`, or "get entry with key 0", depending on which exists. In our current model, it is not possible to for entries with number keys to coexist with elements at the same number index.

With our current behavior, I think this should throw with a duplicate member definition error. It doesn't make sense for this to be accepted as one element declaration:

foo { [0] = 2; 3 }

I think there's some room for improvement here. This is surprising:

bar {
  1
}

foo = (bar) {
  [0] = 0
  [1] = 1
  [2] = 2
}

This produces:

bar {
  1
}
foo {
  0
  [1] = 1
  [2] = 2
}

Possibly, we can change the grammar for "assign to element index N", which might help here. For example, here's some made up syntax:

foo {
  amend [0] = 0
}

@taichi-ishitani
Copy link
Contributor Author

taichi-ishitani commented Mar 28, 2024

I think this should throw with a duplicate member definition error.

Is it expected behavior that Pkl raise an error if an entry and an element have the same idnex?
I'm developing a Pkl parser written in Ruby so I post this issue.

@odenix
Copy link
Contributor

odenix commented Nov 5, 2024

Pkl's behavior for dynamic objects that contain both elements and integer-keyed entries is confusing. I think it cannot even be reasoned about because a key with index x and entry with key x will cause a collision in VmObject.cachedValues.

I can think of several solutions, but one stands out for being simple to understand and implement:

  • A dynamic object cannot have both elements and entries.

This would also solve the following oddities:

  • Dynamic can have both elements and entries, but can't have separate defaults for them
  • Listing and Mapping have a length property, but Dynamic doesn't

Are there legitimate use cases for supporting dynamic objects that contain both elements and entries?

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

No branches or pull requests

3 participants