pkl icon indicating copy to clipboard operation
pkl copied to clipboard

`is` operator does not take current scope into account

Open sin-ack opened this issue 1 year ago • 1 comments

Reproducer:

class Foo {}
local classes = new Listing {
    Foo
}.toList()

function check(a: Any) = classes.any((cls) -> a is cls)
local foo = new Foo {}

result = check(foo)

Error message:

$ jpkl eval test.pkl
–– Pkl Error ––
Cannot find type `cls`.

6 | function check(a: Any) = classes.any((cls) -> a is cls)
                                                       ^^^
at test#check.<function#1> (file:///home/agni/test.pkl, line 6)

Searched in the following places:
* Module `test` and any supermodules
* Module `pkl#base`

6 | function check(a: Any) = classes.any((cls) -> a is cls)
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at test#check (file:///home/agni/test.pkl, line 6)

9 | result = check(foo)
             ^^^^^^^^^^
at test#result (file:///home/agni/test.pkl, line 9)

106 | text = renderer.renderDocument(value)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at pkl.base#Module.output.text (https://github.com/apple/pkl/blob/47f2143/stdlib/base.pkl#L106)

This is contradictory with List.filterIsInstance which can take an arbitrary class at evaluation time (from what I can understand).

sin-ack avatar Oct 08 '24 08:10 sin-ack

is accepts a type, rather than an expression, and types and values exist in different namespaces.

This is intentional, and changing this would be a breaking change for how is works. It's akin to is in Kotlin, Swift, etc.

bioball avatar Oct 08 '24 13:10 bioball

Sorry for not getting back on this. I disagree with the decision but respect it. Is there a way to achieve what I want to do in Pkl, then, without resorting to tons of separate foo is Class checks?

sin-ack avatar Oct 11 '24 11:10 sin-ack

You can compare the class values directly, e.g.

import "pkl:reflect"

class Foo {}
local classes = new Listing {
    Foo
}.toList()

function isClassOrSubclass(value: Any, clazz: Class?) =
  if (clazz == null) false
  else if (value.getClass() == clazz) true
  else
    isClassOrSubclass(value, reflect.Class(value).superclass?.reflectee)

function check(a: Any) = classes.any((it) -> isClassOrSubclass(a, it))

bioball avatar Oct 11 '24 14:10 bioball

That works, thanks for the solution.

sin-ack avatar Oct 11 '24 15:10 sin-ack

@bioball How about adding method Class.isInstance(value) to base.Class?

odenix avatar Oct 13 '24 14:10 odenix

I'm open to it! Feel free to submit a PR for it. Maybe hasInstance is a better name.

bioball avatar Oct 13 '24 20:10 bioball