Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Promise for non-Passable is not Passable #2421

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Promise for non-Passable is not Passable #2421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f7c079
feat(types): Promise for unpassable is unpassable
turadg Aug 20, 2024
38a1cf6
refactor: simplify deeplyFulfilled
turadg Aug 20, 2024
017b2c8
types(pass-style): narrow Passable definition
mhofman Aug 21, 2024
4bff8f8
types(pass-style): More tweaks
mhofman Aug 22, 2024
db8a4da
types: even more tweaks
mhofman Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/captp/src/captp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ export const makeCapTP = (
epoch,
questionID,
target,
// @ts-expect-error Type 'unknown' is not assignable to type 'Passable<PassableCap, Error>'.
// @ts-expect-error Type 'unknown[]' is not assignable to type 'Passable'.
method: serialize(harden([null, args])),
});
return promise;
Expand All @@ -430,7 +430,7 @@ export const makeCapTP = (
epoch,
questionID,
target,
// @ts-expect-error Type 'unknown' is not assignable to type 'Passable<PassableCap, Error>'.
// @ts-expect-error Type 'unknown[]' is not assignable to type 'Passable'.
method: serialize(harden([prop, args])),
});
return promise;
Expand Down
1 change: 1 addition & 0 deletions packages/eventual-send/src/exports.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { HandledPromiseConstructor } from './types.d.ts';
//

