StackOverflowError with recursive validators #16

New Issue

adam · 2025-12-30T01:19:34+01:00

adam commented

2025-12-30 01:19:34 +01:00

Originally created by @madisp on GitHub (Feb 4, 2024).

Having two validators recurse into eachother blows up with a StackOverflowError. I don't really expect a fix here, but a better error message (e.g. what was currently being evaluated) would be nice.

PKL code:

class Link {
  operationRef: String?((this == null).xor(operationId == null))
  operationId: String?((this == null).xor(operationRef == null))
  parameters: Mapping<String, Any>
  requestBody: unknown?
  description: String?
  server: Server?
}

(I know I can omit the validator from operationId as the first one already covers it, initially I thought to add it to both just for some symmetry.)

Full stacktrace here: https://gist.github.com/madisp/f2d8f6edb50b9fccee15e17d87fa35a3

Originally created by @madisp on GitHub (Feb 4, 2024). Having two validators recurse into eachother blows up with a `StackOverflowError`. I don't really expect a fix here, but a better error message (e.g. what was currently being evaluated) would be nice. PKL code: ``` class Link { operationRef: String?((this == null).xor(operationId == null)) operationId: String?((this == null).xor(operationRef == null)) parameters: Mapping<String, Any> requestBody: unknown? description: String? server: Server? } ``` (I know I can omit the validator from `operationId` as the first one already covers it, initially I thought to add it to both just for some symmetry.) --- Full stacktrace here: https://gist.github.com/madisp/f2d8f6edb50b9fccee15e17d87fa35a3

adam closed this issue

2025-12-30 01:19:34 +01:00

adam commented

2025-12-30 01:19:35 +01:00

@jstewmon commented on GitHub (Feb 4, 2024):

In case others come across this issue looking for other workarounds (OP presents perfectly good workaround for its scenario), here are some things I tried...

Use a local class and a typealias to enrich the local class with constraints:

local class InternalLink {
  operationRef: String?
  operationId: String?
}

typealias Link = InternalLink((operationId is Null).xor(operationRef is Null))

Hidden properties can be used to define the checks on the class:

local class InternalLink {
  operationRef: String?
  operationId: String?
  hidden oneOfRefId: Boolean = (operationRef is Null).xor(operationId is Null)
}

typealias Link = InternalLink(oneOfRefId)

Similarly, hidden an anonymous function can be used to provide a more robust error message:

local class InternalLink {
  operationRef: String?
  operationId: String?
  hidden isValid = (l: InternalLink) ->
      if (!(l.operationRef is Null).xor(l.operationId is Null))
        throw("Exactly one of operationRef or operationId must be provided")
      else
        true
}
typealias Link = InternalLink(oneOfRefId)

The downside to all of these approaches is that the constraints are implied by the class.

I didn't see it mentioned in the docs, but it seems we can amend the class default function and leverage when to add constraints like this.

class Link {
  default {
    when (!(operationRef is Null).xor(operationId is Null)) {
      throw("Exactly one of operationRef or operationId must be provided")
    }
  }
  operationRef: String?
  operationId: String?
}

However, the error output does not include a reference to the item that is responsible for the error. 😕

Supposing it isn't currently possible to provide type constraints on a class definition, I think that would be a nice feature that provides relief for situations where one might need circular references among class properties.

@jstewmon commented on GitHub (Feb 4, 2024): In case others come across this issue looking for other workarounds (OP presents perfectly good workaround for its scenario), here are some things I tried... ---- Use a local class and a typealias to enrich the local class with constraints: ``` local class InternalLink { operationRef: String? operationId: String? } typealias Link = InternalLink((operationId is Null).xor(operationRef is Null)) ``` Hidden properties can be used to define the checks on the class: ``` local class InternalLink { operationRef: String? operationId: String? hidden oneOfRefId: Boolean = (operationRef is Null).xor(operationId is Null) } typealias Link = InternalLink(oneOfRefId) ``` Similarly, hidden an anonymous function can be used to provide a more robust error message: ``` local class InternalLink { operationRef: String? operationId: String? hidden isValid = (l: InternalLink) -> if (!(l.operationRef is Null).xor(l.operationId is Null)) throw("Exactly one of operationRef or operationId must be provided") else true } typealias Link = InternalLink(oneOfRefId) ``` The downside to all of these approaches is that the constraints are implied by the class. ---- I didn't see it mentioned in the docs, but it seems we can amend the class default function and leverage `when` to add constraints like this. ``` class Link { default { when (!(operationRef is Null).xor(operationId is Null)) { throw("Exactly one of operationRef or operationId must be provided") } } operationRef: String? operationId: String? } ``` However, the error output does not include a reference to the item that is responsible for the error. 😕 ---- Supposing it isn't currently possible to provide type constraints on a class definition, I think that would be a nice feature that provides relief for situations where one might need circular references among class properties.

adam commented

2025-12-30 01:19:35 +01:00

@holzensp commented on GitHub (Feb 5, 2024):

The problem here is that xor eagerly evaluates its arguments. More generally, though, this circular dependency is fundamentally an unbounded-depth recursion (i.e. it stack overflows). I tend to write this as

operationRef: String?((this == null) == (operationId != null))

