Runtime behavior of optional/undefinedable doesn’t match TS with exactOptionalPropertyTypes

[andersk]

#385 was resolved by adding v.undefinedable with different typing from v.optional:

import * as v from "valibot";

const optional_schema = v.object({ field: v.optional(v.number()) });
type OptionalOutput = v.InferOutput<typeof optional_schema>;
// { field?: number }

const undefinedable_schema = v.object({ field: v.undefinedable(v.number()) });
type UndefinedableOutput = v.InferOutput<typeof undefinedable_schema>;
// { field: number | undefined }

However, this typing is inconsistent with the runtime behavior: at runtime, both schemas accept and return both of {} and { field: undefined }. This means that both schemas break type safety with tsc --strict --exactOptionalPropertyTypes.

const a = v.parse(optional_schema, { field: undefined });
if ("field" in a) { // narrows a.field to string
  const n: number = a.field; // oops, this is undefined
}

const b = v.parse(
  v.union([undefinedable_schema, v.object({ bogus: v.string() })]),
  {}
);
if (!("field" in b)) { // narrows b to { bogus: string }
  const s: string = b.bogus; // oops, this is undefined
}

Either the types should be adjusted to match the runtime behavior, or the runtime behavior should be adjusted to match the types.

Zod does the former:

import { z } from "zod";

const optional_schema = z.object({ field: z.optional(z.number()) });
type OptionalOutput = z.output<typeof optional_schema>;
// { field?: number | undefined }

[andersk]

v.unknown has the same problem.

import * as v from "valibot";

const unknown_schema = v.object({ field: v.unknown() });
type UnknownOutput = v.InferOutput<typeof unknown_schema>;
// { field: unknown }

const b = v.parse(
  v.union([unknown_schema, v.object({ bogus: v.string() })]),
  {}
);
if (!("field" in b)) { // narrows b to { bogus: string }
  const s: string = b.bogus; // oops, this is undefined
}

In Zod, unknown implies optionality:

import { z } from "zod";

const unknown_schema = z.object({ field: z.unknown() });
type UnknownOutput = z.output<typeof unknown_schema>;
// { field?: unknown }

const b = z.union([unknown_schema, z.object({ bogus: z.string() })]).parse({});
if (!("field" in b)) { // safely avoids narrowing b
  const s: string = b.bogus; // safely fails type checking
}

[fabian-hiller]

Thank you for creating this PR. This is intentionally and documented. We do not distinguish between missing and undefined entries in object schemas. The reason for this decision is that it reduces the bundle size, and we also expect that most users will expect this behavior. But feel free to use this issue to discuss this decision with me and others.

[andersk]

If we do not distinguish between missing and undefined at runtime (which is fine), then to restore type safety, we need to change the type declarations to agree with the runtime behavior. That is,

• we need to change v.InferOutput<typeof optional_schema> to return { field?: number | undefined } (not { field?: number }), and
• we need to change v.InferOutput<typeof undefinedable_schema> to return { field?: number | undefined } (not { field: number | undefined }), and
• we need to change v.InferOutput<typeof unknown_schema> to return { field?: unknown } (not { field: unknown }),

to reflect the fact that we can in fact output either missing or undefined at runtime in all of these cases.

[fabian-hiller]

The current solution is a tradeoff where we intentionally do not distinguish between missing and undefined entries for a smaller bundle size, but still allow users to output the type they need while knowing that there is a mismatch in runtime behavior. I can understand why it sounds like a dump, but it seems to be the best trade at the moment.

We could also add a question mark when using undefinedable so that users can match the type and runtime behavior without having to wrap optional around undefinedable.

[andersk]

The whole job of a validation library is to establish that your runtime values match the expected types! Declaring fictitious types that don’t match the runtime behavior defeats the entire point. It does not help anyone, and is wildly unsafe, to make a compile time declaration that the value has the type you need, when at runtime the value does not have that type.

