endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
762 stars 68 forks source link

"Checker" parameters would be more ergonomic as "Rejectors" #2285

Open gibson042 opened 1 month ago

gibson042 commented 1 month ago

What is the Problem Being Solved?

2284 demonstrated that there is a relatively small but still significant performance cost to creating reject = !!check && ((T, ...subs) => check(false, X(T, ...subs))) functions that will only be used in failure paths, and so moved that definition into a new CX function and updated many sites to use that:

-    reject && reject`…`
+    !!check && CX(check)`…`

However, this has drawbacks of its own with respect to comprehensibility and readability, especially when prettier introduces line breaks like

    (
      !!check &&
      CX(
        check,
      )`…`
    )

Description of the Design

I think we'd be better off replacing (cond: boolean, details?: Details | undefined) => boolean "Checker" parameters with optional (template: TemplateStringsArray | string[], ...args: any[]) => false "Rejector" parameters, recovering the old local pattern but without local function creation.

And per https://github.com/endojs/endo/issues/2285#issuecomment-2115922362 , we'll also want to rename internal functions for indicating the progress of this transition.

-const checkSomething = (candidate, check = undefined) => {
+const confirmSomething = (candidate, reject = false) => {
   return (
     isOk(candidate) ||
-    (!!check && CX(check)`…`)
+    (reject && reject`…`)
   );
 };

And at the highest possible level, we'd replace use of x => x checkers with undefined rejectors and assertChecker with assert.Fail.

-    checkSomething(candidate, assertChecker)
+    confirmSomething(candidate, Fail)

Security Considerations

None that I can see.

Scaling Considerations

Negligible, but the new pattern might be slightly faster.

Test Plan

n/a, this is behavior-preserving refactoring.

Compatibility Considerations

assertChecker is exported by the pass-style package, and used by at least the patterns package, which also makes use of checkers so should be updated similarly. But I think all such use is internal for supporting {is,assert}$Type package exports (e.g., isKey and assertKey).

Upgrade Considerations

If any exports do expose a Checker parameter, we'll need to consider a deprecation strategy.

erights commented 1 month ago

I like this a lot! Deprecation is somewhat of an issue. Fortunately, most packages only use a checkFoo internally, exporting only the isFoo and assertFoo callers, which would not need to change. To help preserve correctness across this transition, I suggesting changing the checkFoo names to something else. What?

gibson042 commented 1 month ago

Hmm, I was thinking the "check" names would remain, but that does make sense. The "is" names would be perfect if they weren't theirselves exposed, but I'd rather not reveal this new pattern (at least not right now). So I guess we're looking at synonyms or near-synonyms of "check" ("verify", "ensure", "test", "matches", etc.). What about "matches", in loose correspondence with patterns?

export const isSafePromise = pr => matchesSafePromise(pr);
export const assertSafePromise = pr => matchesSafePromise(pr, Fail);

I think that would work directly everywhere except in patternMatchers.js, where we'd probably redefine both checkMatches and matches in terms of e.g. const privateMatches = (specimen, pattern, reject = false, label = undefined) => ….

erights commented 1 month ago

I like the direction. I'm ok with the choice of matchesFoo but the collision you mention does make it awkward. You mention test. How about testFoo? I think I like that better. Does it have any problematic collisions?

erights commented 1 month ago

After more reflection, I really don't like matchesFoo because it sounds too specific to patterns and pattern matching. I also no longer like testFoo because it sounds like it has something to do with our ava regression testing. ensureFoo isn't quite right, because it suggests that it makes sure that foo is true. verifyFoo isn't bad. What are other candidates?

gibson042 commented 1 month ago

https://www.merriam-webster.com/thesaurus/verify suggests "confirm", "validate", "certify", "affirm", etc. Of the entire list, I think I like "verify" and "confirm" best.

export const isSafePromise = pr => verifySafePromise(pr);
export const assertSafePromise = pr => verifySafePromise(pr, Fail);
export const isSafePromise = pr => confirmSafePromise(pr);
export const assertSafePromise = pr => confirmSafePromise(pr, Fail);
erights commented 1 month ago

Both sound good to me. I like confirmFoo slightly better.