From f38a2d5e8892ebc81e087decf97a396cdb7899fb Mon Sep 17 00:00:00 2001 From: Glen Whitney Date: Sat, 12 Apr 2025 00:07:18 -0700 Subject: [PATCH] test: ensure that a function requiring conversion depends on converter This test turned out to be a very discerning one. Adding it uncovered numerous bugs, large and small, in the TypeDispatcher. The most major one was that a Map distinguishes keys by strict equality, and hence since every lookup for a cached behavior was using a newly-generated array of types, the cache was never being hit. So it looked like methods were being updated as dependencies changed, when what was really happening was that the behavior was simply being regenerated from scratch on every call, which would not be performant as the prototype scaled. This bug is now fixed (by switching to a third-party ArrayKeyedMap), along with many smaller bugs too numerous to list. It should now be feasible to go through Pocomath and add all of the functions that depend on numbers and booleans only. --- package.json5 | 3 + pnpm-lock.yaml | 9 ++ src/core/Type.js | 5 +- src/core/TypeDispatcher.js | 128 ++++++++++++++--------- src/core/TypePatterns.js | 23 +++- src/core/__test__/TypeDispatcher.spec.js | 14 ++- src/core/__test__/TypePatterns.spec.js | 10 +- src/core/__test__/helpers.spec.js | 4 +- src/core/helpers.js | 4 +- src/number/NumberT.js | 3 +- src/number/__test__/NumberT.spec.js | 9 +- 11 files changed, 151 insertions(+), 61 deletions(-) diff --git a/package.json5 b/package.json5 index 19d0d4f..50e6463 100644 --- a/package.json5 +++ b/package.json5 @@ -18,4 +18,7 @@ devDependencies: { mocha: '^11.1.0', }, + dependencies: { + 'array-keyed-map': '^2.1.3', + }, } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3a24d24..2704e5b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -7,6 +7,10 @@ settings: importers: .: + dependencies: + array-keyed-map: + specifier: ^2.1.3 + version: 2.1.3 devDependencies: mocha: specifier: ^11.1.0 @@ -49,6 +53,9 @@ packages: argparse@2.0.1: resolution: {integrity: sha512-8+9WqebbFzpX9OR+Wa6O29asIogeRMzcGtAINdpMHHyAg10f05aSFVBbcEqGf/PXw1EjAZ+q2/bEBg3DvurK3Q==} + array-keyed-map@2.1.3: + resolution: {integrity: sha512-JIUwuFakO+jHjxyp4YgSiKXSZeC0U+R1jR94bXWBcVlFRBycqXlb+kH9JHxBGcxnVuSqx5bnn0Qz9xtSeKOjiA==} + balanced-match@1.0.2: resolution: {integrity: sha512-3oSeUO0TMV67hN1AmbXsK4yaqU7tjiHlbxRDZOpH0KW9+CeX4bRAaX0Anxt0tx2MrpRpWwQaPwIlISEJhYU5Pw==} @@ -398,6 +405,8 @@ snapshots: argparse@2.0.1: {} + array-keyed-map@2.1.3: {} + balanced-match@1.0.2: {} binary-extensions@2.3.0: {} diff --git a/src/core/Type.js b/src/core/Type.js index e527f40..70e1ed1 100644 --- a/src/core/Type.js +++ b/src/core/Type.js @@ -1,3 +1,5 @@ +import {onType} from './helpers.js' + const typeObject = {} // have to make sure there is only one export const types = () => typeObject @@ -5,7 +7,8 @@ export const types = () => typeObject export class Type { constructor(f, options = {}) { this.test = f - this.from = new Map(options.from ?? []) + this.from = options.from ?? onType() // empty Implementations if no ... + // ... conversions specified } toString() { return this.name || `[Type ${this.test}]` diff --git a/src/core/TypeDispatcher.js b/src/core/TypeDispatcher.js index 76eace6..8c495d5 100644 --- a/src/core/TypeDispatcher.js +++ b/src/core/TypeDispatcher.js @@ -1,11 +1,10 @@ -import {typeOf, Type, bootstrapTypes} from './Type.js' +import ArrayKeyedMap from 'array-keyed-map' import { Implementations, isPlainFunction, isPlainObject, onType } from './helpers.js' -import { - alwaysMatches, matched, needsCollection, Any, Multiple -} from './TypePatterns.js' +import {bootstrapTypes, Returns, typeOf, Type} from './Type.js' +import {matched, needsCollection, Passthru} from './TypePatterns.js' export class TypeDispatcher { constructor(...specs) { @@ -37,25 +36,39 @@ export class TypeDispatcher { continue } if (typeof val === 'function') { - val = onType(Multiple(Any), val) + val = onType(Passthru, val) } if (val instanceof Implementations) { if (!(key in this)) { - // need to set up the item + // Need to "bootstrap" the item: + // We initially define it with a temporary getter, only + // because we don't know whether ultimately it will produce + // a function or a non-callable value, and the "permanent" + // getter we want depends on which it turns out to be. This + // situation means it's not supported to replace a key + // corresponding to a method, after it's been fetched once, + // with a definition that produces a non-callable value; + // the TypeDispatcher will go on returning a function, and + // even if you call that function, it will throw an error + // when it tries to call the non-callable value. + // Conversely, if you try to replace a key corresponding + // to a non-callable value, after it's been fetched once, + // with a function, it will work, but it will not be able to + // perform type dispatch on that function. Object.defineProperty(this, key, { enumerable: true, configurable: true, get: () => { let tryValue - let tryTypes = [] try { tryValue = this.resolve(key, []) } catch { - // Has no value for the empty type list, so - // find a type list it will have a value for - tryTypes = - this._implementations[key][0][0].sampleTypes() - tryValue = this.resolve(key, tryTypes) + // Has no value for the empty type list, so therefore + // it must be a method, as there is no way to supply + // any types for a non-function value. Hence, we can + // just make tryValue any plain function, since it is + // never actually used, just its type analyzed. + tryValue = () => undefined } // Redefine the property according to what sort of // entity it is: @@ -74,7 +87,7 @@ export class TypeDispatcher { }) return standard } - if (tryTypes.length) tryValue = undefined + if (typeof tryValue === 'object') { if (!('resolve' in tryValue)) { tryValue.resolve = types => this.resolve(key, types) @@ -92,6 +105,7 @@ export class TypeDispatcher { }) return get() } + Object.defineProperty(this, key, { enumerable: true, configurable: true, @@ -100,13 +114,16 @@ export class TypeDispatcher { return tryValue } }) + + // Finally, initialize the other data for this key: this._implementations[key] = [] - this._behaviors[key] = new Map() - this._dependencies[key] = new Map() + this._behaviors[key] = new ArrayKeyedMap() + this._dependencies[key] = new ArrayKeyedMap() } + // Now add all of the patterns of this implementation: for (const [pattern, result] of val.patterns) { - if (alwaysMatches(pattern)) { + if (pattern === Passthru) { if (key in this._fallbacks) this._disengageFallback(key) this._fallbacks[key] = result } else { @@ -128,7 +145,7 @@ export class TypeDispatcher { } // install value as a catchall value - this.merge({[key]: onType(Multiple(Any), val)}) + this.merge({[key]: onType(Passthru, val)}) } } @@ -151,7 +168,10 @@ export class TypeDispatcher { } else { const from = state.pos++ if ('actual' in elt) { // incorporate conversion - const convert = elt.matched.from.get(elt.actual)(this) + let convert = elt.convertor + if (!convert.returns) { // it's a factory that produces convert + convert = convert(this, elt.actual) + } extractors.push(args => convert(args[from])) } else extractors.push(args => args[from]) } @@ -167,14 +187,17 @@ export class TypeDispatcher { if (this.resolve._genDepsOf?.length) this._addToDeps(key, types) const behave = this._behaviors[key] + // Return the cached resolution if it's there if (behave.has(types)) return behave.get(types) + + // Otherwise, perform the resolution and cache the result const imps = this._implementations[key] let needItem = true let item - let pattern let template if (imps.length) { for (const options of [{}, {convert: true}]) { + let pattern for ([pattern, item] of imps) { let finalIndex ;[finalIndex, template] = pattern.match(types, options) @@ -189,50 +212,59 @@ export class TypeDispatcher { if (needItem && key in this._fallbacks) { needItem = false item = this._fallbacks[key] - template = [types] + template = types } if (needItem) { throw new TypeError(`no matching definition of '${key}' on '${types}'`) } - if (!isPlainFunction(item) || 'returns' in item) { + // If this key is producing a non-function value, we're done + if (!isPlainFunction(item)) { behave.set(types, item) return item } - // item is a Factory. We have to use it to build the behavior + + // item is a function, either a direct behavior or + // a factory. We have to use it to build the final behavior // First set up to record dependencies if (!('_genDepsOf' in this.resolve)) { this.resolve._genDepsOf = [] } - this.resolve._genDepsOf.push([key, types]) + let theBehavior = () => undefined - try { - theBehavior = item( - DependencyRecorder(this, [], this), matched(template)) - } catch { - // Oops, didn't work as a factory, so guess we were wrong. - // Just make it the direct value for this key on these types: - behave.set(types, item) - return item - } finally { - this.resolve._genDepsOf.pop() - } - if (typeof theBehavior === 'function' - && theBehavior.length - && needsCollection(template) - ) { - // have to wrap the behavior to collect the actual arguments - // in the way corresponding to the template. That may generate - // more dependencies: - this.resolve._genDepsOf.push([key, types]) + this.resolve._genDepsOf.push([key, types]) // Important: make sure + // not to return without popping _genDepsOf + if (!('returns' in item)) { + // looks like a factory try { - const collectFunction = this._generateCollectFunction(template) - theBehavior = (...args) => theBehavior(...collectFunction(args)) - } finally { - this.resolve._genDepsOf.pop() + theBehavior = item( + DependencyRecorder(this, [], this), matched(template)) + } catch { + // Oops, didn't work as a factory, so guess we were wrong. + // Just make it the direct value for this key on these types: + theBehavior = item + } + } else theBehavior = item + + let finalBehavior = theBehavior + if (typeof theBehavior === 'function') { + const returning = theBehavior.returns + if (!returning) { + throw new TypeError( + `No return type specified for ${key} on ${types}`) + } + if (theBehavior.length && needsCollection(template)) { + // have to wrap the behavior to collect the actual arguments + // in the way corresponding to the template. Generating that + // argument transformer may generate more dependencies. + const morph = this._generateCollectFunction(template) + finalBehavior = + Returns(returning, (...args) => theBehavior(...morph(args))) } } - behave.set(types, theBehavior) - return theBehavior + + this.resolve._genDepsOf.pop() // OK, now it's safe to return + behave.set(types, finalBehavior) + return finalBehavior } // Method called to invalidate a set of behaviors diff --git a/src/core/TypePatterns.js b/src/core/TypePatterns.js index 0af2fb0..efe05f8 100644 --- a/src/core/TypePatterns.js +++ b/src/core/TypePatterns.js @@ -20,8 +20,13 @@ class MatchTypePattern extends TypePattern { const actual = typeSequence[position] if (position < typeSequence.length) { if (actual === this.type) return [position + 1, actual] - if (options.convert && this.type.from.has(actual)) { - return [position + 1, {actual, matched: this.type}] + if (options.convert) { + for (const [pattern, convertor] of this.type.from.patterns) { + const [pos] = pattern.match([actual]) + if (pos === 1) { + return [position + 1, {actual, convertor, matched: this.type}] + } + } } } return [-1, Undefined] @@ -131,8 +136,18 @@ class MultiPattern extends TypePattern { } export const Multiple = item => new MultiPattern(item) -export const alwaysMatches = - pat => pat instanceof MultiPattern && pat.pattern instanceof AnyPattern + +// Like Multiple(Any) except leaves the argument list alone; it doesn't +// chunk it into a single Array of all arguments +class PassthruPattern extends TypePattern { + match(typeSequence, options={}) { + const position = options.position ?? 0 + return [typeSequence.length, typeSequence.slice(position)] + } + sampleTypes() {return []} +} + +export const Passthru = new PassthruPattern() // returns the template just of matched types, dropping any actual types export const matched = (template) => { diff --git a/src/core/__test__/TypeDispatcher.spec.js b/src/core/__test__/TypeDispatcher.spec.js index 3cb35e4..62a997e 100644 --- a/src/core/__test__/TypeDispatcher.spec.js +++ b/src/core/__test__/TypeDispatcher.spec.js @@ -1,7 +1,8 @@ import assert from 'assert' import {TypeDispatcher} from '../TypeDispatcher.js' -import * as numbers from '#number/all.js' +import * as booleans from '#boolean/all.js' import * as generics from '#generic/all.js' +import * as numbers from '#number/all.js' import {onType} from "#core/helpers.js" import {Any} from "#core/TypePatterns.js" import {Returns} from "#core/Type.js" @@ -47,4 +48,15 @@ describe('TypeDispatcher', () => { gnmath.merge({multiply: plain((a,b) => Math.floor(a) * b)}) assert.strictEqual(gnmath.square(-2.5), 7.5) }) + it('detects dependencies on conversion operations', () => { + const bgn = new TypeDispatcher(booleans, generics, numbers) + const {BooleanT, NumberT} = bgn.types + assert(!bgn._behaviors.negate.has([BooleanT])) + assert.strictEqual(bgn.negate(true), -1) + assert(bgn._behaviors.negate.has([BooleanT])) + const deps = bgn._dependencies.negate + bgn.merge({number: onType([BooleanT], Returns(NumberT, b => b ? 2 : 0))}) + assert(!bgn._behaviors.negate.has([BooleanT])) + assert.strictEqual(bgn.negate(true), -2) + }) }) diff --git a/src/core/__test__/TypePatterns.spec.js b/src/core/__test__/TypePatterns.spec.js index 0a081ea..5aefc3a 100644 --- a/src/core/__test__/TypePatterns.spec.js +++ b/src/core/__test__/TypePatterns.spec.js @@ -1,5 +1,7 @@ import assert from 'assert' -import {pattern, Any, Multiple, Optional} from '../TypePatterns.js' +import { + pattern, Any, Multiple, Optional, needsCollection +} from '../TypePatterns.js' import {Undefined, TypeOfTypes} from '../Type.js' describe('Type patterns', () => { @@ -93,4 +95,10 @@ describe('Type patterns', () => { whyNot.sampleTypes(), [Undefined, TypeOfTypes]) assert(whyNot.equal(whyNot)) }) + it('determines whether a template needs a collection function', () => { + assert(!needsCollection([Undefined])) + assert(needsCollection([Undefined, [Undefined, Undefined]])) + assert(needsCollection( + [Undefined, {actual: Undefined, matched: TypeOfTypes}])) + }) }) diff --git a/src/core/__test__/helpers.spec.js b/src/core/__test__/helpers.spec.js index 50969bc..c8321d8 100644 --- a/src/core/__test__/helpers.spec.js +++ b/src/core/__test__/helpers.spec.js @@ -9,8 +9,8 @@ describe('Core helpers', () => { it('defines what Implementations are', () => { const imps = onType(Undefined, 7, [TypeOfTypes, Undefined], -3) assert(imps instanceof Implementations) - assert(imps.patterns instanceof Map) - assert(imps.patterns.keys().every(k => k instanceof TypePattern)) + assert(imps.patterns instanceof Array) + assert(imps.patterns.every(([k]) => k instanceof TypePattern)) }) it('detects plain objects', () => { diff --git a/src/core/helpers.js b/src/core/helpers.js index a681c0e..ce000e5 100644 --- a/src/core/helpers.js +++ b/src/core/helpers.js @@ -2,12 +2,12 @@ import {pattern} from './TypePatterns.js' export class Implementations { constructor(imps) { - this.patterns = new Map() + this.patterns = [] this._add(imps) } _add(imps) { for (let i = 0; i < imps.length; ++i) { - this.patterns.set(pattern(imps[i]), imps[++i]) + this.patterns.push([pattern(imps[i]), imps[++i]]) } } also(...imps) { diff --git a/src/number/NumberT.js b/src/number/NumberT.js index 0a08feb..99c53ff 100644 --- a/src/number/NumberT.js +++ b/src/number/NumberT.js @@ -1,6 +1,7 @@ import {Type} from '#core/Type.js' +import {onType} from '#core/helpers.js' import {BooleanT} from '#boolean/BooleanT.js' export const NumberT = new Type(n => typeof n === 'number', { - from: [[BooleanT, math => math.number.resolve([BooleanT])]], + from: onType(BooleanT, math => math.number.resolve([BooleanT])), }) diff --git a/src/number/__test__/NumberT.spec.js b/src/number/__test__/NumberT.spec.js index 9e09131..008dcad 100644 --- a/src/number/__test__/NumberT.spec.js +++ b/src/number/__test__/NumberT.spec.js @@ -12,7 +12,14 @@ describe('NumberT Type', () => { }) it('can convert from BooleanT to NumberT', () => { - const cnvBtoN = NumberT.from.get(BooleanT)(math) + const convertImps = NumberT.from + let cnvBtoN + for (const [pattern, convFactory] of convertImps.patterns) { + if (pattern.match([BooleanT])) { + cnvBtoN = convFactory(math) + break + } + } assert.strictEqual(cnvBtoN(true), 1) assert.strictEqual(cnvBtoN(false), 0) })