sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.56k stars 148 forks source link

Incorrect inferred type for the empty object `Type.Object({})`. #888

Open sdeprez opened 1 month ago

sdeprez commented 1 month ago

Code:

import {Static, Type} from '@sinclair/typebox'

const schema = Type.Object({})
type T = Static<typeof schema>

In that case, the type of T is {}, which in Typescript actually validates every non-null values, so the following assignments are all valid:

let t: T = 42
let t: T = false
let t: T = {extra_unwanted_keys: true}

One way to really represent the empty object type would be to use Record<string, never> which behaves the way we want, see https://www.totaltypescript.com/the-empty-object-type-in-typescript.

sinclairzx81 commented 1 month ago

@sdeprez Hi,

In that case, the type of T is {}, which in Typescript actually validates every non-null values, so the following assignments are all valid:

This is somewhat intentional (and partially historical) but mostly a consequence of TObject types not factoring additionalProperties constraints in inference (although the permissible number and string assignments are unfortunate). I think to provide better inference here, TObject should infer as one of the following depending on the whether additionalProperties has been specified.

For example, consider the following (where an empty object without constraints is permissible of additional properties)

const T = Type.Object({})                                          // type T = Record<PropertyKey, unknown>

const S = Type.Object({}, { additionalProperties: false })         // type T = Record<PropertyKey, never>

const U = Type.Object({}, { additionalProperties: Type.Number() }) // type T = Record<PropertyKey, number>

Unfortunately, this leads to some fairly awkward scenarios.

const T = Type.Object({
    x: Type.String(),
    y: Type.String(),
    z: Type.String(),
}, {
    additionalProperties: Type.Number()
})

type T = Record<PropertyKey, number> & {
    x: string,
    y: string,
    z: string
}

const a: T = { // error unassignable: property values of string
    x: '1',    // are not number
    y: '2',
    z: '3'
}

Do you have any thoughts or suggestions regarding the appropriate inference when factoring the above additional Property constraints? Open to discussion on this.

Cheers S

sdeprez commented 1 month ago

Thanks for your detailed response!

I agree that if we factor additionalProperties, it gets more complex :thinking: . But maybe we can at least improve the type {} for Type.object({}), which is arguably wrong because it allows any non-null values? It could use object for instance so at least it would only allow real objects, and not other primitive values, which is consistent with the generated json schema.

For the record, I'm using this great library ;) typebox inside fastify and I'm using Static to extract types from my serializers. I tripped over this issue here when I wondered when one route handler that was returning a boolean did not make the typecheck fail. So I would be happy with an object type instead, and it wouldn't check for additional properties but that's consistent with the other types from what I understand, and unwanted properties would be removed by the serializer anyway.

sinclairzx81 commented 1 month ago

@sdeprez Hi!

But maybe we can at least improve the type {} for Type.object({}), which is arguably wrong because it allows any non-null values? It could use object for instance so at least it would only allow real objects, and not other primitive values, which is consistent with the generated json schema.

Yeah, let me have a think about this. I think if making changes to TObject inference, I'd want to factor the additionalProperties in that inference. Similarly based on https://github.com/sinclairzx81/typebox/issues/889, I think there is also a case to consider factoring unevaluatedProperties in inference too (where this issue highlights some of the cross over between inference, types and runtime assertion logic).

Ill add a consideration tag to this issue and take a technical deep dive on it next time I get to sit down with the code. However given the complexity and general changes required, I wouldn't expect this functionality to land until the next minor revision (scheduled later on this year), but will see how things go.

Will keep you posted! S