gcanti / io-ts

Runtime type system for IO decoding/encoding
https://gcanti.github.io/io-ts/
MIT License
6.71k stars 328 forks source link

Validation should fail when required property (which may be `undefined`) is missing #596

Open OliverJAsh opened 3 years ago

OliverJAsh commented 3 years ago

🐛 Bug report

Current Behavior

const t = require('io-ts')
const result = t.type({ foo: t.string, bar: t.union([t.undefined, t.string])}).decode({ foo: 'foo' })
console.log({result});
// { result: { _tag: 'Right', right: { foo: 'foo', bar: undefined } } }

Expected behavior

I expected it to return a Left to match the TS behaviour:

// Errors as expected
// Property 'bar' is missing in type '{ foo: string; }' but required in type '{ foo: string; bar: string | undefined; }'.ts(2741)
const result: { foo: string; bar: string | undefined } = { foo: 'foo' };

This is because bar is defined as a required property which must be explicitly set as either string or undefined.

Reproducible example

See above.

Suggested solution(s)

Additional context

Your environment

Which versions of io-ts are affected by this issue? Did this work in previous versions of io-ts?

Software Version(s)
io-ts
fp-ts
TypeScript
samhh commented 3 years ago

Potentially relevant:

SHaTRO commented 3 years ago

Semantically speaking "undefined" and not defined are supposed to be equivalent. Requiring "undefined" to define something as "undefined" is literally a contradiction. That is why missing and undefined have been treated as semantically equivalent - because they are.

If you need to define something as having no value, then first it must be a thing (defined) and secondly must be defined as having no value (also defined). That is what "null" is for. The reason we frown on "null" is because it does this: defines a property as having no value.

Now, if we were to say, change our semantics to "declare" that a property is undefined - we are still defining it when we declare it, but we are "defining" it as "undefined" which is a contradiction. Folks who insist on doing this should be using "null" to distinguish between "defined as not having a value" and "undefined" (which means missing/empty/void).

I believe this interpretation is completely consistent with --strictOptionalProperties


TL;DR - to say that a "required" condition is fulfilled by declaring something as "undefined" is semantically incorrect. Something that is declared as "undefined" has also been declared as "missing".

OliverJAsh commented 2 years ago

Semantically speaking "undefined" and not defined are supposed to be equivalent.

@SHaTRO I'm not sure if they are equivalent in terms of behaviour, because the in operator will produce different results when the key doesn't exist versus when it does exist but the value is set to undefined:

type A = { foo: string | undefined }
const a: A = { foo: undefined };
'foo' in a; // true

type B = { foo?: string }
const b: A = {};
'foo' in b; // false

I'm not trying to imply it's a good idea to use "explicit undefined"s, rather I'm just pointing out that the behaviour in io-ts doesn't match the behaviour in TypeScript.

Say we have this type:

const t = require('io-ts')
const I = t.type({ foo: t.string, bar: t.union([t.undefined, t.string])})

It is reasonable to expect that, after a successful decode, 'bar' in value will produce true only if the key existed on the input value, because we have declared the key as required.

However this is not currently the case. In this example, the decode succeeds even though the required key is missing:

const result = I.decode({ foo: 'foo' })
console.log({result});
// { result: { _tag: 'Right', right: { foo: 'foo', bar: undefined } } }
console.log('bar' in result)
// => true
// Key 'bar' never existed in the input value ❗️

OTOH, if we model the type using a TypeScript type instead, the assignment fails:

// Errors as expected
// Property 'bar' is missing in type '{ foo: string; }' but required in type '{ foo: string; bar: string | undefined; }'.ts(2741)
const result: { foo: string; bar: string | undefined } = { foo: 'foo' };
ragnese commented 2 years ago

@SHaTRO Unfortunately, I have to agree with @OliverJAsh. TypeScript has made the (questionable, IMO) decision that there is a distinct difference between having a field with a value of undefined and a field not being present/assigned.

I'm sure we're all aware of the following, but I'll add it just to be clear:

interface OptionalField {
    field?: number
}
interface RequiredField {
    field: number | undefined
}
declare const o: OptionalField
declare const r: RequiredField

let x: OptionalField = o
let y: RequiredField = r

x = y // Okay
y = x // Error

In light of this, I think the behavior suggested by this issue would be more correct (albeit very tedious and usually not what a user of this library probably wants most of the time).

Perhaps it would be valuable to include (in addition to the suggestion in OP) an optional() wrapper that works like partial({}), but only for individual fields of a Props definition, rather than needing to intersect your non-partial fields with your partial fields to get the desired effect.

OliverJAsh commented 2 years ago

I just realised that this also means the following codec law does not hold:

import * as assert from "assert";
import * as E from "fp-ts/Either";
import { pipe } from "fp-ts/function";
import * as t from "io-ts";

const codec = t.type({
    foo: t.undefined,
});

const u = {};

// ❌ This fails
assert.deepEqual(
    pipe(
        codec.decode(u),
        E.fold(() => u, codec.encode)
    ),
    u
);
AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

{
  foo: undefined
}

should loosely deep-equal

{}

Here's a more practical example that uses optionFromNullable:

import * as assert from "assert";
import * as E from "fp-ts/Either";
import { pipe } from "fp-ts/function";
import * as t from "io-ts";
import { optionFromNullable } from "io-ts-types";

const codec = t.type({
    foo: optionFromNullable(t.string),
});

const u = {};

// ❌ This fails
assert.deepEqual(
    pipe(
        codec.decode(u),
        E.fold(() => u, codec.encode)
    ),
    u
);
AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

{
  foo: null
}

should loosely deep-equal

{}