-
Notifications
You must be signed in to change notification settings - Fork 72
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
1488 Passable (REVERTED in #1961) #1933
Conversation
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.
is a recursive definition of Passable
feasible?
a6c2693
to
75605a1
Compare
packages/pass-style/src/types.js
Outdated
* (p: Iterable): 'remotable'; | ||
* (p: Iterator): 'remotable'; |
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.
I don't understand why Iterable
and Iterator
are here. I wonder what I'm missing.
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.
hacks along the way. I'll see about removing them
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.
I didn't know how else to handle these cases
endo/packages/patterns/test/test-copyMap.js
Lines 131 to 133 in 3d927c3
t.is(passStyleOf(entriesIterable), 'remotable'); | |
const entriesIterator = entriesIterable[Symbol.iterator](); | |
t.is(passStyleOf(entriesIterator), 'remotable'); |
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.
oh. wild. I didn't realize getCopyMapEntries()
returned a passable value.
I did know that tsc
thinks getCopyMapEntries()
returns any
before this PR.
https://github.com/Agoric/dapp-offer-up/blob/8da2191af4a3333e0ec0f0cd884f739fa021950a/contract/src/gameAssetContract.js#L21
packages/pass-style/src/types.js
Outdated
* @typedef {Passable} RemotableObject | ||
* @template {string} S pass style | ||
* @template {InterfaceSpec} I interface tag | ||
* @typedef {PassStyled<S> & {[Symbol.toStringTag]: I}} TaggedRecord |
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.
This use of "Tagged" is different from the tagged
passStyle, right? That seems like a source of confusion.
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.
Different, yes. This is as in "toStringTag". Naming suggestions welcomed
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.
I see.... 🤔
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.
"PassStyled" seems like a good name... do we need both types, or can they be collapsed together?
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.
I wasn't sure. I thought I saw an object with the [PASS_STYLE]
property and not the toStringTag
or vice-versa. Are they supposed to be always present together?
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.
Are they supposed to be always present together?
Yes, AFAIK. A Tagged has own [PASS_STYLE]: "tagged", [Symbol.toStringTag]: $tag
, and a Remotable has a prototype chain in which the penultimate object has own [PASS_STYLE]: "remotable", [Symbol.toStringTag]: $iface
(where both $tag
and $iface
must be strings, and the latter must either be "Remotable" or start with "Alleged: " or "DebugName: ").
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.
Thanks! 1b30d60
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.
This is very nice!!!
@@ -81,28 +81,28 @@ const makePassStyleOf = passStyleHelpers => { | |||
* structures, so without this cache, these algorithms could be | |||
* O(N**2) or worse. | |||
* | |||
* @type {WeakMap<Passable, PassStyle>} | |||
* @type {WeakMap<WeakKey, PassStyle>} |
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.
Do we have a TypeScript type corresponding with our Key concept (Passable structure in which each leaf is a Passable primitive value or a Remotable)?
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.
That reminds me: Key is another motivation for parameterizing Passable: Passable<Remotable, never>
PureData is Passable<never, never>
as discussed earlier
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if` | ||
assert(passStyleMemo.has(inner)); |
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.
What value is this sanity check providing, given that get
is guaranteed to return undefined for absent keys?
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if` | |
assert(passStyleMemo.has(inner)); |
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.
Indeed. I flubbed the implementation but the case I was concerned about is if passStyleMemo
has inner
but that value is falsy. Does this need to handle that?
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.
I don't think so, because passStyleMemo
values are always PassStyle strings. And if we do care, I guess the place for such an assertion would be adjacent to set
rather than get
.
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.
Unfortunately, simpy knowledge that it is a string does not by itself imply that it is truthy, since the empty string is falsy. In this case, we know that it is always a pass-style string, which is always a non-empty string and thus always truthy. But still, to avoid even needing to worry about this edge case, I suggest rewriting
if (inner) {
to
if (inner !== undefined) {
or
if (typeof inner === 'string') {
FWIW, I prefer the !== undefined
.
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 any case, given that we statically know we're only putting strings in there, I'd say no further sanity check is needed.
packages/pass-style/src/remotable.js
Outdated
/** | ||
* Simple semantics, just tell what interface (or undefined) a remotable has. | ||
* @type {{ | ||
* <T extends string>(val: import('./types.js').TaggedRecord<any, T>): T; | ||
* (val: any): any; | ||
* }} |
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.
Cool! Can we narrow further?
/** | |
* Simple semantics, just tell what interface (or undefined) a remotable has. | |
* @type {{ | |
* <T extends string>(val: import('./types.js').TaggedRecord<any, T>): T; | |
* (val: any): any; | |
* }} | |
/** | |
* Simple semantics, just tell what interface (or undefined) a remotable has. | |
* @type {{ | |
* <T extends string>(val: import('./types.js').TaggedRecord<any, T>): T; | |
* (val: any): string | undefined; | |
* }} |
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.
Since remotable tags are only for debugging, I don't think we should use them for static typing.
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.
Can we narrow further?
I had tried that and it caused some problems. I'll document.
Works! thx
Since remotable tags are only for debugging, I don't think we should use them for static typing.
That's what this doc says,
endo/packages/patterns/src/types.js
Lines 310 to 313 in 40f6cf8
* @property {(label?: string) => Matcher} remotable | |
* Matches a far object or its remote presence. | |
* The optional `label` is purely for diagnostic purposes and does not | |
* add any constraints. |
but when we changed the label it failed rehydration,
https://github.com/Agoric/agoric-sdk/blob/62a7c9c42007020994f6036cf4090590e4811b8e/packages/zoe/src/zoeService/startInstance.js#L65
Is that a bug?
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.
It's an unpleasant surprise, at least.
I suppose the tag is pass-invariant, so using it for static typing is maybe not all that bad.
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.
when we changed the label it failed rehydration, https://github.com/Agoric/agoric-sdk/blob/62a7c9c42007020994f6036cf4090590e4811b8e/packages/zoe/src/zoeService/startInstance.js#L65
Is that a bug?
Plausibly, although not related to this PR. The fundamental issue is that agoric-sdk doesn't allow a new vat incarnation to change stateShape
, and adding/removing/altering the label of a Matcher<'remotable'> is a change (albeit a benign one from internal structure like { [Symbol.for("passStyle")]: "tagged", [Symbol.toStringTag]: "match:kind", payload: "remotable" }
to { [Symbol.for("passStyle")]: "tagged", [Symbol.toStringTag]: "match:remotable", payload: { label: "$label" } }
). This will need to be allowed in the fix for Agoric/agoric-sdk#7337 / Agoric/agoric-sdk#7407 .
packages/pass-style/src/types.js
Outdated
* @typedef {Passable} RemotableObject | ||
* @template {string} S pass style | ||
* @template {InterfaceSpec} I interface tag | ||
* @typedef {PassStyled<S> & {[Symbol.toStringTag]: I}} TaggedRecord |
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.
"PassStyled" seems like a good name... do we need both types, or can they be collapsed together?
packages/patterns/src/types.js
Outdated
@@ -22,7 +22,7 @@ export {}; | |||
/** @typedef {import('@endo/marshal').RankCover} RankCover */ | |||
|
|||
/** | |||
* @typedef {Passable} Key | |||
* @typedef {Exclude<Passable, Error | Promise>} Key |
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.
This is still overly broad; we should probably have a TODO.
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.
Exclude at the top level doesn't address promises and errors nested further down. Again, I hope we can parameterize Passable.
@@ -61,7 +61,7 @@ export {}; | |||
*/ | |||
|
|||
/** | |||
* @typedef {Passable} Pattern | |||
* @typedef {Exclude<Passable, Error | Promise>} Pattern |
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.
Likewise here.
I am only following from a distance, but I believe you can have circular type definitions in |
I'm not sure about that but I do know the solution is more complicated than changing to .ts. Even with .ts,
causes
|
Attn @michaelfig re #1933 (comment) circular types mysteries |
#1952 shows how to address the circular type issues, I think. I discussed it with @turadg today. It's not clear how much work it would be to finish that direction. It's based on earlier ocapn work, but at the time I learned tsc's limitations around self-referential types, I neglected to take good notes. @mhofman gave a particularly useful pointer earlier today: |
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.
Very enthusiastic to see this PR be merged!
Looks Good To Me so far, but review not yet completed.
@@ -117,7 +117,7 @@ export const makeEncodeToCapData = (encodeOptions = {}) => { | |||
* Readers must not care about this order anyway. We impose this requirement | |||
* mainly to reduce non-determinism exposed outside a vat. | |||
* | |||
* @param {Passable} passable | |||
* @param {any} passable |
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.
Curious: Why did this "regress" from Passable
to any
?
(scare quotes because the old Passable
meant any
. So the real question is: Why couldn't this become the new Passable
?)
Likewise for similar changes elsewhere.
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.
All the string replacements of old-Passable to any
is because the new (non-any) Passable caused too many errors and I judged solving them to not be worth it. Usually because the following code relied heavily on dynamic type inspection and would not benefit from the static type information. This was mostly in switch
statements where TS cannot statically infer the type narrowing being used.
@@ -269,11 +269,11 @@ harden(makeEncodeToCapData); | |||
* @property {( | |||
* encodedRemotable: Encoding, | |||
* decodeRecur: (e: Encoding) => Passable | |||
* ) => (Promise|Remotable)} [decodeRemotableFromCapData] | |||
* ) => (Promise|RemotableObject)} [decodeRemotableFromCapData] |
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.
Thanks for doing this type renaming. Having a type name collide with a value (function) name was really terrible. Good to see this straightened out. No change suggested.
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if` | ||
assert(passStyleMemo.has(inner)); |
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.
Unfortunately, simpy knowledge that it is a string does not by itself imply that it is truthy, since the empty string is falsy. In this case, we know that it is always a pass-style string, which is always a non-empty string and thus always truthy. But still, to avoid even needing to worry about this edge case, I suggest rewriting
if (inner) {
to
if (inner !== undefined) {
or
if (typeof inner === 'string') {
FWIW, I prefer the !== undefined
.
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if` | ||
assert(passStyleMemo.has(inner)); |
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 any case, given that we statically know we're only putting strings in there, I'd say no further sanity check is needed.
const keyMemo = new WeakSet(); | ||
|
||
/** | ||
* @param {Passable} val | ||
* @param {Key} val |
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.
Excellent!
No change suggested.
// @ts-nocheck So many errors that the suppressions hamper readability. | ||
// TODO parameterize MatchHelper which will solve most of them |
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.
Interesting.
No change suggested.
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.
LGTM!!
packages/pass-style/src/types.d.ts
Outdated
}; | ||
export type ExtractStyle<P extends PassStyled<any>> = P[typeof PASS_STYLE]; | ||
export type PassByCopy = | ||
| import('type-fest').Primitive |
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.
TIL 'type-fest'
. Does everyone else already know about this? Is it worth a comment or URL?
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.
I figure it's intuitive that it's an NPM package and those are easy to find. But I have to clean up the commits anyway so I'll include this link https://github.com/sindresorhus/type-fest/blob/main/source/primitive.d.ts
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.
This is a weird dependency to take on for non test types. It's simple enough, can we just inline it?
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.
Ok, I'll inline it.
packages/pass-style/src/types.d.ts
Outdated
| any[] | ||
| CopyRecord |
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.
Curious why we do use CopyRecord
rather than Record
but we use any[]
rather than CopyArray
?
Also, shouldn't this at least be Passable[]
, or does this encounter recursive type problems again?
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.
changed to CopyArray
export type PassByRef = | ||
| RemotableObject | ||
| Promise<RemotableObject> | ||
| Promise<PassByCopy>; |
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.
I was going to suggest
export type PassByRef = | |
| RemotableObject | |
| Promise<RemotableObject> | |
| Promise<PassByCopy>; | |
export type PassByRef = | |
| RemotableObject | |
| Promise<Passable>; |
but then I realized that your's correctly excludes Promise<Promise>
, which is great! No change suggested.
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.
You could likely do Promise<Awaited<Passable>>
export type Passable< | ||
PC extends PassableCap = PassableCap, | ||
E extends Error = Error, |
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.
Took me awhile to understand what's going on here. This is awesome! No change suggested.
export type PassStyleOf = { | ||
(p: undefined): 'undefined'; |
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.
I don't understand the type notation here. What does it mean?
What especially confuses me is { ... : ...
suggests a record type where the thing to the left of the colon is a property key. But that does not look consistent with the rest. From the rest, I'd guess that it is a type overload, but overloaded on what? Is p
a parameter?
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.
Yep, it defines overloads for the function signature. If the call matches a function that has one parameter, of type undefined
, then the return type is the string literal 'undefined'
. Each case defines another signature. They're matched in order so the most general is last.
packages/pass-style/src/types.d.ts
Outdated
(p: any[]): 'copyArray'; | ||
(p: Iterable<any>): 'remotable'; | ||
(p: Iterator<any, any, undefined>): 'remotable'; | ||
<T extends PassStyled<any>>(p: T): ExtractStyle<T>; |
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.
Is this too open ended? Does it loose the static knowledge that the extracted style can only be one of a small number of enumerated strings?
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.
tightened up with,
export type TaggedOrRemotable = 'tagged' | 'remotable';
* trip (as exists between vats) to produce data structures disconnected from | ||
* any potential proxies. | ||
*/ | ||
export type PureData = Passable<never, never>; |
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.
Awesome! No change suggested.
packages/pass-style/src/types.d.ts
Outdated
* This is an interface specification. | ||
* For now, it is just a string, but will eventually be `PureData`. Either |
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.
No change suggested.
But noting that I doubt we ever would change this from a string. But the time we'd want to, it will be too great a compat hazard to bother with. Unless we encounter a compelling need, which I do not expect.
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.
I'll revise the text to something less certain than "will eventually be"
* | ||
* See the various uses for good examples. | ||
*/ | ||
export type Checker = ( |
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.
No change suggested.
Just FYI as of #1935 I expect I'll migrate Checker
to a lower level utils package. But there's no reason for that to affect this PR, which will merge first.
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.
Did a quick pass. Lots of great changes. I'm a little surprised some type definitions drop down to not plumbing through the PassableCap restriction, and I would need to look closer where that loosening would have an impact, but we can always iterate on that later
packages/pass-style/src/types.d.ts
Outdated
| 'remotable' | ||
| 'error' | ||
| 'promise'; | ||
export type PassStyled<S extends string> = { |
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.
Why not extends PassStyle
?
export type PassByRef = | ||
| RemotableObject | ||
| Promise<RemotableObject> | ||
| Promise<PassByCopy>; |
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.
You could likely do Promise<Awaited<Passable>>
chore(patterns): define Key using parameterized Passable
tighten Pattern better RemotableObject WIP punt getInterfaceOf WIP fix sorting restore PassableCap fix valMap type runtime fix decodeErrorCommon acknowledge deferred work fixes to get types building
@turadg @kriskowal all, Now that the endo-agoric-sdk sync is behind us, I would love to see this PR move forward again! What is needed? At Agoric/agoric-sdk#8771 I have a PR of postponed changes on the agoric-sdk side that were waiting on that sync. Would that one help or hurt the effort to move forward on this PR? |
In Endo, a PR re-applying these changes. And in agoric-sdk a PR that integrates with that PR, demonstrating agoric-sdk CI passing. That way when land the Endo PR we can follow up quickly with a release and make the SDK PR the basis of an Endo upgrade. I've started in
I'm not sure. I think 8771 should be prioritized, though, because it doesn't require an Endo release. I'll try to get 8774 passing and I expect by the time it's R4R that master will have 8771 so I'll solve whatever else comes up. |
closes: #1488 ## Description Reattempting: - #1933 Again: - Makes `Passable` a generic type instead of `any`. - Defines overloads for `passStyleOf` to return the actual style. - Defines the `Key` in terms of `Passable` - Makes a ton of fixes and suppressions in places that relied on the previous `any`'s ### Security Considerations n/a ### Scaling Considerations n/a ### Documentation Considerations Better types are better documentation. ### Testing Considerations - [ ] Agoric/agoric-sdk#8774 ### Upgrade Considerations These changes may cause type errors downstream. I don't think we should call those breaking change à la semver because they don't affect the runtime.
closes: #XXXX refs: #1933 ## Description The declaration site had both a correct @type declaration and a redundant vestigial partial @returns declaration. Because the latter also was not properly formed, it caused the warning ``` .../endojs/endo/packages/pass-style/src/remotable.js 175:1 warning Missing JSDoc @returns type jsdoc/require-returns-type ✖ 1 problem (0 errors, 1 warning) ``` This PR removes it, and does a bit of trivial polishing of the remainder. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations more accurate type declarations should eventually help documentation ### Testing Considerations none ### Upgrade Considerations none
closes: #1488
Description
Passable
a generic type instead ofany
.passStyleOf
to return the actual style.Key
in terms ofPassable
any
'sSecurity Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
Better types are better documentation.
Testing Considerations
I improved the test coverage. Additional suggestions welcomed.
Upgrade Considerations
These changes may cause type errors downstream. I don't think we should call those breaking change à la semver because they don't affect the runtime.