arktype icon indicating copy to clipboard operation
arktype copied to clipboard

Use the result with the shortest path when finalizing errors for a cyclic value

Open ssalbdivad opened this issue 10 months ago • 1 comments

A Boston TS attendee (please take credit if you see this) suggested this could be optimized using a breadth-first approach to traversal.

While changing the traversal pattern would likely incur a significant performance cost, for cyclic types, we already need to keep track of a map of which values have been checked by which types.

Currently, if the combination we're checking has already been checked (whether valid or not), we don't recheck it. We still don't need to recheck the value, but when this occurs, we could update the error path if the one we just encountered is shorter than the one that is stored.

In the example at https://arktype.io/docs/scopes/, , we should expect the following improvement:

❌ problems:

dependencies/0/dependencies/0/contributors/0/email must be a valid email (was 'david@sharktypeio')
contributors/0/email must be a valid email (was 'david@sharktypeio')
❌ problems:

contributors/0/email must be a valid email (was 'david@sharktypeio')

ssalbdivad avatar Apr 19 '24 15:04 ssalbdivad

I started working on implementing this but it was more complex and had more perf cost than anticipated to remap the errors. I'm going to revert this for now, here's what I started with in case someone has a better solution and wants to pick it up:

on TraversalContext:


	// Each cyclic alias adds objects it encounters to this map with a
	// corresponding value of the shortest path at which the value was
	// encountered. This allows us to include only the error message
	// corresponding to the shortest path.
	seen: { [name in string]?: Map<unknown, TraversalPath> } = {}
	cyclicAliases: string[] = []


	traverseAlias<traverseResolution extends TraversalMethod>(
		alias: string,
		traverseResolution: traverseResolution,
		data: unknown
	): ReturnType<traverseResolution> {
		const seen: Map<unknown, TraversalPath> = (this.seen[alias] ??= new Map())
		const seenPath = seen.get(data)
		if (seenPath === undefined) {
			// if this is the first time the cyclic type has checked a reference
			// to data, initialize to the current path
			seen.set(data, [...this.path])
			this.cyclicAliases.push(alias)
			traverseResolution(data, this) as never
			this.cyclicAliases.pop()
		} else if (this.path.length < seenPath.length) {
			// if we've already validated the data but at a longer path,
			// transform any corresponding errors to use the current path
			this.errors.forEach(e => {
				if (e.cyclicAliases.includes(alias))
					e.path = [...this.path, ...e.path.slice(seenPath.length)]
			})
			seen.set(data, [...this.path])
		}
		// otherwise, the data has already been validated at a location at least as shallow,
		// so we don't need to validate anything else.
		return true as never
	}

Errors also had cyclicAliases attached.

ssalbdivad avatar May 14 '24 20:05 ssalbdivad