From 3d1a71aa40c08ca65f19a236fc0f30c960c13e2d Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Wed, 24 Aug 2022 16:02:39 -0700 Subject: [PATCH] fix: refinements --- .../test/unitTests/test-paramGovernance.js | 2 +- packages/store/src/keys/checkKey.js | 2 +- .../store/src/patterns/patternMatchers.js | 137 +++++++----------- packages/store/src/types.js | 6 +- packages/store/test/test-patterns.js | 73 ++++++++++ 5 files changed, 133 insertions(+), 87 deletions(-) diff --git a/packages/governance/test/unitTests/test-paramGovernance.js b/packages/governance/test/unitTests/test-paramGovernance.js index 9477f3bd68fe..7a0604f2089e 100644 --- a/packages/governance/test/unitTests/test-paramGovernance.js +++ b/packages/governance/test/unitTests/test-paramGovernance.js @@ -216,7 +216,7 @@ test('multiple params bad change', async t => { ), { message: - 'In "getAmountOf" method of (Zoe Invitation issuer) arg 0: bigint "[13n]" - Must be a remotable', + 'In "getAmountOf" method of (Zoe Invitation issuer) arg 0: bigint "[13n]" - Must be a remotable (Payment)', }, ); }); diff --git a/packages/store/src/keys/checkKey.js b/packages/store/src/keys/checkKey.js index 6b822bc432e4..13661acf6d9c 100644 --- a/packages/store/src/keys/checkKey.js +++ b/packages/store/src/keys/checkKey.js @@ -161,7 +161,7 @@ export const checkCopySet = (s, check = x => x) => { X`Not a copySet: ${s}`, ) && checkElements(s.payload, check) && - checkKey(s.payload); + checkKey(s.payload, check); if (result) { copySetMemo.add(s); } diff --git a/packages/store/src/patterns/patternMatchers.js b/packages/store/src/patterns/patternMatchers.js index d1e4ab0e9b0e..20195d923ea3 100644 --- a/packages/store/src/patterns/patternMatchers.js +++ b/packages/store/src/patterns/patternMatchers.js @@ -1,3 +1,4 @@ +/* eslint-disable no-use-before-define */ // @ts-check import { @@ -24,7 +25,6 @@ import { checkScalarKey, isScalarKey, checkCopySet, - checkCopyBag, checkCopyMap, copyMapKeySet, } from '../keys/checkKey.js'; @@ -33,7 +33,6 @@ import { const { quote: q, details: X } = assert; const { entries, values } = Object; -const { ownKeys } = Reflect; /** @type WeakSet */ const patternMemo = new WeakSet(); @@ -104,35 +103,16 @@ const makePatternKit = () => { return matchHelper.checkIsMatcherPayload(patt.payload, check); } switch (tag) { - case 'copySet': { - if (!checkCopySet(patt, check)) { - return false; - } - // For a copySet to be a pattern, all its elements must be patterns. - // If all the elements are keys, then the copySet pattern is also - // a key and is already taken of. For the case where some elements - // are non-key patterns, general support is both hard and not - // currently needed. Currently, we only need a copySet of a single - // non-key pattern element. + case 'copySet': + case 'copyBag': { assert( - patt.payload.length === 1, - X`Non-singleton copySets with matcher not yet implemented: ${patt}`, + !isKey(patt), + X`internal: The key case should have been dealt with earlier: ${patt}`, + ); + return check( + false, + X`A ${q(tag)} must be a Key but was not: ${patt}`, ); - return checkPattern(patt.payload[0], check); - } - case 'copyBag': { - if (!checkCopyBag(patt, check)) { - return false; - } - // If it is a CopyBag, then it must also be a key and we - // should never get here. - if (isKey(patt)) { - assert.fail( - X`internal: The key case should have been dealt with earlier: ${patt}`, - ); - } else { - assert.fail(X`A CopyMap must be a Key but was not: ${patt}`); - } } case 'copyMap': { return ( @@ -337,37 +317,26 @@ const makePatternKit = () => { if (matchHelper) { return matchHelper.checkMatches(specimen, patt.payload, check); } - const msg = X`${specimen} - Only a ${q(pattTag)} matches a ${q( - pattTag, - )} pattern: ${patt}`; - if (specStyle !== 'tagged') { - return check(false, msg); - } - const specTag = getTag(specimen); - if (pattTag !== specTag) { - return check(false, msg); + if (specStyle !== 'tagged' || getTag(specimen) !== pattTag) { + return check( + false, + X`${specimen} - Only a ${q(pattTag)} matches a ${q( + pattTag, + )} pattern: ${patt}`, + ); } const { payload: pattPayload } = patt; const { payload: specPayload } = specimen; switch (pattTag) { - case 'copySet': { - if (!checkCopySet(specimen, check)) { - return false; - } - if (pattPayload.length !== specPayload.length) { - return check( - false, - X`copySet ${specimen} - Must have as many elements as copySet pattern: ${patt}`, - ); - } - // Should already be validated by checkPattern. But because this - // is a check that may loosen over time, we also assert everywhere - // we still rely on the restriction. + case 'copySet': + case 'copyBag': { assert( - patt.payload.length === 1, - X`Non-singleton copySets with matcher not yet implemented: ${patt}`, + !isKey(patt), + X`internal: The key case should have been dealt with earlier: ${patt}`, + ); + assert.fail( + X`internal: A ${q(pattTag)} must be a Key but was not: ${patt}`, ); - return checkMatches(specPayload[0], pattPayload[0], check, 0); } case 'copyMap': { if (!checkCopySet(specimen, check)) { @@ -568,7 +537,7 @@ const makePatternKit = () => { // /////////////////////// Match Helpers ///////////////////////////////////// /** @type {MatchHelper} */ - const matchAnyHelper = Far('M.any helper', { + const matchAnyHelper = Far('match:any helper', { checkMatches: (_specimen, _matcherPayload, _check = x => x) => true, checkIsMatcherPayload: (matcherPayload, check = x => x) => @@ -737,7 +706,7 @@ const makePatternKit = () => { }); /** @type {MatchHelper} */ - const matchRemotableHelper = Far('M.remotable helper', { + const matchRemotableHelper = Far('match:remotable helper', { checkMatches: (specimen, remotableDesc, check = x => x) => { const specimenKind = passStyleOf(specimen); if (specimenKind === 'remotable') { @@ -753,17 +722,11 @@ const makePatternKit = () => { }, checkIsMatcherPayload: (allegedRemotableDesc, check = x => x) => - check( - passStyleOf(allegedRemotableDesc) === 'copyRecord', - X`A remotableDesc must be a copyRecord: ${allegedRemotableDesc}`, - ) && - check( - typeof allegedRemotableDesc.label === 'string', - X`A remotableDesc must have a string label: ${allegedRemotableDesc}`, - ) && - check( - ownKeys(allegedRemotableDesc).length === 1, - X`Additional properties on remotableDesc not yet recognized: ${allegedRemotableDesc}`, + checkMatches( + allegedRemotableDesc, + harden({ label: M.string() }), + check, + 'match:remotable payload', ), getRankCover: (_remotableDesc, _encodePassable) => @@ -886,10 +849,12 @@ const makePatternKit = () => { ), checkIsMatcherPayload: (entryPatt, check = x => x) => - check( - passStyleOf(entryPatt) === 'copyArray' && entryPatt.length === 2, - X`${entryPatt} - Must be an pair of patterns`, - ) && checkPattern(entryPatt, check), + checkMatches( + entryPatt, + harden([M.pattern(), M.pattern()]), + check, + 'match:recordOf payload', + ), getRankCover: _entryPatt => getPassStyleCover('copyRecord'), @@ -928,10 +893,12 @@ const makePatternKit = () => { ), checkIsMatcherPayload: (entryPatt, check = x => x) => - check( - passStyleOf(entryPatt) === 'copyArray' && entryPatt.length === 2, - X`${entryPatt} - Must be an pair of patterns`, - ) && checkPattern(entryPatt, check), + checkMatches( + entryPatt, + harden([M.pattern(), M.pattern()]), + check, + 'match:bagOf payload', + ), getRankCover: () => getPassStyleCover('tagged'), @@ -954,10 +921,12 @@ const makePatternKit = () => { ), checkIsMatcherPayload: (entryPatt, check = x => x) => - check( - passStyleOf(entryPatt) === 'copyArray' && entryPatt.length === 2, - X`${entryPatt} - Must be an pair of patterns`, - ) && checkPattern(entryPatt, check), + checkMatches( + entryPatt, + harden([M.pattern(), M.pattern()]), + check, + 'match:mapOf payload', + ), getRankCover: _entryPatt => getPassStyleCover('tagged'), @@ -1149,6 +1118,11 @@ const makePatternKit = () => { const PromiseShape = makeKindMatcher('promise'); const UndefinedShape = makeKindMatcher('undefined'); + const makeRemotableMatcher = (label = undefined) => + label === undefined + ? RemotableShape + : makeMatcher('match:remotable', harden({ label })); + /** * @param {'sync'|'async'} callKind * @param {ArgGuard[]} argGuards @@ -1224,10 +1198,7 @@ const makePatternKit = () => { set: () => SetShape, bag: () => BagShape, map: () => MapShape, - remotable: (label = undefined) => - label === undefined - ? RemotableShape - : makeMatcher('match:remotable', harden({ label })), + remotable: makeRemotableMatcher, error: () => ErrorShape, promise: () => PromiseShape, undefined: () => UndefinedShape, diff --git a/packages/store/src/types.js b/packages/store/src/types.js index d801f08e7606..7cebf5f41b24 100644 --- a/packages/store/src/types.js +++ b/packages/store/src/types.js @@ -500,8 +500,10 @@ * @property {() => Matcher} set A CopySet * @property {() => Matcher} bag A CopyBag * @property {() => Matcher} map A CopyMap - * @property {(interfaceName?: string) => Matcher} remotable - * A far object or its remote presence + * @property {(label?: string) => Matcher} remotable + * A far object or its remote presence. The optional `label` is purely for + * diagnostic purpose. It does not enforce any constraint beyond the + * must-be-a-remotable constraint. * @property {() => Matcher} error * Error objects are passable, but are neither keys nor symbols. * They do not have a useful identity. diff --git a/packages/store/test/test-patterns.js b/packages/store/test/test-patterns.js index 48f98765bf26..e203cc3130a7 100644 --- a/packages/store/test/test-patterns.js +++ b/packages/store/test/test-patterns.js @@ -2,6 +2,7 @@ // eslint-disable-next-line import/no-extraneous-dependencies import { test } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js'; +import { makeTagged } from '@endo/marshal'; import { makeCopyBag, makeCopyMap, makeCopySet } from '../src/keys/checkKey.js'; import { fit, matches, M } from '../src/patterns/patternMatchers.js'; import '../src/types.js'; @@ -263,6 +264,71 @@ const matchTests = harden([ ], ], }, + { + specimen: makeTagged('mysteryTag', 88), + yesPatterns: [M.any(), M.not(M.pattern())], + noPatterns: [ + [ + M.pattern(), + 'A passable tagged "mysteryTag" is not a pattern: "[mysteryTag]"', + ], + ], + }, + { + specimen: makeTagged('match:any', undefined), + yesPatterns: [M.any(), M.pattern()], + noPatterns: [ + [M.key(), 'A passable tagged "match:any" is not a key: "[match:any]"'], + ], + }, + { + specimen: makeTagged('match:any', 88), + yesPatterns: [M.any(), M.not(M.pattern())], + noPatterns: [[M.pattern(), 'Payload must be undefined: 88']], + }, + { + specimen: makeTagged('match:remotable', 88), + yesPatterns: [M.any(), M.not(M.pattern())], + noPatterns: [ + [ + M.pattern(), + 'match:remotable payload: 88 - Must be a copyRecord to match a copyRecord pattern: {"label":"[match:kind]"}', + ], + ], + }, + { + specimen: makeTagged('match:remotable', harden({ label: 88 })), + yesPatterns: [M.any(), M.not(M.pattern())], + noPatterns: [ + [ + M.pattern(), + 'match:remotable payload: label: number 88 - Must be a string', + ], + ], + }, + { + specimen: makeTagged('match:recordOf', harden([M.string(), M.nat()])), + yesPatterns: [M.pattern()], + noPatterns: [ + [ + M.key(), + 'A passable tagged "match:recordOf" is not a key: "[match:recordOf]"', + ], + ], + }, + { + specimen: makeTagged( + 'match:recordOf', + harden([M.string(), Promise.resolve(null)]), + ), + yesPatterns: [M.any(), M.not(M.pattern())], + noPatterns: [ + [ + M.pattern(), + 'match:recordOf payload: [1]: A "promise" cannot be a pattern', + ], + ], + }, ]); test('test simple matches', t => { @@ -281,3 +347,10 @@ test('test simple matches', t => { } } }); + +test('well formed patterns', t => { + // @ts-expect-error purposeful type violation for testing + t.throws(() => M.remotable(88), { + message: 'match:remotable payload: label: number 88 - Must be a string', + }); +});