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.

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

Creating a Dynamic with property default set produces a broken object #561

Open
HT154 opened this issue Jun 29, 2024 · 3 comments · May be fixed by #700
Open

Creating a Dynamic with property default set produces a broken object #561

HT154 opened this issue Jun 29, 2024 · 3 comments · May be fixed by #700
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@HT154
Copy link
Contributor

HT154 commented Jun 29, 2024 •
edited
Loading

The behavior as-is is very surprising! Check this out:

import "pkl:json"

hidden jsonString = """
  {
    "hello": "world",
    "default": "greeting"
  }
  """

asDynamic = new json.Parser {}.parse(jsonString)
asMapping = new json.Parser { useMapping = true }.parse(jsonString)

Result:

asDynamic {
  hello = "world"
}
asMapping {
  ["hello"] = "world"
  ["default"] = "greeting"
}

I think throwing an error (or at least some kind of warning) here instead of returning unexpected results is desirable. Something like "encountered object key default when parsing JSON, resulting Dynamic will be incomplete or invalid" might make sense.

N.B.: Attempting to access the default yields further surprises:

asDynamicDefault = asDynamic.default

Result:

❯ pkl eval test.pkl
–– Pkl Error ––
Expected value of type `Function1`, but got type `String`.
Value: "greeting"

1741 | hidden default: (unknown) -> Any = (_) -> new Dynamic {}
                       ^^^^^^^^^^^^^^^^
at pkl.base#Dynamic.default (https://github.com/apple/pkl/blob/0.26.0/stdlib/base.pkl#L1741)

1 | 
    ^
at generated (source:unavailable)

11 | asDynamicDefault = asDynamic.default
                        ^^^^^^^^^^^^^^^^^
at test#asDynamicDefault (file:///.../test.pkl, line 11)

106 | text = renderer.renderDocument(value)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at pkl.base#Module.output.text (https://github.com/apple/pkl/blob/0.26.0/stdlib/base.pkl#L106)

This similarly affects yaml.Parser.parse() and Map.toDynamic().
@HT154 HT154 changed the title pkl:json.Parser should throw an error when useMapping = false and key "default" is encountered json.Parser should throw an error when useMapping = false and key "default" is encountered Jun 30, 2024
@holzensp holzensp added bug Something isn't working good first issue Good for newcomers labels Jul 1, 2024
@bioball
Copy link
Contributor

bioball commented Jul 3, 2024

Fair issue; seems like a good idea to throw here instead.

@lemonnn-8
Copy link

Hey, I can take this up.

HT154 added a commit to HT154/pkl that referenced this issue Oct 16, 2024
@HT154
Error when JSON/YAML parsing would set property default in a Dynamic
8a2b659 
Resolves apple#561
@HT154 HT154 linked a pull request Oct 16, 2024 that will close this issue
Open
HT154 added a commit to HT154/pkl that referenced this issue Oct 16, 2024
@HT154
Error when JSON/YAML parsing would set property default in a Dynamic
109101e 
Resolves apple#561
HT154 added a commit to HT154/pkl that referenced this issue Oct 17, 2024
@HT154
Error when JSON/YAML parsing would set property default in a Dynamic
ea57d3b 
Resolves apple#561
HT154 added a commit to HT154/pkl that referenced this issue Oct 24, 2024
@HT154
Error when JSON/YAML parsing would set property default in a Dynamic
e8ee462 
Resolves apple#561
HT154 added a commit to HT154/pkl that referenced this issue Oct 24, 2024
@HT154
Error when JSON/YAML parsing would set property default in a Dynamic
db1c43a 
Resolves apple#561
HT154 added a commit to HT154/pkl that referenced this issue Oct 26, 2024
@HT154
Error when JSON/YAML parsing would set property default in a Dynamic
1478941 
Resolves apple#561
HT154 added a commit to HT154/pkl that referenced this issue Oct 29, 2024
@HT154
Error when JSON/YAML parsing would set property default in a Dynamic
2d25e42 
Resolves apple#561
@HT154
Copy link
Contributor Author

HT154 commented Feb 7, 2025

This actually doesn't just affect JSON/YAML parsing. It also affects Map.toDynamic():

❯ pkl eval pkl:base -x 'Map("default",  1, "other", 2).toMap().toDynamic().default'       
–– Pkl Error ––
Expected value of type `Function1`, but got type `Int`.
Value: 1

@HT154 HT154 changed the title json.Parser should throw an error when useMapping = false and key "default" is encountered Creating a Dynamic with property default set produces a broken object Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Error when JSON/YAML parsing would set property default in a Dynamic HT154/pkl
4 participants
@HT154 @holzensp @bioball @lemonnn-8