apple / pkl

A configuration as code language with rich validation and tooling.
https://pkl-lang.org
Apache License 2.0
10.21k stars 275 forks source link

Pain Point: Deeply Nested Amends (No Flat Member Syntax) #423

Open dwbrite opened 6 months ago

dwbrite commented 6 months ago

I'm testing Pkl as a replacement for k8s tools like Kustomize/Helm. So far it's quite nice, but one pain point is modifying deeply nested properties.

As an example, say I have a HomeAssistant pkl package with a default volume claim of 1Gi, which I'd like to bump up to 2Gi.

import "ha-app.pkl" as homeassistant // extends AppEnvCluster, sets isLeafModule() = true
// assume the above is a third-party package

// default HomeAssistant application, with one modified property
local ha_app = (homeassistant) {
  statefulSets {
    ["core"] {
      spec {
        volumeClaimTemplates {
          [[metadata.name == "homeassistant-config"]] {
            spec {
              resources {
                requests {
                  ["storage"] = "2Gi"
                } // 9 lines of brackets :')
              }
            }
          }
        }
      }
    }
  }
}

output {
  files { // for use by argocd
    ["homeassistant.yaml"] = ha_app.output
  }
}

18 LoC for a single deeply nested amendment seems a little excessive. One way I've found to improve this is to remove some newlines, but I don't find this ideal.

// I guess this is better?
local ha_app = (homeassistant) { statefulSets { ["core"] { spec { volumeClaimTemplates {
  [[metadata.name == "homeassistant-config"]] { spec { resources { requests {
    ["storage"] = "2Gi"
  }}}}
}}}}}

I could create late-binding variables in a new pkl module which extends ha-app further, but that seems like I'd just be moving the problem down the line.


Personally I'd like to see dot syntax supported for amendments. In my case, that might look something like:

local ha_app = (homeassistant) {
  statefulSets["core"].spec.volumeClaimTemplates {
    [[metadata.name == "homeassistant-config"]] {
      spec.resources.requests["storage"] = "2Gi"
    }
  }
}

Am I missing a more idiomatic solution?

bioball commented 6 months ago

We'd like to add support for flat member syntax (the solution that you suggested) at some point. I definitely agree that this a pain point.

dwbrite commented 6 months ago

That's great to hear, and thanks for the quick response! Feel free to change the title to something more suitable for tracking flat member syntax.

Fwiw, Pkl has been fantastic for me, and I often find myself telling people how amazing it is :) I'm hoping I can bring it into my day job soon 🤞

translatenix commented 6 months ago

foo.bar.baz.qux {...} looks very similar to expressions foo.bar.baz.qux and (foo.bar.baz.qux) {...}, which complicates parsing and reading. I think a different syntax should at least be considered.

holzensp commented 6 months ago

Actually, the (...) in object amends syntax disambiguates that entirely. A slightly trickier thing about flat member syntax is; what do you do with consecutive amends;

foo.bar.baz = 0
foo.bar.qux.quux = "hello"
foo.bar.qux.corge = "world"

Is that the same as

foo {
  bar {
    baz = 0
    qux {
      quux = "hello"
      corge = "world"
    }
  }
}

or

foo {
  bar {
    baz = 0
  }
} {
  bar {
    qux {
      quux = "hello"
    }
  }
} {
  bar {
    qux {
      corge = "world"
    }
  }
}

That has implications for what super means (and for performance, but that's a shortcoming of the current implementation - albeit non-trivial). There are reasons why you're currently not allowed to amend the same property twice. Flat member syntax makes that ambiguous, because the overlap may be partial.

Or... should that ambiguity should syntactically be disallowed and should this require that properties are aggregated in notation, i.e.

foo.bar {
  baz = 0
  qux {
    quux = "hello"
    corge = "world"
  }
}
bioball commented 6 months ago

In both of those cases, super.foo should have the same meaning, no? It means the parent foo value in the prototype chain. super by itself isn't a valid expression.

An interesting problem here is what outer should mean. It would be surprising if these two were the same:

foo.bar.baz = outer.qux
foo {
  bar {
    baz = outer.qux
  }
}

Another one is this:

bar = 15

foo.bar.baz = bar

It would be surprising if it expands to the below snippet, because in the below snippet, the bar within baz = bar references the middle bar, rather than the module-level bar.

bar = 15

foo {
  bar {
    baz = bar
  }
}
translatenix commented 6 months ago

Actually, the (...) in object amends syntax disambiguates that entirely.

Distinguishing foo.bar.baz.qux {…} from foo.bar.baz.qux requires arbitrary lookahead. It’s also difficult to parse for a human, especially in the middle of a large program. The similarity with (foo.bar.baz.qux) {…} further complicates human parsing.

bioball commented 6 months ago

Distinguishing foo.bar.baz.qux {…} from foo.bar.baz.qux requires arbitrary lookahead. It’s also difficult to parse for a human, especially in the middle of a large program. The similarity with (foo.bar.baz.qux) {…} further complicates human parsing.

I see your point, but I'm not very convinced that this is a problem. This is unambiguous without needing lookahead:

foo.bar.baz.qux { ... }

This does require lookahead (because bar.baz.qux is a valid expression, and expressions are valid object members):

foo {
  bar.baz.qux { ... }
}

But:

  1. Generally I wouldn't expect to see code that looks like this. I would more expect them to be flattened at the top level, as per the first snippet in my post.
  2. Even in lookahead cases, it seems easy to distinguish.

Before shipping this feature, we'd definitely need to play around with this syntax in real world code.

translatenix commented 6 months ago

The lookahead here seems easy to me.

Arbitrary lookahead can cause problems with syntax highlighters and parser tools. It can also cause performance issues. It’s best avoided if possible. https://github.com/elves/elvish/issues/664

This is unambiguous without needing lookahead:

I think that’s only the case because typed objects can’t have elements and entries. Is this limitation here to stay?

I would more expect them to be flattened at the top level, as per the first snippet in my post.

I think it’s best to avoid such assumptions when designing a feature. If the feature can be used inside objects, it will be used inside objects.

bioball commented 6 months ago

I think that’s only the case because typed objects can’t have elements and entries. Is this limitation here to stay?

No, it's because expressions are not permitted at the module level. And, this isn't likely to change.

translatenix commented 6 months ago

No, it's because expressions are not permitted at the module level.

But if typed objects could have elements, it would make a lot of sense to allow expressions (elements) at the module level. This would also work great for rendering formats that support top-level arrays (JSON, etc.). Same for entries.

translatenix commented 6 months ago

Or... should that ambiguity should syntactically be disallowed and should this require that properties are aggregated in notation,

This might be a good first (and possibly last) step. Good editor support could ease the strictness pain. Definitely worth prototyping.

It would be surprising if it expands to the below snippet, because in the below snippet, the bar within baz = bar references the middle bar, rather than the module-level bar.

Sounds like a simple expansion won't be good enough.

bioball commented 6 months ago

But if typed objects could have elements, it would make a lot of sense to allow expressions (elements) at the module level. This would also work great for rendering formats that support top-level arrays (JSON, etc.). Same for entries.

Even if typed objects eventually allow entries and elements, I'm kind of bearish on this at the module level.

This starts to feel too much like a scripting language:

module foo

something()

somethingElse()

The way to render top-level arrays is to assign to output.value, e.g.

someListing: Listing<String> = new { "hello" }

output {
  value = someListing
  renderer = new JsonRenderer {}
}

Sounds like a simple expansion won't be good enough.

Yeah; we need to explore what the scoping rules should be. And, it might require that we start resolving variables at parse-time (within AstBuilder.java).