I understand that the current runtime behavior has a bundle size advantage. So the only good solution is to correct the types.

This is not a novel unproven proposal. Zod’s runtime has the same behavior as Valibot’s runtime, and Zod declares its types correctly, like I showed in the examples above.

[fabian-hiller]

Are you aware of any issues that can actually cause problems due to this mismatch in a specific scenario? So far, the pros of the current implementation outweigh the cons (feel free to read #385), but I am happy to re-evaluate and possibly change it.

[andersk]

I provided such scenarios in my original report. On both of the lines with a comment // oops, this is undefined, TypeScript infers that a variable is a string or a number when it is actually undefined at runtime.

A bug like this can result in an arbitrary wrong type being assigned to any value:

import * as v from "valibot";

function cast<T, U>(t: T): U {
  const a = v.parse(v.object({ b: v.optional(v.never()) }), { b: undefined });
  while (!("b" in a)) {}
  return a.b ?? t;
}

const n: number = cast("twenty-too");
console.log(n);

$ tsc --module Node16 --strict --exactOptionalPropertyTypes test.ts
$ node test.js
twenty-too

Please appreciate that a type safety violation is an actual problem. In a project as large as Zulip, it is not possible for the core reviewers and hundreds of new open-source contributors to keep the entire codebase in our heads, and manually account for subtle discrepancies between the compiler’s reported types and the actual types, so it is very easy for those discrepancies to snowball into major bugs and security vulnerabilities. We need to be able to rely on TypeScript’s correct checking, and TypeScript needs to be able rely on libraries’ correct type definitions in order to provide that.

Yes, I read #385; it’s the very first thing I linked in my original report. I’m pretty sure the reporter doesn’t realize how unsafe the situation has become (“I can now return the validation result directly and don't have to check for those undefined values anymore”).

[fabian-hiller]

Thank you very much for your time and feedback! I agree and will be reviewing and re-evaluating the situation over the next few weeks. I will share my findings with you so that we can discuss the final decision together.

[fabian-hiller]

I have thought about this and there is a problem. We can't know at runtime if exactOptionalPropertyTypes is enabled or not. There is no way to change the validation behavior based on this configuration. Therefore, I don't think the runtime behavior can always match the type information. The only way to "fix" this would be to not support exactOptionalPropertyTypes, but I am not sure if we should do that.

[andersk]

We don’t need to detect anything at runtime (or at all). When exactOptionalPropertyTypes is disabled, TypeScript already treats both { field: number | undefined } and { field?: number } as if they were { field?: number | undefined }. This also disables type narrowing based on if ("field" in number). So this type fix will only have an effect for those who’ve explicitly enabled exactOptionalPropertyTypes, without changing anything for those who haven’t.

If you have any doubt about how this should work, look at how Zod handles this; as far as I can tell, it does this all correctly.

[fabian-hiller]

As far as I know, Zod does not support exactOptionalPropertyTypes yet. So nothing will change if you enable this config. My understanding is that if this option is disabled, undefined should be accepted as a value at runtime, but not if it is enabled. This would mean we need to somehow know at runtime if it is enabled, but as far as I know this is not technically possible.

[andersk]

Zod works fine with exactOptionalPropertyTypes enabled. Zulip is using it that way.

Zod’s behavior, as I explained in my original report above, is that z.object({ field: z.optional(z.number()) }) outputs type { field?: number | undefined }, and accepts both the values {} and { field: undefined } at runtime, so that the type declaration matches its runtime behavior. This is the same whether exactOptionalPropertyTypes is enabled or disabled. No change is needed, and type safety is maintained either way.

(There are users who have requested that Zod provide a way to distinguish undefined from missing, which would potentially allow for finer types when exactOptionalPropertyTypes is enabled. But that is not what I am requesting here. I am requesting type safety.)

[fabian-hiller]

Thank you! I will discuss this with Colin from Zod and David from ArkType and report back here.