export type {
Callable,
RemotableBrand,
DataOnly,
FarRef,
Expand Down
39 changes: 12 additions & 27 deletions packages/pass-style/src/deeplyFulfilled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ import { passStyleOf } from './passStyleOf.js';
import { makeTagged } from './makeTagged.js';

/**
* @import {Passable, Primitive, CopyRecord, CopyArray, CopyTagged, RemotableObject} from '@endo/pass-style'
* @import {Passable, CopyRecord, CopyArray, CopyTagged, RemotableObject} from '@endo/pass-style'
*/
/** @import {Callable, RemotableBrand} from '@endo/eventual-send' */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I'lll just mention that I'm a bit surprised that RemotableBrand is defined by @endo/eventual-send but RemotableObject is defined by @endo/pass-style. No change suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah they're very different, somewhat orthogonal things. I've been discussing with @michaelfig, and from eventual-send's pov, the target or arguments don't require any specific pass-style. RemotableBrand helps the typing to capture the shape of remote methods (and the shape of local data) without exposing any actual property on the type itself (its storing them in the private fields of a "stamped" class).

RemotableObject is the pass-style brand, which includes the 2 symbol properties (PASS_STYLE and Symbol.toStringTag), denoting a remotable in passables.


const { ownKeys } = Reflect;
const { fromEntries } = Object;
Expand All @@ -23,32 +24,16 @@ const { fromEntries } = Object;
*/

/**
* Currently copied from @agoric/internal utils.js.
* TODO Should migrate here and then, if needed, reexported there.
*
* @typedef {(...args: any[]) => any} Callable
*/

/**
* Currently copied from @agoric/internal utils.js.
* TODO Should migrate here and then, if needed, reexported there.
*
* @template {{}} T
* @typedef {{
* [K in keyof T]: T[K] extends Callable ? T[K] : DeeplyAwaited<T[K]>;
* }} DeeplyAwaitedObject
*/

/**
* Currently copied from @agoric/internal utils.js.
* TODO Should migrate here and then, if needed, reexported there.
*
* @template T
* @typedef {T extends PromiseLike<any>
* ? Awaited<T>
* : T extends {}
* ? Simplify<DeeplyAwaitedObject<T>>
* : Awaited<T>} DeeplyAwaited
* @typedef {boolean extends (T extends never ? true : false)
* ? T // pass through the `any` type
* : T extends PromiseLike<any>
* ? DeeplyAwaited<Awaited<T>>
* : T extends object
* ? T extends (Callable | RemotableBrand<any, any> | RemotableObject)
* ? T
* : Simplify<{[K in keyof T]: DeeplyAwaited<T[K]>}>
* : T} DeeplyAwaited
*/

/**
Expand All @@ -73,7 +58,7 @@ const { fromEntries } = Object;
* is for the higher "@endo/patterns" level of abstraction to determine,
* because it defines the `Key` notion in question.
*
* @template {Passable} [T=Passable]
* @template {Passable} T
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to remove the default?

In general when I write a type-constrained type parameter, I also provide that type constraint as a default binding of the type parameter. Should I stop doing so?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For generic functions we want to rely on inference. I don't remember exactly why I dropped it, but I think a default doesn't have any advantages here, especially one where the default value is the constraint itself.

For type definitions it's a different story as it allows you to reference the bare type without type parameters.

* @param {T} val
* @returns {Promise<DeeplyAwaited<T>>}
*/
Expand Down
76 changes: 76 additions & 0 deletions packages/pass-style/src/deeplyFulfilled.test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/* eslint-disable */
import { expectType } from 'tsd';
import { Far } from './make-far';
import { makeTagged } from './makeTagged';
import { deeplyFulfilled } from './deeplyFulfilled';
import type { CopyTagged, Passable, RemotableObject } from './types';

const remotable = Far('foo', {
myFunc() {
return 'foo';
},
});

type MyRemotable = typeof remotable;
type MyNonBrandedRemotable = { myFunc(): 'foo' } & RemotableObject;

const copyTagged = makeTagged('someTag', remotable);

const someUnknown: unknown = null;
const someAny: any = null;

expectType<undefined>(await deeplyFulfilled(undefined));
expectType<string>(await deeplyFulfilled('str'));
expectType<boolean>(await deeplyFulfilled(true));
expectType<number>(await deeplyFulfilled(1));
expectType<bigint>(await deeplyFulfilled(1n));
expectType<symbol>(await deeplyFulfilled(Symbol.for('foo')));
expectType<null>(await deeplyFulfilled(null));
expectType<Error>(await deeplyFulfilled(new Error()));
expectType<MyRemotable>(await deeplyFulfilled(remotable));
expectType<MyNonBrandedRemotable>(
await deeplyFulfilled(remotable as MyNonBrandedRemotable),
);
expectType<CopyTagged<'someTag', MyRemotable>>(
await deeplyFulfilled(copyTagged),
);
expectType<[]>(await deeplyFulfilled([]));
expectType<{}>(await deeplyFulfilled({}));

expectType<MyRemotable>(await deeplyFulfilled(Promise.resolve(remotable)));
expectType<{ a: MyRemotable }>(
await deeplyFulfilled(Promise.resolve({ a: Promise.resolve(remotable) })),
);

// By default TS infer an array type instead of a tuple type
const tuple: [Promise<MyRemotable>] = [Promise.resolve(remotable)];

expectType<Passable>(tuple);
expectType<[MyRemotable]>(await deeplyFulfilled(tuple));
expectType<[MyRemotable]>(await deeplyFulfilled(Promise.resolve(tuple)));

expectType<CopyTagged<'someOtherTag', MyRemotable>>(
await deeplyFulfilled(
Promise.resolve(makeTagged('someOtherTag', Promise.resolve(remotable))),
),
);

// @ts-expect-error not passable
deeplyFulfilled(someUnknown);
// @ts-expect-error not passable
deeplyFulfilled(Promise.resolve(someUnknown));
// @ts-expect-error not passable
deeplyFulfilled(Promise.resolve({ a: Promise.resolve(someUnknown) }));
// @ts-expect-error not passable
deeplyFulfilled(Promise.resolve([Promise.resolve(someUnknown)]));

expectType<any>(await deeplyFulfilled(someAny));
expectType<any>(await deeplyFulfilled(Promise.resolve(someAny)));
expectType<{ a: any }>(
await deeplyFulfilled(Promise.resolve({ a: Promise.resolve(someAny) })),
);
expectType<[any]>(
await deeplyFulfilled(
Promise.resolve([Promise.resolve(someAny)] as [Promise<any>]),
),
);
4 changes: 2 additions & 2 deletions packages/pass-style/src/passStyleOf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ harden(toPassableError);
* the results of coercing those errors to passable errors.
*
* @param {unknown} specimen
* @returns {Passable<never, Error>}
* @returns {Passable<never, Error, false>}
*/
export const toThrowable = specimen => {
harden(specimen);
Expand Down Expand Up @@ -378,6 +378,6 @@ export const toThrowable = specimen => {
}
}
}
return /** @type {Passable<never,never>} */ (specimen);
return /** @type {Passable<never, never, false>} */ (specimen);
};
harden(toThrowable);
76 changes: 57 additions & 19 deletions packages/pass-style/src/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ export type PassByCopy =

export type PassByRef =
| RemotableObject
| Promise<RemotableObject>
| Promise<PassByCopy>;
| PromiseLike<RemotableObject>
| PromiseLike<PassByCopy>;

/**
* A Passable is acyclic data that can be marshalled. It must be hardened to
Expand All @@ -80,20 +80,54 @@ export type PassByRef =
* using 'slots').
*/
export type Passable<
PC extends PassableCap = PassableCap,
R extends RemotableObject = RemotableObject,
E extends Error = Error,
> = void | Primitive | Container<PC, E> | PC | E;

