Throw when an object key default would become a Dynamic property - #1811
Throw when an object key default would become a Dynamic property#1811adityasingh2400 wants to merge 1 commit into
default would become a Dynamic property#1811Conversation
`Dynamic` declares a hidden `default` property, so an object key named `default` cannot be represented as a `Dynamic` property. `json.Parser.parse()`, `yaml.Parser.parse()` and `yaml.Parser.parseAll()` silently dropped the key instead, and reading `.default` on the result then failed with a confusing type error about `Function1`. These methods now throw, and the message points at `useMapping = true`, which parses into a `Mapping` and preserves the key. SnakeYAML wraps any exception other than `YamlEngineException` thrown during construction, so the new `org.pkl.core.util.yaml.ParseException` is unwrapped from the cause rather than caught directly. Without that, the rendered hint would carry a Java class name. Fixes apple#561
|
Flagging a red I saw on my own fork's CI in case it shows up here once the workflows are approved to run. The That job is unrelated to this change, which is a parser-level check on object keys named |
|
No checks run on this PR here, so flagging the build result from my fork since it is the only CI signal. The full
|
1 participant
Fixes #561
Dynamicdeclares a hiddendefaultproperty, so an object key nameddefaultcannot be represented as aDynamicproperty.json.Parser.parse(),yaml.Parser.parse()andyaml.Parser.parseAll()silently dropped the key instead, and reading.defaulton the result then failed with a confusing type error aboutFunction1. These methods now throw, and the message points atuseMapping = true, which parses into aMappingand preserves the key.This is a redo of #700, which carried approvals from bioball, stackoverflow and holzensp before its own author closed it and deleted the head ref. I kept the error wording that was settled on that thread. One deliberate difference: SnakeYAML's
BaseConstructorrethrowsYamlEngineExceptionunchanged but wraps every otherRuntimeException, so the neworg.pkl.core.util.yaml.ParseExceptionis unwrapped from the cause rather than caught directly. Catching it directly is what made the rendered hint carry aorg.pkl.core.util.yaml.ParseException:prefix in the old thread.Verified by restoring the three changed sources to
mainand rerunning: four snippet tests fail, and both new error snippets reproduce the reported bug verbatim, with thedefaultkey simply absent from the output. With the change,:pkl-core:testis 1583 tests with 0 failures and spotless is clean. I grepped the repo for YAML and JSON fixtures containing adefaultkey and for.pklfiles calling either parser outsidepkl-core/src/testand found none, so no other module reaches the new error path. Native-image and the multi-JDK matrix were not run locally.One thing for maintainers to weigh: bioball flagged this as a breaking change on the original thread. I added a release note under 0.33 Breaking Changes, but if it needs to wait for a major version window that is entirely reasonable and I am happy to retarget it.