fix: preserve outer self/$ for super[name] lookup (#829)#835
Closed
He-Pin wants to merge 2 commits intodatabricks:masterfrom
Closed
fix: preserve outer self/$ for super[name] lookup (#829)#835He-Pin wants to merge 2 commits intodatabricks:masterfrom
He-Pin wants to merge 2 commits intodatabricks:masterfrom
Conversation
Motivation:
visitLookupSuper was calling sup.value(key, pos) without passing self,
so the field retrieved through super[name] had its self/$ bound to the
parent object instead of the merged outer self. Fields defined in mixins
applied AFTER the parent therefore became unreachable from inside fields
retrieved via super[name] + override, breaking grafana/mimir's
overrideSuperIfExists helper:
sjsonnet.Error: Field does not exist: ingest_storage_ingester_autoscaling_enabled
at [overrideSuperIfExists].(mimir/rollout-operator.libsonnet:18:64)
at [<root>].(mimir/memberlist.libsonnet:106:50)
Modification:
Pass scope.bindings(e.selfIdx) as the self argument, mirroring the
existing semantics of visitSelectSuper (super.name). Also use it as
the fallback when sup is null.
Result:
super[name] now agrees with super.name; fields retrieved through
computed-key super lookup see $ bound to the outermost merge result,
matching google/jsonnet, go-jsonnet, and jrsonnet. Adds two regression
tests under new_test_suite/: a focused parity/edge-case suite and a
multi-file end-to-end repro of the mimir failure.
References:
- databricks#829
…#829) Motivation: The fix in the previous commit is one line and easy to "clean up" by removing the seemingly redundant `self` argument, which would silently re-introduce issue databricks#829. Modification: Add a DO NOT CHANGE / WHY block in front of visitLookupSuper explaining why `self` must be passed explicitly to `sup.value(...)` and pointing at the cross-implementation contract with super.name. Result: Future readers (humans and AI alike) have an in-source warning explaining the invariant before they touch this method.
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
1 participant
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.
Motivation:
Issue #829 shows that sjsonnet fails to evaluate Grafana Mimir manifests because
super[name]bindsself/$incorrectly during computed-key super lookups. That makes fields defined by later-applied mixins unreachable from inside the field returned bysuper[name], while other Jsonnet implementations evaluate the same manifests successfully.Modification:
selfthroughvisitLookupSuper, matching the existingsuper.namebehavior instead of rebinding to the parent object.selfargument must not be removed fromsup.value(...).Result:
sjsonnetnow evaluates the Mimir-styleoverrideSuperIfExists(name, override)pattern correctly, andsuper[name]matches the semantics ofsuper.nameacross the added regressions. The branch also leaves a guardrail comment in the evaluator so this subtle binding invariant is less likely to regress in future refactors.References: