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.

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

Merged
merged 6 commits into from
Jan 11, 2024
Merged

1488 Passable (REVERTED in #1961) #1933

merged 6 commits into from
Jan 11, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Jan 8, 2024

closes: #1488

Description

  • 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

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.

Copy link
Contributor

@dckc dckc left a 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?

packages/pass-style/src/types.js Outdated Show resolved Hide resolved
@turadg turadg force-pushed the 1488-Passable branch 2 times, most recently from a6c2693 to 75605a1 Compare January 9, 2024 22:43
@turadg turadg mentioned this pull request Jan 9, 2024
Comment on lines 74 to 75
* (p: Iterable): 'remotable';
* (p: Iterator): 'remotable';
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

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

t.is(passStyleOf(entriesIterable), 'remotable');
const entriesIterator = entriesIterable[Symbol.iterator]();
t.is(passStyleOf(entriesIterator), 'remotable');

Copy link
Contributor

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

* @typedef {Passable} RemotableObject
* @template {string} S pass style
* @template {InterfaceSpec} I interface tag
* @typedef {PassStyled<S> & {[Symbol.toStringTag]: I}} TaggedRecord
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.... 🤔

Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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: ").

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! 1b30d60

Copy link
Contributor

@gibson042 gibson042 left a 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!!!

packages/marshal/src/marshal.js Outdated Show resolved Hide resolved
packages/marshal/src/marshal.js Outdated Show resolved Hide resolved
@@ -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>}
Copy link
Contributor

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)?

Copy link
Contributor

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

Comment on lines 103 to 104
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if`
assert(passStyleMemo.has(inner));
Copy link
Contributor

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?

Suggested change
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if`
assert(passStyleMemo.has(inner));

Copy link
Member Author

@turadg turadg Jan 10, 2024

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?

Copy link
Contributor

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.

Copy link
Contributor

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 .

Copy link
Contributor

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.

Comment on lines 169 to 174
/**
* 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;
* }}
Copy link
Contributor

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?

Suggested change
/**
* 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;
* }}

Copy link
Contributor

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.

Copy link
Member Author

@turadg turadg Jan 10, 2024

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,

* @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?

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 an unpleasant surprise, at least.

I suppose the tag is pass-invariant, so using it for static typing is maybe not all that bad.

Copy link
Contributor

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 .

* @typedef {Passable} RemotableObject
* @template {string} S pass style
* @template {InterfaceSpec} I interface tag
* @typedef {PassStyled<S> & {[Symbol.toStringTag]: I}} TaggedRecord
Copy link
Contributor

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/keys/copySet.js Outdated Show resolved Hide resolved
@@ -22,7 +22,7 @@ export {};
/** @typedef {import('@endo/marshal').RankCover} RankCover */

/**
* @typedef {Passable} Key
* @typedef {Exclude<Passable, Error | Promise>} Key
Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise here.

@mhofman
Copy link
Contributor

mhofman commented Jan 10, 2024

I am only following from a distance, but I believe you can have circular type definitions in .d.ts files, but not in JSDoc

@turadg
Copy link
Member Author

turadg commented Jan 10, 2024

I am only following from a distance, but I believe you can have circular type definitions in .d.ts files, but not in JSDoc

I'm not sure about that but I do know the solution is more complicated than changing to .ts. Even with .ts,

export type CopyArray<T extends Passable = Passable> = T[];

causes

Type alias 'PassByCopy' circularly references itself.

@turadg turadg requested a review from gibson042 January 10, 2024 21:24
@turadg turadg marked this pull request as ready for review January 10, 2024 21:24
@erights
Copy link
Contributor

erights commented Jan 10, 2024

Attn @michaelfig re #1933 (comment) circular types mysteries

@dckc
Copy link
Contributor

dckc commented Jan 10, 2024

#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:

@turadg
Copy link
Member Author

turadg commented Jan 10, 2024

#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.

I think that solved it, @dckc . PTAL 5b8b349 9aac68e.

Copy link
Contributor

@erights erights left a 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
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 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.

Copy link
Member Author

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]
Copy link
Contributor

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.

Comment on lines 103 to 104
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if`
assert(passStyleMemo.has(inner));
Copy link
Contributor

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 .

Comment on lines 103 to 104
// sanity check on falsy values. Do 'get' first to get the type narrowing of the `if`
assert(passStyleMemo.has(inner));
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent!

No change suggested.

Comment on lines +1 to +2
// @ts-nocheck So many errors that the suppressions hamper readability.
// TODO parameterize MatchHelper which will solve most of them
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting.

No change suggested.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM!!

};
export type ExtractStyle<P extends PassStyled<any>> = P[typeof PASS_STYLE];
export type PassByCopy =
| import('type-fest').Primitive
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

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.

Comment on lines 26 to 52
| any[]
| CopyRecord
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 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to CopyArray

Comment on lines +29 to +58
export type PassByRef =
| RemotableObject
| Promise<RemotableObject>
| Promise<PassByCopy>;
Copy link
Contributor

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

Suggested change
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.

Copy link
Contributor

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>>

Comment on lines +55 to +84
export type Passable<
PC extends PassableCap = PassableCap,
E extends Error = Error,
Copy link
Contributor

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.

Comment on lines +71 to +99
export type PassStyleOf = {
(p: undefined): 'undefined';
Copy link
Contributor

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?

Copy link
Member Author

@turadg turadg Jan 11, 2024

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.

(p: any[]): 'copyArray';
(p: Iterable<any>): 'remotable';
(p: Iterator<any, any, undefined>): 'remotable';
<T extends PassStyled<any>>(p: T): ExtractStyle<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 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?

Copy link
Member Author

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>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! No change suggested.

Comment on lines 157 to 158
* This is an interface specification.
* For now, it is just a string, but will eventually be `PureData`. Either
Copy link
Contributor

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.

Copy link
Member Author

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 = (
Copy link
Contributor

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.

@mhofman mhofman self-requested a review January 11, 2024 00:17
Copy link
Contributor

@mhofman mhofman left a 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

| 'remotable'
| 'error'
| 'promise';
export type PassStyled<S extends string> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not extends PassStyle?

Comment on lines +29 to +58
export type PassByRef =
| RemotableObject
| Promise<RemotableObject>
| Promise<PassByCopy>;
Copy link
Contributor

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>>

packages/pass-style/src/types.d.ts Outdated Show resolved Hide resolved
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 turadg merged commit e423804 into master Jan 11, 2024
14 checks passed
@turadg turadg deleted the 1488-Passable branch January 11, 2024 01:09
@turadg turadg mentioned this pull request Jan 11, 2024
@dckc dckc changed the title 1488 Passable 1488 Passable (REVERTED in #1961) Jan 11, 2024
@erights
Copy link
Contributor

erights commented Jan 20, 2024

@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?

@turadg
Copy link
Member Author

turadg commented Jan 20, 2024

What is needed?

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

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?

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.

@turadg turadg mentioned this pull request Apr 23, 2024
1 task
turadg added a commit that referenced this pull request May 2, 2024
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.
erights added a commit that referenced this pull request May 5, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing Remotable and PassByRef types
5 participants