apple / pkl

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

Output converters should allow selection for properties by Annotation #576

Open HT154 opened 4 months ago

HT154 commented 4 months ago

Currently, annotations are a helpful language feature for modules using pkl:reflect and tooling like IDEs, but they're not broadly useful in most modules. One way they could be made more useful is through supporting them as a type of match for ValueRenderer converters.

Background

Here's an example where current language features fail to fully meet a module's needs. Take a module that has been adapted to validate property availability in multiple versions of a service:

module MyServiceConfig

hidden targetVersion: UInt = 2

function availableBefore(version: UInt): Boolean = if (targetVersion < version) true
  else throw("available before \(version), currently targeting \(targetVersion)")

function availableAfter(version: UInt): Boolean = if (version >= targetVersion) true
  else throw("available after \(version), currently targeting \(targetVersion)")

myOldProperty: String(availableBefore(2))?

myNewProperty: String(availableAfter(2))?

The above code works just fine, but problems arise the moment this needs to be used outside the module's top level:

myNestedValue: MyNestedClass

class MyNestedClass {
  nestedOldProperty: String(availableBefore(2))?
}

This produces an error:

Cannot call method availableBefore from here because it is not const.  Classes, typealiases, and annotations can only reference const members of their enclosing module.
To fix, either make the accessed member const, or add a self-import of this module, and access this member off of the self import.

Neither of the proposed solutions work here.

One workaround today might be to define an output converter to handle this:

module MyServiceConfig

hidden targetVersion: UInt = 2

function availableBefore(version: UInt): Null = if (targetVersion < version) null
  else throw("available before \(version), currently targeting \(targetVersion)")

function availableAfter(version: UInt): Null = if (version >= targetVersion) null
  else throw("available after \(version), currently targeting \(targetVersion)")

myOldProperty: String?

myNewProperty: String?

myNestedValue: MyNestedClass

class MyNestedClass {
  nestedOldProperty: String?
}

output {
  renderer = new YamlRenderer {
    converters {
      ["myOldProperty"] = (it) -> availableBefore(2) ?? it
      ["myNewProperty"] = (it) -> availableAfter(2) ?? it
      ["myNestedValue.nestedOldProperty"] = (it) -> availableBefore(2) ?? it
    }
  }
}

The major downside here is that this separates the information about availability from the property definition and assumes that MyServiceConfig will always be the root module being evaluated. This is particularly fatal in cases like Kubernetes configurations where there are many fields in many modules, some of which may not be known to the parent module at time of writing.

Proposal

My proposed solution here is to allow converters to match on annotation classes. The converter function would need to be passed both the annotation value and the underlying property value, so it would either need to be allowed to be a Function2<«annotation value», «property value», Any> or be passed a Pair<«annotation value», «property value»> (or possibly some new class eg. ValueRendererConverterMatchAnnotation).

This would make annotations broadly useful in a variety of scenarios, including the versioning example above. Here's how that example might look if this were implemented:

module MyServiceConfig

hidden targetVersion: UInt = 2

class Available extends Annotation {
  before: UInt?

  after: UInt?

  function evaluate(target: UInt, value: Any): Any =
    if (before != null && target >= before) throw("available before \(before), currently targeting \(target)")
    else if (after != null && after < target) throw("available after \(after), currently targeting \(target)")
    else value
}

@Available { before = 2 }
myOldProperty: String?

@Available { after = 2 }
myNewProperty: String?

myNestedValue: MyNestedClass

class MyNestedClass {
  @Available { before = 2 }
  nestedOldProperty: String?
}

output {
  renderer = new YamlRenderer {
    converters {
      [Available] = (it: Pair<Available, Any>) -> it.key.evaluate(targetVersion, it.value)
      // OR
      [Available] = (available: Available, value: Any) -> available.evaluate(targetVersion, value)
    }
  }
}

Modules embedding this one would need to copy its converters to inherit this behavior, but that's very doable since this is portable and not dependent on property key paths like the workaround above.

bioball commented 4 months ago

Generally, this feature makes sense to me. However, I don't think that this contributes to the validation story--output converters generally are not the right place to apply validation because they don't apply to the library use-case (e.g. evaluating Pkl into Java/Go/Swift structs).

It would, however, be useful for transformation, because it helps untether a dependency between a template and a renderer. For example, something like:

@JsonPropertyName { value = "my-foo" }
myFoo: String

I'd encourage that you write a SPICE here, and perhaps even pair it with an implementation and it can be discussed in detail there.

HT154 commented 2 months ago

I'm back to thinking about this some more. There seems to be a few problems this has potential to solve:

This last use case currently seems like the most compelling motivation for this feature as-proposed, though having better control over property keys during rendering is definitely valuable separately.