fix(typeEvlauator): handle rest on object splat operations

[sgulseth]

When splatting an object we didn't map over the rest property, if it's an object we should append the attributes. If rest is unknown we should treat the entire object as unknown.

Fixes sanity-io/sanity#6555

[judofyr]

Something looks a bit sketchy.

Here we invoke the mapper multiple times:

groq-js/src/typeEvaluator/typeEvaluate.ts

Lines 124 to 128 in 15ec7ac

	if (node.type === 'union') {
	for (const scoped of node.of) {
	mapObjectSplat(scoped, scope, mapper)
	}
	}

But here we assume that it's only being called once per name:

groq-js/src/typeEvaluator/typeEvaluate.ts

Lines 165 to 167 in 15ec7ac

	mapObjectSplat(value, scope, (name, attribute) => {
	attributes[name] = attribute
	})

mapObjectSplat also only handles union and object and ignores unknown. It seems that if you now splat something which we can't determine it ends up being ignored? 🤔

I'm not 100% sure what the right approach here is though. Maybe more usage of mapConcrete?

[sgulseth]

yess, this should probably be a concrete type with mapConcrete, will fix. Didn't think much about fixing other bugs than the one referenced 😅

[sgulseth]

@judofyr did another pass on comparing with mapped concrete type node

[judofyr]

Hmm. Doesn't this mean that if this returns false we end up returning {type: 'object', attributes: {}} below? What about we instead wrap all of this in mapConcrete?

return mapConcrete(node, scope, (node) => {
  if (node.type !== 'object') return {type: 'unknown'}
  const attributes: Record<string, ObjectAttribute> = {}
  // same same…
})

[sgulseth]

I did another pass, moved the handling of the object splats out in its own methods

[judofyr]

I don't think this makes sense: If the splatted node is not an object then we return that as the result of the whole object?

In the following query:

*[_type == "author"][0] {
    _type == "author" => select(age >= 18 => {lastname}, {firstname}),
    _id,
    _type
  }

Then we now end up generating:

{
  "type": "union",
  "of": Array [
    Object {
      "type": "object",
      "attributes": Object {
        "firstname": Object {
          "type": "objectAttribute",
          "value": Object {
            "type": "string",
          },
        },
      },
    },
    Object {
      "type": "object",
      "attributes": Object {
        "lastname": Object {
          "type": "objectAttribute",
          "value": Object {
            "type": "string",
          },
        },
      },
    },
    Object {
      "type": "null",
    },
  ],
}

Because the "conditional splat" turns into a union and thus we hit this case.

[judofyr]

This code has just basically takes a type: "object" and then makes a copy of it without rest or dereferencesTo. That doesn't look right to me.

[judofyr]

This means that if concreteRest.type is "union" we end up returning null?

[judofyr]

It's a bit unclear to me what the intended behavior of handling objects with unions and inlines are at the moment. If there's union in any of ObjectSplat there's multiple approaches:

• We can do the the "cross product": E.g. if there's 3 different object splats which all have 3 unions we end up with a top-level union of 9 different alternatives.
• We can choose that this is just always {type: "unknown"}.
• We can take the last union of objects, change all of them to have rest: unknown. That way we get typings for the "last" set of attributes, but not the "earlier" ones.

Maybe we can add some more fine-grained tests using overrideTypeForNode which demonstrates how it will behave in various cases?

[judofyr]

How about creating some tests like this (maybe turn it into a table-based one?):

const nodeWithType = (type: TypeNode) => {
  const expr: ExprNode = {type: 'Value', value: null}
  overrideTypeForNode(expr, type)
  return expr
}

t.test('objects', (t) => {
  // test 1
  typeEvaluate(
    {
      type: 'Object',
      attributes: [
        {type: 'ObjectAttributeValue', name: 'foo', value: nodeWithType({type: 'string'})},
      ],
    },
    [],
  )

  // test 2
  typeEvaluate(
    {
      type: 'Object',
      attributes: [
        {type: 'ObjectAttributeValue', name: 'foo', value: nodeWithType({type: 'string'})},
        {type: 'ObjectAttributeValue', name: 'foo', value: nodeWithType({type: 'number'})},
      ],
    },
    [],
  )

  // test 3
  typeEvaluate(
    {
      type: 'Object',
      attributes: [
        {type: 'ObjectAttributeValue', name: 'foo', value: nodeWithType({type: 'string'})},
        {type: 'ObjectSplat', value: nodeWithType({type: 'unknown'})},
      ],
    },
    [],
  )
})

[sgulseth]

I've addressed your feedback(100% agree), we should now be handling unions by always using mapConcrete. I also moved resolveInline into a separate function so we can be explicit about resolving inline nodes.

[judofyr]

Is there any value in returning {foo: boolean} | unknown instead of just unknown here?

[sgulseth]

We could short circuit so that if any of the returned elements are unknown we return unknown. Guess this could happen if you have a schema where a field is either an object or and array of something, not sure that's very realistic, though it could be during a migration for example.

If you have multiple attributes in the object, you could simplify the type check by just asserting one value and typescript would assume it's not the unknown object 🤔

Personally no strong opinions

[judofyr]

So, here we have:

• Attribute with noah
• Conditional splat with bear | fox | deer
• Conditional splat with deer | elk | moose
• Conditional splat with ox | pig | chicken

Wouldn't one possible choice here be an object with {noah, beer, deer, ox}? If all the conditionals match? I can't find that in the list.

[judofyr]

Also, it's way more common to have conditional splats with a plain object (e.g. _type == "foo" => {abc, def}) and not a union so it would be nice to have some tests for that as well.

[sgulseth]

Pushed my latest changes, there's optimizations to be done in the object handler itself(probably way too many loops 😅), but if it's correct I can give optimizations another passs

[judofyr]

I've mainly focused on the tests and they all look sensible to me.

(I haven't looked much at the implementation. Let me know if there's anything particular there you'd like me to review. Otherwise I'm pretty happy doing a "test-first" approach for this code anyway.)