but adding the inverse to operationId also still gives you a stack overflow when both are non-null.

This special-case of two properties referring directly to each other might be catchable, but it's not very general, so a better error message is surprisingly tricky.

@jstewmon's suggestion is a pattern I use sometimes as well, but it does suffer - as mentioned - from quietly not being checked if oneOfRefId is not evaluated. I have a draft design for "class constraints" that would allow you to

local class InternalLink((operationRef == null) == (operationId != null)) {
  operationRef: String?
  operationId: String?
}

This essentially adds the constraint to all instances of the class. I'm hoping to come back to this before too long.

By the way... you may want to express this more principled as a union type.

@holzensp commented on GitHub (Feb 5, 2024): The problem here is that `xor` eagerly evaluates its arguments. More generally, though, this circular dependency is fundamentally an unbounded-depth recursion (i.e. it stack overflows). I tend to write this as ``` operationRef: String?((this == null) == (operationId != null)) ``` but adding the inverse to `operationId` also still gives you a stack overflow when both are non-null. This special-case of two properties referring directly to each other might be catchable, but it's not very general, so a better error message is surprisingly tricky. @jstewmon's suggestion is a pattern I use sometimes as well, but it does suffer - as mentioned - from quietly not being checked if `oneOfRefId` is not evaluated. I have a draft design for "class constraints" that would allow you to ``` local class InternalLink((operationRef == null) == (operationId != null)) { operationRef: String? operationId: String? } ``` This essentially adds the constraint to all instances of the class. I'm hoping to come back to this before too long. By the way... you _may_ want to express this more principled as a union type.

adam commented

2025-12-30 01:19:35 +01:00

@jstewmon commented on GitHub (Feb 5, 2024):

By the way... you may want to express this more principled as a union type.

I considered this, but IIUC we are limited to single-inheritance for classes, which means we can only practically support a single "one of" constraint, right?

Using the original example, we might factor out to a union type like this:

abstract class LinkBase {
  parameters: Mapping<String, Any>
  requestBody: unknown?
  description: String?
}

class LinkRef extends LinkBase {
  operationRef: String
}

class LinkId extends LinkBase {
  operationId: String
}

typealias Link = LinkRef | LinkId

But, if I have n "one of" constraints, it would be impractical to define discrete types for all possible combinations.

I think the type system would need to support some form of type intersection (a la typescript) to make this approach practical. I'm not suggesting that Pkl needs to support type intersections, but I'd be more inclined to use constraints on a "wide" type than to try to factor out base/union types without it.

@jstewmon commented on GitHub (Feb 5, 2024): > By the way... you _may_ want to express this more principled as a union type. I considered this, but IIUC we are limited to single-inheritance for classes, which means we can only practically support a single "one of" constraint, right? Using the original example, we might factor out to a union type like this: ``` abstract class LinkBase { parameters: Mapping<String, Any> requestBody: unknown? description: String? } class LinkRef extends LinkBase { operationRef: String } class LinkId extends LinkBase { operationId: String } typealias Link = LinkRef | LinkId ``` But, if I have _n_ "one of" constraints, it would be impractical to define discrete types for all possible combinations. I think the type system would need to support some form of type intersection (a la typescript) to make this approach practical. I'm not suggesting that Pkl needs to support type intersections, but I'd be more inclined to use constraints on a "wide" type than to try to factor out base/union types without it.

adam commented

2025-12-30 01:19:35 +01:00

@holzensp commented on GitHub (Feb 6, 2024):

Single inheritance; true. Type intersection; might be nice (there's a nuanced discussion to be had there). It depends how you intend to consume this; if you want to produce text output with an output renderer, you might want to consider the only distinction (in this case) is the name of the property. You can use converters to do something useful;

class Link {
  parameters: Mapping<String, Any>
  requestBody: unknown?
  description: String?
  hidden operationType: *"Ref"|"Id"
  operation: String
}

output {
  renderer = new JsonRenderer {
    converters {
      [Link] = (it) -> new Mapping {
        for (k, v in it.toMap) {
          [k + if (k == "operation") it.operationType else ""] = v
        }
      }
    }
  }
}

@holzensp commented on GitHub (Feb 6, 2024): Single inheritance; true. Type intersection; might be nice (there's a nuanced discussion to be had there). It depends how you intend to consume this; if you want to produce text output with an output renderer, you might want to consider the only distinction (in this case) is the name of the property. You can use `converters` to do something useful; ``` class Link { parameters: Mapping<String, Any> requestBody: unknown? description: String? hidden operationType: *"Ref"|"Id" operation: String } output { renderer = new JsonRenderer { converters { [Link] = (it) -> new Mapping { for (k, v in it.toMap) { [k + if (k == "operation") it.operationType else ""] = v } } } } } ```

adam commented

2025-12-30 01:19:35 +01:00

@holzensp commented on GitHub (Feb 9, 2024):

Closing this issue, because it's a natural consequence of the language definition. If you feel it shouldn't be (and have some further thoughts on how it could be improved), feel free to re-open, @madisp

@holzensp commented on GitHub (Feb 9, 2024): Closing this issue, because it's a natural consequence of the language definition. If you feel it shouldn't be (and have some further thoughts on how it could be improved), feel free to re-open, @madisp

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/pkl#16