Switch IsCallable and IsConstructor to return boolean

[rbuckton]

This changes the behavior of IsCallable and IsConstructor to return boolean instead of BooleanValue so that they can be turned into type guards in a future type-safety PR.

[Jack-Works]

I like this idea, but it is ambiguous when we should use BooleanValue and when we should use a boolean. Also, the test failed.

[rbuckton]

I like this idea, but it is ambiguous when we should use BooleanValue and when we should use a boolean.

There are a few other functions that return boolean rather than BooleanValue, so it's not unheard of. Unfortunately, using boolean is the only way to enforce a user-defined type guard in TypeScript.

Also, the test failed.

I missed a usage of IsCallable in test/test262.js, which has now been fixed. That does, apparently, make this a breaking change that may require a new major version, assuming engine262 follows semver.

[Jack-Works]

do you feel ok with this change? cc @devsnek

[devsnek]

there used to be a util to call runtime/abstract ops from inside js by prefixing them with %, like %IsCallable(f), in which case it was useful for them to return the lang values. additionally it matches the spec better. but ultimately i don't think either of those things matter too much for simple operations like this, so 🤷

my biggest curiosity is around the "future type safety". The spec does not statically care whether a value is callable or a constructor, in fact that's why these test functions exist in the first place, so why would engine262 care? if this work is generally going in the direction of what happened to completion.mts i would suggest not doing it.

That does, apparently, make this a breaking change that may require a new major version, assuming engine262 follows semver.

this package doesn't follow semver, but you should probably double check that https://engine262.js.org keeps working: https://github.com/engine262/engine262.github.io/blob/master/src/worker.js

[Jack-Works]

For me, the type of safety that is worth adding is if we can avoid casting things. (Cast is somehow a foot gun that might get unaligned with the actual status of the code)

For example, if there is code like below, I think this is worth adding:

if (IsGenerator(value) === Value.true) {
    const brand = (value as GeneratorObjectValue).GeneratorBrand
}
// to
if (IsGenerator(value)) {
    const brand = value.GeneratorBrand
    // do not need cast, value is already GeneratorObjectValue
}

Assert(IsGenerator(value) === Value.true)
const brand = (value as GeneratorObjectValue).GeneratorBrand

// to
Assert(IsGenerator(value))
const brand = value.GeneratorBrand
// do not need cast, value is already GeneratorObjectValue