Conversation
edsilfer
requested review from
apattidb,
iuliand-db,
lihaoyi,
samk-db and
szeiger
edsilfer
force-pushed
the
edgar/fix-duplicate-field-on-list-comprehension
branch
from
230693f to
5259355
Compare
lihaoyi-databricks
approved these changes
Nov 9, 2020
Contributor
lihaoyi-databricks
left a comment
lihaoyi-databricks
left a comment
There was a problem hiding this comment.
One small nit, but otherwise looks good
edsilfer
force-pushed
the
edgar/fix-duplicate-field-on-list-comprehension
branch
from
5259355 to
a893c6f
Compare
stephenamar-db
deleted the
edgar/fix-duplicate-field-on-list-comprehension
branch
stephenamar-db
pushed a commit
that referenced
this pull request
Aug 21, 2025
…#470) This PR fixes a bug in the detection of duplicate fields: Official `jsonnet` and `go-jsonnet` both detect and fail on duplicate field names even when the field names are dynamically computed via [field name expression](https://jsonnet.org/ref/language.html#field-name-expressions): ```jsonnet { k: 1, ["k"]: 2 } ``` produces ``` Error: RUNTIME ERROR: Duplicate field name: "k" example1.jsonnet:1:1-19 $ During evaluation ``` However, sjsonnet currently accepts this and outputs ``` {"k" : 2} ``` with a last-assignment-wins policy. sjsonnet already correctly detects _literal_ field name duplication [at parse time](https://github.com/databricks/sjsonnet/blob/0.5.4/sjsonnet/src/sjsonnet/Parser.scala#L401-L413) (e.g. `{k: 1, k: 2}`) and detects field name duplication in object comprehensions at both [parse](https://github.com/databricks/sjsonnet/blob/0.5.4/sjsonnet/src/sjsonnet/Parser.scala#L472-L497) and [evaluation](https://github.com/databricks/sjsonnet/blob/0.5.4/sjsonnet/src/sjsonnet/Evaluator.scala#L771-L788) time (see #100 and #156). This PR adds duplicate field name detection at normal object evaluation time. The core check is simple and cheap: when adding a member to the object's fields hashmap, check the `put()` return value and fail if we detect that a duplicate key has been inserted. One subtlety to handle involves sjsonnet's static object optimizations: it's possible that we might detect duplicates during static optimization because a dynamic field expression might be constant-folded / statically optimized into a fixed literal. Even though we can detect this at optimization time, we must defer the throwing of an error until the object is actually evaluated or accessed during program execution: this is necessary in order to faithfully preserve lazy evaluation semantics. I chose to handle this by rendering such objects ineligible for static optimization: they will remain as `MemberList`s. I added new regression tests in EvaluatorSuite to cover this bug and confirmed that the expected behavior matches the jsonnet and go-jsonnet implementations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
This PR fixes issue 99 by issuing an error in case an
Expr.Stris matching with anExpr.ForSpecwith a value greater than 1. This condition denotes that theExpr.Strfield value would be overridden in each interaction of theExpr.ForSpecHow is this tested?
./mill "sjsonnet[2.13.3].jvm.test.testLocal"to make sure that all the tests are still passing