fix(typeEvaluator): fix in-operator union handling

[sgulseth]

Two changes is involved in this pr:
* This pr builds on top of #253 which fixes in-operator on union types
* Removes resolveCondition so that we can reuse types that are walked when we are resolving filters.

[sgulseth]

A bit annoying to need this, but the reason is that when resolving "and" conditions like _id == $id && _type == "foo" we don't want to short circuit the mapper when walking _id == $id(resolves to unknown because parameter is unknown).

Alternatively we can use mapUnion in handleOpCallNode instead, and there do a unknown check so it at least returns a boolean.

function handleOpCallNode(node: OpCallNode, scope: Scope): TypeNode {
  $trace('opcall.node %O', node)
  const lhs = walk({node: node.left, scope})
  const rhs = walk({node: node.right, scope})
  return mapUnion(lhs, scope, (left) =>
    // eslint-disable-next-line complexity, max-statements
    mapUnion(rhs, scope, (right) => {
      $trace('opcall.node.concrete "%s" %O', node.op, {left, right})
      if (left.type === 'unknown' || right.type === 'unknown') {
        return {type: 'boolean'}
      }

Then we would resolve the correct type for a projection like:

{
"foo": $myParameter == 1
}

Which currently resolves to unknown

[judofyr]

_id == $id(resolves to unknown because parameter is unknown)

Just a quick comment (before reading the rest of the problem): == (and !=) are one of the few operators which actually always return true/false (null is not possible). So regardless of what the RHS is then this will be boolean.

[judofyr]

And similarly: Comparisons operators are always boolean | null. We should always be able to fallback to that.

[sgulseth]

Change this into function handleOpCallNode handling unknowns with mapUnion, and ==/!= will now always return a boolean, while the other operators short circuits to unknown if an unknown is given.

[sgulseth]

neither foo nor bar exists on the document, so this will always be false

[sgulseth]

_id is a string, so fixed the comparison

[sgulseth]

a typo, but added the actual attribute so that the test makes sense

[judofyr]

Having evaluate type tests for And and Or will probably uncover potential bugs around the handling.

t.test(`And`, async (t) => {
  subtestBinary({
    t,
    variants1: trivialVariant,
    variants2: trivialVariant,
    build: (left, right) => ({type: 'And', left, right})
  })
})

t.test(`Or`, async (t) => {
  subtestBinary({
    t,
    variants1: trivialVariant,
    variants2: trivialVariant,
    build: (left, right) => ({type: 'Or', left, right})
  })
})

[judofyr]

_id == $id(resolves to unknown because parameter is unknown)

Just a quick comment (before reading the rest of the problem): == (and !=) are one of the few operators which actually always return true/false (null is not possible). So regardless of what the RHS is then this will be boolean.

[judofyr]

And similarly: Comparisons operators are always boolean | null. We should always be able to fallback to that.

[sgulseth]

A bit hacky, but just needed to short circuit the operator if used with path.

Another approach, that might come in a follow-up PR is to put the scope into filterMode, and we can then use that to determine what typenode to return in the function. This could also then be used to rewrite the scope when using literal comparisons or the defined method

[judofyr]

One proper request: The handling of null + true seems broken in And/Or. I think this would have been covered by adding a typeEvaluateCompare test where we add union({type: 'null'}, …). Currently we only union by unknown. It's a bit tricky to add this test without exploding all of the variants though. We might need more fine-grained per-op variations…

One suggestion which can be done separately from this PR: Don't distinguish between mapConcrete and mapUnion, but always use one (I propose keeping the name mapConcrete) which also requires you to handle unknown.

[judofyr]

Isn't this wrong if both values are [canBeTrue, canBeNull] and [canBeTrue, canBeNull]? 🤔

In which case the actual type should be nullIUnion({type: 'boolean'}), but here it would be detected as {type: 'boolean', value: true}?

I think potentially this can be easier expressed by defining the logic on BooleanInterpretation:

function booleanOr(left: BooleanInterpretation, right: BooleanInterpretation): BooleanInterpretation {
  // One side is true => return `true`
  if (left.canBeTrue && !left.canBeFalse && !left.canBeNull) return left
  if (right.canBeTrue && !right.canBeFalse && !right.canBeNull) return right

  // One side is false => return the other side
  if (!left.canBeTrue && left.canBeFalse && !left.canBeNull) return right
  if (!right.canBeTrue && right.canBeFalse && !right.canBeNull) return left

  // Guaranteed to be boolean at least:
  if (!left.canBeNull && !right.canBeNull) return {canBeTrue: true, canBeFalse: true, canBeNull: false}

  // Fallback to the most permissive:
  return {canBeTrue: true, canBeFalse: true, canBeNull: true}
}

I think this covers the main cases.

And then and becomes quite symmetric:

function booleanAnd(left: BooleanInterpretation, right: BooleanInterpretation): BooleanInterpretation {
  // One side is true => return that side
  if (left.canBeTrue && !left.canBeFalse && !left.canBeNull) return right
  if (right.canBeTrue && !right.canBeFalse && !right.canBeNull) return left

  // One side is false => return `false`
  if (!left.canBeTrue && left.canBeFalse && !left.canBeNull) return left
  if (!right.canBeTrue && right.canBeFalse && !right.canBeNull) return right

  // Guaranteed to be boolean at least:
  if (!left.canBeNull && !right.canBeNull) return {canBeTrue: true, canBeFalse: true, canBeNull: false}

  // Fallback to the most permissive:
  return {canBeTrue: true, canBeFalse: true, canBeNull: true}
}

[sgulseth]

Fixed in #260