samchon / typia

Super-fast/easy runtime validators and serializers via transformation
https://typia.io/
MIT License
4.45k stars 153 forks source link

TypeScript: Property of T | undefined Check Mismatch #519

Open sinclairzx81 opened 1 year ago

sinclairzx81 commented 1 year ago

Summary

TypeScript flags the following code with an error

TypeScript Link Here

type T = { x: number | undefined }

function test(value: T) {}

test({}) 
//    ^
//    Property 'x' is missing in type '{}' but required in type 'T'

However the is and equals checks return true for the same type.

import typia from 'typia'

type T = { x: number | undefined }

const X = typia.is<T>({})
const Y = typia.equals<T>({})

console.log(X) // true | expect false
console.log(Y) // true | expect false

Notes

The intent here is to validate for the policy of T1 however Typia appears to be using the policy of T2.

type T1 = { x: number | undefined } // x property key is REQUIRED
                                    // x property value can be (number | undefined)

type T2 = { x?: number }            // x property key is OPTIONAL
                                    // x property value can be (number | undefined)

Use Case

This was previously discussed on https://github.com/samchon/typia/pull/256#issuecomment-1287826222, however may be worth revisiting for the following usage.

type T = { x: number | undefined } // expect 'x' key to be required property

const V = {}                       // an object without 'x' property

const R = typia.is<T>(V)           // reports true, suggesting 'x' exists

const K = Object.keys(V)           // error: expected ['x'] got []

System

Node: 16.17.1 TypeScript: 4.9.5 Typia: 3.5.7

Config

{
  "transform": "typia/lib/transform",
  "functional": true,  
  "numeric": true,  
  "finite": true, 
}
samchon commented 1 year ago

How to distinguish it?

sinclairzx81 commented 1 year ago

Compiler

The following will check for the optional modifier ? on ts.PropertySignature compiler side.

function isOptionalProperty(node: ts.PropertySignature) {
  return node.questionToken !== undefined
}

Assertion

The runtime assertion would require checking for the existence of the key x ONLY if:

To do the assertion, you will want to ensure it's only done when necessary, this is how I'd probably approach it.

During Emit:

Ensure T assertion occurs BEFORE undefined assertion in T | undefined. This to short circuit the expression and only run the slower 'x' in value check when the value is undefined.

samchon commented 1 year ago

Ah, I got it by your hint. It is possible to distinguish through obj.hasOwnProperty(key) function.

It would better to support the explicit undefined type through configuration.

This undefined configuration seems the proper case. Do you agree? Then I will develop in this weekend.

https://github.com/samchon/typia/blob/e5d4d7af0f37ac038496dc36cddf09be13489ecb/src/transformers/ITransformOptions.ts#L48-L61

sinclairzx81 commented 1 year ago

Ah, I got it by your hint. It is possible to distinguish through obj.hasOwnProperty(key) function.

Sure, just use which ever has the fastest runtime performance.


It would better to support the explicit undefined type through configuration.

I would have expected this check to be the default as I feel it's better capturing the expected semantics of TypeScript required and optional property key checks (as seen here), But if implementing as a config, would it also be possible to provide a strict option that just switches everything on?

{
  "transform": "typia/lib/transform",
  "strict": true, // function + numeric + finite + nan + required-undefined
}

Or maybe reading strict: true from tsconfig.json ?


samchon commented 1 year ago

Do you have any idea about expected type name of error instaces? (TypeGuardError and IValidation.errors)

sinclairzx81 commented 1 year ago

@samchon I do, but error handling is perhaps a topic for another time. I could recommend setting up a TypeGuardErrorCode enumeration similar to this, and update TypeGuardError to accept the code on it's constructor. This code can be used to differentiate between different classes of error (i.e. missing property keys), as well as make provisions for internationalization (i18n) support in future.

export enum TypeGuardErrorCode {
   ObjectRequiredProperties,    // use this code if missing property (this error)
   ObjectAdditionalProperties,  // use this code if additional properties
   Boolean,                     // use this code if not a boolean
   String,                      // use this code if not a string
   Number,                      // use this code if not a number
   Undefined,                   // use this code if not a undefined
   Null,                        // use this code if not null
   // ... more
   Unknown                      // use this code on unknown error
}

export class TypeGuardError extends Error {
  constructor(
    public readonly code: TypeGuardErrorCode, // error code detected during assertion
    public readonly path: string,             // RFC 6901 JSON Pointer
    public readonly value: unknown,           // actual: value
    public readonly expect: unknown,          // expect: type
    message: string                           // optional message (code more important!)
  ) {
    super(message)
  }
}

Just so long as typia.is<{ x: undefined }>({}) is returning false, you should be able to throw the following on assert at a later time.

throw new TypeGuardError(TypeGuardErrorCode.ObjectRequiredProperty, '/x', undefined, 'undefined', 'Expect required property')
samchon commented 1 year ago

If change the TypeGuardError, when it would be a break change causing major version update to v4.0.

I should delay this issue for a while and consider how to solve it.

sinclairzx81 commented 1 year ago

This has been implemented recently in TypeBox. To implement this correctly, you will need to check if the property value extends undefined. TypeBox implements a fast path extends check specifically for this case (but it should be possible perform a similar check using the Compiler API by checking for unions of undefined).

hlovdal commented 11 months ago

@sinclairzx81 Those links works fine now, but the branch is a moving target and the links are almost guaranteed to be broken when someone reads this in the future. Could you update to specific commit references instead?

sinclairzx81 commented 11 months ago

@hlovdal

Could you update to specific commit references instead?

Updated