pkl icon indicating copy to clipboard operation
pkl copied to clipboard

Output converters should allow selection for properties by Annotation

Open HT154 opened this issue 1 year ago • 2 comments

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.

  • If availableBefore/availableAfter are const, then targetVersion must also be const, but this design specifically requires that amending modules be able to set a targetVersion.
  • The self-import also doesn't work here since it always gets the default value and not the one from the amending module.

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.

HT154 avatar Jul 12 '24 02:07 HT154

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.

bioball avatar Jul 22 '24 21:07 bioball

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

  • The validation issues outlined above. Clearly not the best fit in all cases since language binding use cases don't apply. I'm still looking for a tool for this, and maybe a special type of annotation that can validate/transform a value when it's manifested/forced could work.
  • Influencing property keys during rendering. For this to be generalized (i.e. not part of the renderer implementations) it seems like it would need API other than converters (or an extension thereof) since converters can only transform values.
  • Influencing the rendering of values with multiple possible representations without adding potentially repetitive fixed properties. Duration is a good example, where it would be nice to annotate a property with eg. @DurationInt { unit = "s" } to force rendering as an integer number of seconds.

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.

HT154 avatar Sep 16 '24 00:09 HT154