Skip to content

fix: bind self/$ to outer self in super[name] lookup (#829)#832

Merged
stephenamar-db merged 2 commits intodatabricks:masterfrom
He-Pin:fix/issue-829-super-lookup
May 10, 2026
Merged

fix: bind self/$ to outer self in super[name] lookup (#829)#832
stephenamar-db merged 2 commits intodatabricks:masterfrom
He-Pin:fix/issue-829-super-lookup

Conversation

@He-Pin
Copy link
Copy Markdown
Contributor

@He-Pin He-Pin commented May 9, 2026

Motivation

Issue #829: sjsonnet failed to evaluate grafana/mimir manifests with

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)

while google/jsonnet (C++), go-jsonnet, and jrsonnet all evaluate it correctly.

Root cause: visitLookupSuper (computed-key super[name]) called sup.value(key, pos) without passing the actual self, so Val.Obj.value defaulted self to sup (the parent in the merge chain). Fields retrieved through super[name] therefore had their $/self bound to the parent rather than the merged outer self, making _config keys defined in mixins applied after the parent unreachable. The static-key path visitSelectSuper (super.name) was already correct — it passed scope.bindings(e.selfIdx) as self. The two paths must agree.

Modification

sjsonnet/src/sjsonnet/Evaluator.scala — make visitLookupSuper mirror visitSelectSuper by passing the current self explicitly:

   def visitLookupSuper(e: LookupSuper)(implicit scope: ValScope): Val = {
     var sup = scope.bindings(e.selfIdx + 1).asInstanceOf[Val.Obj]
     val key = visitExpr(e.index).cast[Val.Str]
-    if (sup == null) sup = scope.bindings(e.selfIdx).asInstanceOf[Val.Obj]
-    sup.value(key.str, e.pos)
+    val self = scope.bindings(e.selfIdx).asInstanceOf[Val.Obj]
+    if (sup == null) sup = self
+    sup.value(key.str, e.pos, self)
   }

Two regression tests under sjsonnet/test/resources/new_test_suite/ (run on JVM, Scala.js and Scala Native via FileTests):

  1. super_lookup_dollar_binding.jsonnet — focused directional suite covering: parity between super.name and super[name] for $ binding; the overrideSuperIfExists pattern verbatim; bare super[name] (no override merge); 3-level chained mixins; computed key from a variable; object-extension syntax obj { f: super[name] }.
  2. super_lookup_mimir_pattern.jsonnet — multi-file end-to-end reproduction of the issue, with three .libsonnet files mirroring rollout-operator / memberlist / ingest-storage-ingester-autoscaling. Without the fix, this throws the exact error from the issue.

Result

super[name] now agrees with super.name; fields retrieved via computed-key super lookup see $ bound to the outermost merge result, matching google/jsonnet, go-jsonnet, and jrsonnet. Verified by reverting the one-line fix locally — both regression tests fail with the original "Field does not exist" message and the entry-mimir.jsonnet evaluation in the issue is unblocked.

Test plan:

  • ./mill 'sjsonnet.jvm[3.3.7]'.test — 5/5 passing
  • ./mill 'sjsonnet.jvm[2.13.18]'.test — 4/4 passing
  • ./mill 'sjsonnet.jvm[2.12.21]'.test — 4/4 passing
  • ./mill 'sjsonnet.js[3.3.7]'.test — 407/407 passing
  • ./mill 'sjsonnet.native[3.3.7]'.test — 428/428 passing
  • ./mill __.checkFormat — clean
  • Cross-checked output against jsonnet (C++ v0.22.0) on minimal repros and both new test files

References

He-Pin added 2 commits May 10, 2026 02:25
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.
@stephenamar-db stephenamar-db merged commit 0ae7b78 into databricks:master May 10, 2026
5 checks passed
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

Successfully merging this pull request may close these issues.

Scala fails to evaluate mimir manifests

2 participants