refinement chains unexpectedly violate type safety

jedwards1211 commented 3 months ago

z.string()
  .refine((s): s is FooBarString => ...)
  .refine((s: FooBarString) => // no TS error, but this can get called with a non-FooBarString
    ...
  )

I don't personally understand the benefit of the existing behavior, but if it's kept that way, the types should at least be fixed to where the same type is used for the value argument to each .refine predicate in the chain, instead of the output of the previous .refine.

I think the existing behavior is especially unfortunate in cases like this:

z.string().url().refine(
  // this should be okay, s is definitely a url right?
  s => new URL(s).protocol === 'https:'
).parse('foo')

Surprise, nope:

Uncaught TypeError: Invalid URL
    at new URL (node:internal/url:775:36)
    at REPL39:3:8
    at Object.refinement (/Users/andy/gh/clarity/node_modules/.pnpm/zod@3.23.8/node_modules/zod/lib/types.js:217:28)
    at executeRefinement (/Users/andy/gh/clarity/node_modules/.pnpm/zod@3.23.8/node_modules/zod/lib/types.js:3178:39)
    at ZodEffects._parse (/Users/andy/gh/clarity/node_modules/.pnpm/zod@3.23.8/node_modules/zod/lib/types.js:3198:17)
    at ZodEffects._parseSync (/Users/andy/gh/clarity/node_modules/.pnpm/zod@3.23.8/node_modules/zod/lib/types.js:146:29)
    at ZodEffects.safeParse (/Users/andy/gh/clarity/node_modules/.pnpm/zod@3.23.8/node_modules/zod/lib/types.js:176:29)
    at ZodEffects.parse (/Users/andy/gh/clarity/node_modules/.pnpm/zod@3.23.8/node_modules/zod/lib/types.js:157:29) {
  code: 'ERR_INVALID_URL',
  input: 'foo'
}

I don't think anyone would naively expect Zod to try the refinement on non-URLs, since the behavior of other Zod nodes besides refinements is to abort the chain if one node fails parsing.

jedwards1211 commented 3 months ago

I think a better compromise would be for builtin checks like .min(1).max(128).url() to all be tried, since the goal is to receive all of those error messages, but for custom refinements to be skipped after an error unless we explicitly opt into it. Would be a good breaking change for v4.

Especially considering that builtin checks don't create a chain of Zod nodes under the hood like custom refinements do, I think a behavior distinction between builtin checks and custom refinements makes sense.

sunnylost commented 2 months ago

Does https://zod.dev/?id=abort-early match your use case?

jedwards1211 commented 2 months ago

@sunnylost of course I can work around this issue in various ways, including the fatal flag, but I am trying to advocate improving the API to where we don't have to work around it, and it's less surprising.

benj-dobs commented 6 days ago

I strongly agree that the types should be fixed.

I agree that I would find it more useful to abort-early by default. In particular, having that option within the simpler refine API (rather than superRefine) would be great, even if it's opt-in. But mainly I want the types to be accurate :)

colinhacks / zod

refinement chains unexpectedly violate type safety #3667