1488 Passable redux by turadg · Pull Request #2238 · endojs/endo

turadg · 2024-04-23T22:40:22Z

closes: #1488

Description

Reattempting:

1488 Passable (REVERTED in #1961) #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

1488 Endo Passable 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 · 2024-04-23T23:05:14Z

(I just reran the windows-latest flake)

erights · 2024-04-23T23:07:08Z

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.

@kriskowal , I defer to you on this. I'll just say I hope we can adopt that precedent as a general rule ;)

erights

LGTM, thanks!

packages/marshal/src/encodeToCapData.js

erights · 2024-04-23T23:52:28Z

packages/marshal/src/marshal.js


    /**
-     * @param {Remotable | Promise} passable
+     * @param {import('@endo/pass-style').PassableCap} passable


Now that we have @import, and since you're changing these lines anyway, I prefer to gather these all the top into @import declarations.

erights · 2024-04-23T23:56:58Z

packages/marshal/src/marshal.js

+      const extant = valMap.get(index);
+      if (extant) {


Yeah, when the value is necessarily truthy, this pattern is better. (Likewise extant !== undefined when the value is necessarily not undefined.) Only problem in general is the refactoring hazard if a collection's use changes to include falsy or undefined values. Not worried about that here.

packages/marshal/src/rankOrder.js

erights · 2024-04-24T00:02:38Z

packages/marshal/src/rankOrder.js

 * in reverse order by passing a reversed rank comparison function.
 *
- * @param {Iterable<Passable>} passables
+ * @template {Passable} T


What do you think of

Suggested change

* @template {Passable} T

* @template {Passable} [T=Passable]

for all or most of these? It offloads the use site from needing to say <Passable> when that's all it would say anyway. I've been doing essentially this elsewhere and been happy with the results.

that makes sense for parameterized typedefs, but here the template slot's type is constrained by the first argument. Giving a default doesn't help.

erights · 2024-04-24T00:07:56Z

packages/pass-style/src/passStyle-helpers.js

  apply(objectHasOwnProperty, obj, [prop]);
 harden(hasOwnPropertyOf);

+/** @type {(val) => val is {}} */


I've never seen this inline before. Nice.

packages/pass-style/src/passStyleOf.js

erights · 2024-04-24T00:15:38Z

packages/pass-style/src/typeGuards.js

 harden(isRemotable);

 /**
- * @callback AssertArray


Yeah, I don't remember why I have these separate named @callable types. Good riddance, thanks.

erights · 2024-04-24T00:22:40Z

packages/patterns/src/keys/checkKey.js


 /**
- * @param {Passable} val
+ * @param {Key} val


Shouldn't this be

Suggested change

* @param {Key} val

/**

* @param {any} val

* @returns {val is Key}

*/

?

good one!

I've added a test showing how the is Key gets used. I don't think we want to losen the param because if it's not even Passable then that would be good static feedback. If you want to loosen it, please do in a separate PR across all the similar cases.

erights · 2024-04-24T00:25:59Z

packages/patterns/test/types.test-d.ts

+// @ts-expect-error M.any missing parens
+M.arrayOf(M.any);


erights · 2024-04-24T00:27:48Z

Did we stop requiring conventional commits?

erights · 2024-04-24T19:43:05Z

Did we stop requiring conventional commits?

My mistake. All your commits are fine. I was misled by the title, as my habit is to have the title also follow this convention (and usually be identical to the first commit).

kriskowal

I’m substantially excited about this change. I would like to break out a PR for the change to the release and build process so that it can suffer a revert separately if necessary. I’d also like to replace temporary markers XXX and FIXME with something but it doesn’t have to be tedious. It could be a single explanation or link to a tracking issue with a bunch of dittos or daggers for the repeated cases.

⚠️I will not die on this hill and I will approve this change regardless⚠️: My intuition (born from typed languages and sympathy for dependant projects that have to adapt when they upgrade this dependency) is that we should consider this a breaking change both in spirit and semver, and rush the necessary adaptations in Agoric SDK and dapps to minimize the pain. Since we are treading heavily over breaking changes with the “sync endo versions” workflow and resolutions farther out, there’s not much reason to spare the major version bump.

kriskowal · 2024-04-29T19:58:10Z

packages/base64/package.json

  "scripts": {
    "build": "exit 0",
-    "build:types": "tsc --build tsconfig.build.json",
+    "prepack": "tsc --build tsconfig.build.json",


I would like to capture this process change in a separate PR.

kriskowal · 2024-04-29T19:58:51Z

packages/captp/src/captp.js

-   * @type {import('@endo/marshal').ConvertSlotToVal<import('./types.js').CapTPSlot>}
+   * @type {import('@endo/marshal').ConvertValToSlot<import('./types.js').CapTPSlot>}
   */
+  // @ts-expect-error intentional hack


What’s the nature of the hack?

bad comment. the type was wrong so I just removed it

kriskowal · 2024-04-29T19:59:01Z

packages/captp/src/captp.js

          epoch,
          questionID,
          target,
+          // @ts-expect-error XXX


Please expand XXX

kriskowal · 2024-04-29T20:00:07Z

packages/exo/index.js

@@ -1,3 +1,6 @@
 export * from './src/exo-makers.js';

+// eslint-disable-next-line import/export -- ESLint not aware of type exports in types.d.ts


That’s unfortunate and might explain some of my experience.

yeah. I think we should consider disabling the rule. I think it has more false positives and fewer true positives than TS

kriskowal · 2024-04-29T20:00:32Z

packages/exo/src/types.js

@@ -0,0 +1,2 @@
+/** @file Empty twin for .d.ts */


kriskowal · 2024-04-29T20:07:08Z

packages/marshal/src/marshal.js

    const reviveFromSmallcaps = makeDecodeFromSmallcaps({
+      // @ts-expect-error FIXME
      decodeRemotableFromSmallcaps,
+      // @ts-expect-error FIXME


kriskowal · 2024-04-29T20:07:50Z

packages/marshal/src/rankOrder.js

        if (left < right) {
          return -1;
        } else {
+          // @ts-expect-error FIXME narrowed


👆 Is the intention to address all FIXMEs before landing? These could be links to an issue, otherwise.

not before landing. maybe never. I've taken out the FIXME

kriskowal · 2024-04-29T20:08:55Z

packages/marshal/src/rankOrder.js

      }
      case 'symbol': {
        return comparator(
+          // @ts-expect-error FIXME narrowed


I would not object to ditto if these stick around.

Suggested change

// @ts-expect-error FIXME narrowed

// @ts-expect-error ditto, type narrowed

kriskowal · 2024-04-29T20:10:51Z

packages/pass-style/src/passStyleOf.js

          }
          for (const helper of passStyleHelpers) {
            if (helper.canBeValid(inner)) {
+              // @ts-expect-error XXX


kriskowal · 2024-04-29T20:12:22Z

packages/patterns/src/keys/compareKeys.js

      // rank order.
      // Because the invariants above apply to the elements of the array,
      // they apply to the array as a whole.
+      // @ts-expect-error FIXME narrowed


👆 I’m fine with a link and dittos if these are intended to commit. (but don’t like FIXME because I like FIXME and XXX to be used as markers for work that needs to be ironed out before a merge.

kriskowal · 2024-04-29T20:30:19Z

Also, please add NEWS.md with a description of what you’ve done for Agoric/agoric-sdk#8774 so we can refer developers to this when they upgrade their *apps.

turadg · 2024-04-29T23:26:17Z

I had to rebase to resolve merge conflicts created by #2256

I squashed the fixups and rolled in other changes requested by review

fixes a problem with .js files having .d.ts twin

kriskowal

You may want to look over the remaining XXX and TODO cases.

turadg requested review from erights and kriskowal April 23, 2024 22:40

erights approved these changes Apr 24, 2024

View reviewed changes

erights mentioned this pull request Apr 24, 2024

feat: checked cast with TypedMatcher Agoric/agoric-sdk#8394

Merged

turadg force-pushed the 1488-Passable-redux branch from 718b502 to c9cdff1 Compare April 24, 2024 16:33

turadg force-pushed the 1488-Passable-redux branch 3 times, most recently from 78ef12b to 8462132 Compare April 26, 2024 21:38

turadg mentioned this pull request Apr 29, 2024

1488 Endo Passable Agoric/agoric-sdk#8774

Closed

kriskowal requested changes Apr 29, 2024

View reviewed changes

kriskowal added the next-release label Apr 29, 2024

turadg force-pushed the 1488-Passable-redux branch 2 times, most recently from f22e68c to 9444daf Compare April 29, 2024 23:39

turadg requested a review from kriskowal April 30, 2024 00:05

turadg added 6 commits May 2, 2024 14:37

lint: assume tsconfig.json

2b4ae1b

lint: enable EXPERIMENTAL_useProjectService

7ac7d8b

fixes a problem with .js files having .d.ts twin

lint: declare globals

f39b37d

test: isFake type

531c2ac

chore(types): type makeTagged

7d43966

feat(types): generic Passable

fa59e05

kriskowal approved these changes May 2, 2024

View reviewed changes

dckc and others added 3 commits May 2, 2024 15:25

chore(patterns): define Key using parameterized Passable

9fd3e4d

chore(types): misc accommodations

9d4bb52

chore(types): consolidate PassStyled type

e76d85b

turadg added 12 commits May 2, 2024 15:27

refactor: innerStyle

a2898b0

refactor(types): more imports

e67e1fa

chore(types): clean up jsdoc

bd5504b

test(types): stub

0787913

build: export patterns types

6206870

build: export types at top level

c3cdfe3

feat(types): Guard is a RemotableObject

e2bfb54

chore(types): don't narrow 'any'

905172e

chore(types): ToFarFunction

9222cff

chore(type): type inspection fns should allow any

dacd908

feat(types): fromCapData is Passable, but unknown is more practical

5fa54f0

docs: TODO for deeplyFulfilled

1ae099d

turadg force-pushed the 1488-Passable-redux branch from 9444daf to 1ae099d Compare May 2, 2024 22:29

turadg enabled auto-merge (rebase) May 2, 2024 22:29

turadg disabled auto-merge May 2, 2024 22:29

turadg enabled auto-merge May 2, 2024 22:29

turadg merged commit 334b016 into master May 2, 2024

turadg deleted the 1488-Passable-redux branch May 2, 2024 22:36

erights mentioned this pull request Jul 11, 2024

9449 membrane types Agoric/agoric-sdk#9685

Merged

kriskowal removed the next-release label Jul 24, 2024

- * @param {Key} val
+/**
+ * @param {any} val
+ * @returns {val is Key}
+ */

		@@ -1,3 +1,6 @@
		export * from './src/exo-makers.js';

		// eslint-disable-next-line import/export -- ESLint not aware of type exports in types.d.ts

	// @ts-expect-error FIXME narrowed
	// @ts-expect-error ditto, type narrowed

Comments

Conversation

turadg commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Uh oh!

erights commented Apr 23, 2024

Uh oh!

erights commented Apr 23, 2024

Uh oh!

erights left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erights commented Apr 24, 2024

Uh oh!

erights commented Apr 24, 2024

Uh oh!

kriskowal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg commented Apr 23, 2024 •

edited

Loading

turadg Apr 24, 2024 •

edited

Loading

turadg Apr 24, 2024 •

edited

Loading