export type Container<PC extends PassableCap, E extends Error> =
| CopyArrayI<PC, E>
| CopyRecordI<PC, E>
| CopyTaggedI<PC, E>;
interface CopyArrayI<PC extends PassableCap, E extends Error>
extends CopyArray<Passable<PC, E>> {}
interface CopyRecordI<PC extends PassableCap, E extends Error>
extends CopyRecord<Passable<PC, E>> {}
interface CopyTaggedI<PC extends PassableCap, E extends Error>
extends CopyTagged<string, Passable<PC, E>> {}
AllowPromise extends boolean = any,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to both type-constrain the type variable AllowPromise to boolean but give it a default of any?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you didn't adopt the convention of the previous two type variables and write

Suggested change
AllowPromise extends boolean = any,
P extends Promise = Promise,

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, given the other changes

Suggested change
AllowPromise extends boolean = any,
P extends PromiseLike = PromiseLike,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of a hack in the type system. I need to conditionally include a PromiseLike in the union, or a never. But the PromiseLike needs to be parametrized with a specific constraint. As such this type parameter cannot be provided as a type (or inferred), but instead is just a flag to say "please allow promises in the union".

The any default is also a hack to make TS bail out when trying to type match. Without it we wouldn't be able to assign a Passable without promises (AllowPromise=false) to ones with it (or really places where we don't care). I don't fully understand why, this is where I ended at experimentally.

AllowTopLevelPromise extends boolean = AllowPromise,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you make use of this fourth parameter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally to avoid including PromiseLike in the union of acceptable types inside the PromiseLike when recursing the type.

> =
| void
| Primitive
| Container<R, E, AllowPromise>
| R
| E
| (true extends AllowTopLevelPromise
? PromiseLike<Passable<R, E, AllowPromise, false>>
: never);

export type Container<
R extends RemotableObject,
E extends Error,
AllowPromise extends boolean = any,
> =
| CopyArrayI<Passable<R, E, AllowPromise>>
| CopyRecordI<Passable<R, E, AllowPromise>>
| CopyTaggedI<Passable<R, E, AllowPromise>>;
interface CopyArrayI<T extends Passable = any> extends CopyArray<T> {}
interface CopyRecordI<T extends Passable = any> extends CopyRecord<T> {}
interface CopyTaggedI<T extends Passable = any> extends CopyTagged<string, T> {}

export type PassableSubset<
T extends Passable<RemotableObject, Error, false> = Passable<
RemotableObject,
Error,
false
>,
AllowPromise extends boolean = false,
AllowTopLevelPromise extends boolean = AllowPromise,
> =
| T
| SubsetContainer<T, AllowPromise>
| (true extends AllowTopLevelPromise
? PromiseLike<PassableSubset<T, AllowPromise, false>>
: never);

type SubsetContainer<
T extends Passable<RemotableObject, Error, false>,
AllowPromise extends boolean = false,
> =
| CopyArrayI<PassableSubset<T, AllowPromise>>
| CopyRecordI<PassableSubset<T, AllowPromise>>
| CopyTaggedI<PassableSubset<T, AllowPromise>>;

export type PassStyleOf = {
(p: undefined): 'undefined';
Expand All @@ -103,15 +137,15 @@ export type PassStyleOf = {
(p: bigint): 'bigint';
(p: symbol): 'symbol';
(p: null): 'null';
(p: Promise<any>): 'promise';
(p: PromiseLike<any>): 'promise';
(p: Error): 'error';
(p: CopyTagged): 'tagged';
(p: any[]): 'copyArray';
(p: Iterable<any>): 'remotable';
(p: Iterator<any, any, undefined>): 'remotable';
<T extends PassStyled<TaggedOrRemotable, any>>(p: T): ExtractStyle<T>;
(p: { [key: string]: any }): 'copyRecord';
(p: any): PassStyle;
(p: any): never;
};
/**
* A Passable is PureData when its entire data structure is free of PassableCaps
Expand All @@ -135,7 +169,7 @@ export type PassStyleOf = {
* trip (as exists between vats) to produce data structures disconnected from
* any potential proxies.
*/
export type PureData = Passable<never, never>;
export type PureData = Passable<never, never, false>;
/**
* An object marked as remotely accessible using the `Far` or `Remotable`
* functions, or a local presence representing such a remote object.
Expand All @@ -150,7 +184,11 @@ export type RemotableObject<I extends InterfaceSpec = string> = PassStyled<
/**
* The authority-bearing leaves of a Passable's pass-by-copy superstructure.
*/
export type PassableCap = Promise<any> | RemotableObject;
export type PassableCap<
R extends RemotableObject = RemotableObject,
AllowPromise extends boolean = any,
> = R | (true extends AllowPromise ? PromiseLike<R> : never);

/**
* A Passable sequence of Passable values.
*/
Expand Down
Loading
Loading