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

correct types for pattern matchers #6206

Open
turadg opened this issue Sep 14, 2022 · 6 comments
Open

correct types for pattern matchers #6206

turadg opened this issue Sep 14, 2022 · 6 comments
Labels
code-style defensive correctness patterns; readability thru consistency enhancement New feature or request tooling repo-wide infrastructure vaults_triage DO NOT USE

Comments

@turadg
Copy link
Member

turadg commented Sep 14, 2022

What is the Problem Being Solved?

While working on this again, documenting .optional would help (https://github.com/Agoric/agoric-sdk/pull/6202/files#r971342637 )

I wondered why code could use it without the type and it's because M.call() returns

/** @typedef {any} MethodGuardMaker */
/** @typedef {{ klass: 'methodGuard', callKind: 'sync' | 'async', returnGuard: unknown }} MethodGuard */

Relevant

Description of the Design

No any for pattern matcher types.

Security Considerations

Test Plan

@turadg turadg added enhancement New feature or request tooling repo-wide infrastructure code-style defensive correctness patterns; readability thru consistency labels Sep 14, 2022
@erights
Copy link
Member

erights commented Sep 15, 2022

FWIW, this is implemented at

optional: (...optArgGuards) => {
optionalArgGuards === undefined ||
assert.fail(X`Can only have one set of optional guards`);
restArgGuard === undefined ||
assert.fail(X`optional arg guards must come before rest arg`);
return makeMethodGuardMaker(callKind, argGuards, optArgGuards);
},
and used several times in https://github.com/Agoric/agoric-sdk/blob/master/packages/ERTP/src/typeGuards.js

I'm actually surprised I didn't write any comments explaining this at all. I certainly agree it is needed.

Also rest

@erights
Copy link
Member

erights commented Aug 28, 2023

@turadg How much of this issue is addressed by endojs/endo#1715 ?

Note that endojs/endo#1715 is merged into endo. But an endo with that change is not yet released, and therefore not yet available to agoric-sdk.

@turadg
Copy link
Member Author

turadg commented Aug 28, 2023

When I link HEAD of @endo/patterns, it appears to be solved by 1715.
Screenshot 2023-08-28 at 4 34 18 PM

Unfortunately the type when importing M from @agoric/store has gotten worse, in that the whole thing is any. I don't know why but I expect #7949 would fix it

@erights
Copy link
Member

erights commented Aug 28, 2023

Oh sorry! I lost track of #7949. Looking now.

Are you waiting on me for anything else?

@turadg
Copy link
Member Author

turadg commented Aug 29, 2023

No worries! Wasn't waiting on 7949 since it was low prio. If it was high I would have pestered you 😉

@erights
Copy link
Member

erights commented Aug 29, 2023

Good! In any case, LGTMed that one. Please let me know whether it fixes the issue here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-style defensive correctness patterns; readability thru consistency enhancement New feature or request tooling repo-wide infrastructure vaults_triage DO NOT USE
Projects
None yet
Development

No branches or pull requests

4 participants