sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.78k stars 152 forks source link

TypeCheck for `void` should pass for `undefined`, not `null` #342

Closed ehaynes99 closed 1 year ago

ehaynes99 commented 1 year ago

In the tests, we have this block for the TVoid type: https://github.com/sinclairzx81/typebox/blob/078cfacd44336aa01528c3241658516d7f2ec46e/test/runtime/compiler/void.ts#L30-L38

This is inconsistent with the TypeScript compiler. void symbolizes the lack of a return value, which means that the return value is not defined. TypeScript treats void and undefined as equivalents, but does NOT treat null as equivalent to either:

// compiles correctly
const example = (): void => {
  return undefined
}

vs.

const example = (): void => {
  // ERROR: typescript: Type 'null' is not assignable to type 'void'.
  return null
}

Edit: Actually, upon closer inspection, that test is using the wrong type. It's in the void.ts test, but using Type.Null() in the first test. However, the checker for Type.Void() also passes for null.

ehaynes99 commented 1 year ago

This is also inconsistent with TypeBox's own Static:

const Example = Type.Void()
type Example = Static<typeof Example>

// OK
const v1: Example = undefined

// ERROR: typescript: Type 'null' is not assignable to type 'void'.
const v2: Example = null
sinclairzx81 commented 1 year ago

@ehaynes99 Hi!

Yes, this is a quirk of TypeBox (one I am quite open to reviewing). There is a very technical reason why Void asserts with null (that ill outline below), but I am considering having Type.Void() validate for both null | undefined through configuration in later releases, but with undefined used as the default.

const T = Type.Void()

Value.Check(T, undefined) // true
Value.Check(T, void 0)    // true
Value.Check(T, null)      // true with configuration .... but why?

JSON RPC 2.0 and Void Return

The reason why Type.Void() currently validates for null is due in part to the Sidewinder project https://github.com/sinclairzx81/sidewinder requiring some way to handle void return for the function signature below. Sidewinder uses the JSON RPC 2.0 protocol for communication, and was originally built with Ajv for it's validator (which is largely why the schema representation for Void looks as it does)

The Problem

const F = Type.Function([], Type.Void()) // ... consider this signature

function F() {}                          // ... and this implementation

const result = F()                       // ... and this return value

const encoded = JSON.stringify({ jsonrpc: '2.0', id: 42, result }) 

//                                       { "jsonrpc": "2.0", "id":42 } - where is result?

The Specification

The following is an extract from the JSON RPC 2.0 specification for Response payloads. Note the "This member is REQUIRED on success." statement for result

https://www.jsonrpc.org/specification

Response

When a rpc call is made, the Server MUST reply with a Response, except for in the case of Notifications. The Response is expressed as a single JSON Object, with the following members:

The Solution

Because JSON RPC 2.0 mandates a result property on success. Sidewinder (and by extension TypeBox) needed someway to encode a value to signal void return. There were two options considered, number and null.

TypeBox opted for the null assertion over numeric assertion.

Type Inference

While TypeBox internally asserts Void as null, the type inference for this type is still void. It's reasoned this is "probably" safe for the following reasons:

Assert Null and Undefined Policy

There has recently been some updates to TypeBox to support overriding internal type checking policies. There are two currently supported. https://github.com/sinclairzx81/typebox/blob/master/src/system/system.ts#L46-L50

I may add an additional override policy for Void assertion (i.e. AllowVoidNull), and just default Void to assert for undefined by default.

Summary

I've never been particularly happy with TypeBox's Void interpretation, but sometimes disparate specifications just don't line up. I am open to discussion around this if you have any ideas. I think whatever implementation TypeBox goes with in the future, it's going to need to have clear rationales behind it, unfortunately the Void assertion is currently designed to solve a narrow problem related to JSON RPC 2.0...so very much open to change here.

Hope this brings some insight! S

sinclairzx81 commented 1 year ago

@ehaynes99 This should be resolved on 0.26.0. Type.Void() will only assert for values of undefined by default, however I've added a new AllowVoidNull assertion policy that allows for the null assertion (as per JSON RPC rationale above)

import { TypeSystem } from '@sinclair/typebox/system'
import { Value } from '@sinclair/typebox/value'
import { Type } from '@sinclair/typebox'

const R1 = Value.Check(Type.Void(), undefined) // true
const R2 = Value.Check(Type.Void(), void 0)    // true
const R3 = Value.Check(Type.Void(), null)      // false

TypeSystem.AllowVoidNull = true

const R4 = Value.Check(Type.Void(), null)      // true!

Will close off this issue for now. Cheers! S

ehaynes99 commented 1 year ago

Oh, wow. Sorry I didn't respond sooner. I drafted this a couple of times. :)


I can definitely respect the difficulty for some of these. There's no good way to handle void because the language itself didn't handle void very well. Honestly, I don't think we even needed it. It was a "what would Java do?" feature. JS already had an implicit return type of undefined, so they could have just used that. The existence of Promise<void> further highlights this.

The JSON RPC syntax requirement is actually more consistent with the original intent behind the two independent types null and undefined. The intended use for these was -- in the absence of actual static type checking -- a rudimentary way of defining types. A constructor function could always return an object with a known set of properties. The properties that were optional could be set to null, meaning "the property could exist on one of these things, but this particular one doesn't have a value." undefined, on the other hand, meant, "that's not a valid property for this type." Nothing enforced that, though, so people started using them interchangeably. There's even some advantage to doing it wrong, because it shrinks the serialized payloads to omit the optional values. TypeScript cemented this by making optional properties use undefined.

Admittedly I'm not terribly familiar with that spec, nor have I used SideWinder, but for my $0.02, it sounds like this is really more of "the framework should do the translation" problem. The return type of the handler function and the serialized result are only tangentially related. The framework could manage the translation, e.g.:

const result = F() ?? null
const encoded = JSON.stringify({ jsonrpc: '2.0', id: 42, result })