-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: checked cast with TypedMatcher #8394
Changes from all commits
4a80f4b
8852a06
5b9fedf
ed14400
685ffac
0c4f75e
20e1779
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,6 @@ | |
"access": "public" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 91.22 | ||
"atLeast": 91.23 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,6 @@ | |
"access": "public" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 75.02 | ||
"atLeast": 75.1 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,6 +99,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 76.99 | ||
"atLeast": 77.3 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 77.32 | ||
"atLeast": 76.95 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 91.11 | ||
"atLeast": 91.4 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 87.28 | ||
"atLeast": 86.66 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 74.36 | ||
"atLeast": 76.03 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 88.94 | ||
"atLeast": 88.92 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,6 @@ | |
"timeout": "20m" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 80.49 | ||
"atLeast": 80.6 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,6 @@ | |
"access": "public" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 81.63 | ||
"atLeast": 82.44 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,6 @@ | |
"access": "public" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 89.31 | ||
"atLeast": 89.35 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// @ts-check | ||
import { mustMatch as typelessMustMatch } from '@endo/patterns'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we eventually move this into @endo/patterns so the only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think once we've had some stress testing of it so we know it won't change much. |
||
|
||
/** | ||
* @import {MustMatch, PatternType, TypedPattern} from './types.js'; | ||
*/ | ||
|
||
/** @type {MustMatch} */ | ||
export const mustMatch = typelessMustMatch; | ||
|
||
/** | ||
* @template M | ||
* @param {unknown} specimen | ||
* @param {TypedPattern<M>} patt | ||
* @returns {M} | ||
*/ | ||
export const cast = (specimen, patt) => { | ||
// mustMatch throws if they don't, which means that `cast` also narrows the | ||
// type but a function can't both narrow and return a type. That is by design: | ||
// https://github.com/microsoft/TypeScript/issues/34636#issuecomment-545025916 | ||
mustMatch(specimen, patt); | ||
return specimen; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,3 +64,33 @@ export type Remote<Primary, Local = DataOnly<Primary>> = | |
export type FarRef<Primary, Local = DataOnly<Primary>> = ERef< | ||
Remote<Primary, Local> | ||
>; | ||
|
||
/* | ||
* Stop-gap until https://github.com/Agoric/agoric-sdk/issues/6160 | ||
* explictly specify the type that the Pattern will verify through a match. | ||
* | ||
* TODO move all this pattern typing stuff to @endo/patterns | ||
*/ | ||
declare const validatedType: unique symbol; | ||
/** | ||
* Tag a pattern with the static type it represents. | ||
*/ | ||
export type TypedPattern<T> = Pattern & { [validatedType]?: T }; | ||
|
||
export declare type PatternType<TM extends TypedPattern<any>> = | ||
TM extends TypedPattern<infer T> ? T : never; | ||
|
||
// TODO make Endo's mustMatch do this | ||
/** | ||
* Returning normally indicates success. Match failure is indicated by | ||
* throwing. | ||
* | ||
* Note: remotables can only be matched as "remotable", not the specific kind. | ||
* | ||
* @see {import('@endo/patterns').mustMatch} for the implementation. This one has a type annotation to narrow if the pattern is a TypedPattern. | ||
*/ | ||
export declare type MustMatch = <P extends Pattern>( | ||
specimen: unknown, | ||
pattern: P, | ||
label?: string | number, | ||
) => asserts specimen is P extends TypedPattern<any> ? PatternType<P> : unknown; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I understand it, OMG this is cool! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this eventually migrate into endo? Into the @endo/patterns package? I hope so. If you agree, would be good to comment on that expected migration here, or somewhere relevant. Similarly, should #6160 have been an endo issue rather than an agoric-sdk issue, with the eventual fix also eventually being in endo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I expect it'll happen when there's some trigger.
Could have been. Though it's much faster to being useful by making it work in agoric-sdk and moving it to Endo when it's stable. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// @ts-check | ||
import test from 'ava'; | ||
|
||
import { makeExo } from '@endo/exo'; | ||
import { M } from '@endo/patterns'; | ||
import { cast, mustMatch } from '../src/typeCheck.js'; | ||
|
||
/** | ||
* @import {TypedPattern} from '@agoric/internal'; | ||
* @import {RemotableObject} from '@endo/marshal'; | ||
*/ | ||
|
||
const Mstring = /** @type {TypedPattern<string>} */ (M.string()); | ||
const MremotableFoo = /** @type {TypedPattern<RemotableObject<'Foo'>>} */ ( | ||
M.remotable('Foo') | ||
); | ||
const MremotableBar = /** @type {TypedPattern<RemotableObject<'Bar'>>} */ ( | ||
M.remotable('Bar') | ||
); | ||
|
||
const unknownString = /** @type {unknown} */ (''); | ||
|
||
test('cast', t => { | ||
// @ts-expect-error unknown type | ||
unknownString.length; | ||
// @ts-expect-error not any | ||
cast(unknownString, Mstring).missing; | ||
cast(unknownString, Mstring).length; | ||
t.pass(); | ||
}); | ||
|
||
test('mustMatch', t => { | ||
// @ts-expect-error unknown type | ||
unknownString.length; | ||
mustMatch(unknownString, Mstring); | ||
unknownString.length; | ||
t.pass(); | ||
}); | ||
|
||
test('remotable', t => { | ||
const maybeFoo = makeExo(`Remotable1`, undefined, {}); | ||
mustMatch(maybeFoo, MremotableFoo); | ||
maybeFoo; // narrowed to Foo | ||
|
||
const maybeBar = makeExo(`Remotable2`, undefined, {}); | ||
mustMatch(maybeBar, MremotableBar); | ||
maybeBar; // narrowed to Bar | ||
|
||
mustMatch(maybeFoo, MremotableBar); | ||
maybeFoo; // further narrowed to never | ||
mustMatch(maybeBar, MremotableFoo); | ||
maybeBar; // further narrowed to never | ||
|
||
t.pass(); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 89.7 | ||
"atLeast": 90.69 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, which I've written code like this template declaration, I revise it to
since the supertype constraint is usually also a fine default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case there's no way the to omit the parameter because it's positional