feat(errors): confirm/reject pattern (as suggested by #2285)#2923
Merged
feat(errors): confirm/reject pattern (as suggested by #2285)#2923
Conversation
This was referenced Aug 6, 2025
erights
force-pushed
the
markm-rejector-only
branch
from
20d51ab to
29d8e4a
Compare
erights
force-pushed
the
markm-rejector-only
branch
from
29d8e4a to
cf81497
Compare
erights
force-pushed
the
markm-rejector-only
branch
from
cf81497 to
1b011f6
Compare
erights
force-pushed
the
markm-rejector-only
branch
from
1b011f6 to
5debed5
Compare
gibson042
approved these changes
Sep 16, 2025
erights
force-pushed
the
markm-rejector-only
branch
from
5f3461f to
8bfbef1
Compare
erights
added a commit
that referenced
this pull request
Sep 16, 2025
Closes: #XXXX Refs: #2911 #2285 #2918 #2922 ## Description Uses the new Rejector pattern suggested by #2285 and implemented in #2923 to restructure all the check*/Checker patterns in @endo/pass-style to use the confirm*/reject pattern instead. This is also a test and demonstration of the basic functionality of #2923. For this experiment, as @gibson042 expected, the code is both smaller and more readable. Should be a pure refactor, not causing any effects observable from outside @endo/pass-style ### Security Considerations If it is indeed a pure refactor, none. Except for course that the code is simpler, smaller, and easier to understand and review, which helps security. ### Scaling Considerations From eyeballing these, I suspect `passStyleOf` will be faster, but need to measure that to see how much. This could be significant, as previous measurement have consistently shown `passStyleOf` to be a bottleneck. ### Documentation Considerations The actual confim/reject pattern needs to be documented somewhere, for developers that wish to use it internally. But since the confirm* functions/methods normally encapsulated, as they are here, normal developers who consume our APIs won't need to be aware of these changes. ### Testing Considerations No tests needed to change, raising our confidence that it is a pure refactor. ### Compatibility Considerations As a pure refactor with no change to the representation of any data, should be none. There remains the linking problem if these changes to @endo/patterns are linked with a version of @endo/errors prior to #2923. This PR does pull in the `Rejector` type exported by @endo/errors/rejector.js, which does not exist in earlier forms of @endo/errors. But these are only static types and static type imports, so it is hard to see how it could cause a dynamic problem. ### Upgrade Considerations As a pure refactor, none.
erights
added a commit
that referenced
this pull request
Sep 16, 2025
Staged on #2917 Closes: #XXXX Refs: #2911 #2285 #2918 #2917 ## Description Uses the new Rejector pattern suggested by #2285 and implemented in #2923 to restructure all the check*/Checker patterns in @endo/patterns to use the confirm*/reject pattern instead. This is also a test and demonstration of the basic functionality of #2923. For this experiment, as @gibson042 expected, the code is both smaller and more readable. Should be a pure refactor, not causing any effects observable from outside @endo/patterns ### Security Considerations If it is indeed a pure refactor, none. Except of course that the code is simpler, smaller, and easier to understand and review, which helps security. ### Scaling Considerations From eyeballing these, I suspect `matches` and the non-throwing cases of `mustMatch` will be faster. But we need to measure that to see how much. This could be significant, as previous measurement have shown pattern matching to be somewhat of a bottleneck. ### Documentation Considerations The actual confim/reject pattern needs to be documented somewhere, for developers that wish to use it internally. But since the confirm* functions/methods are normally encapsulated, as they are here, normal developers who consume our APIs won't need to be aware of these changes. ### Testing Considerations No tests needed to change, ***except for some error message improvements***, raising our confidence that it is a pure refactor. ### Compatibility Considerations As a pure refactor with no change to the representation of any data, should be none. There remains the linking problem if these changes to @endo/patterns are linked with a version of @endo/errors prior to #2923. This PR does pull in the `Rejector` type exported by @endo/errors/rejector.js, which does not exist in earlier forms of @endo/errors. But these are only static types and static type imports, so it is hard to see how it could cause a dynamic problem. The ***error message improvements*** might cause some golden tests to fail, which will need fixing. However, such improvements are explicitly not considered semantic/normative, and thus the need to fix those golden tests is within the rules of the road for what we consider a "pure refactor". ### Upgrade Considerations As a pure refactor, none. --------- Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #2285
Refs: #2851 #2917 #2918 #2917 #2922
Description
Follows approximately @gibson042 's suggestions at #2285, with some minor differences I'd like @gibson042 's feedback on:
Rejectortype to end with=> neverrather than=> false.{false}type disjunct to theRejectortyperejectparameter to theconfirmFoofunctions non-optional, since it would also be supplied by theisFooorassertFoofunctions. Typically, only the latter two would be exported. The only reason I can see to makeconfirmFoo'srejectparameter optional is to be able to export and use it directly as the predicate, which would be an abstraction leak.As stated in #2285, the purpose is to migrate existing uses of
Checkerto useRejectorinstead. The first real tests for that is #2917 to use Rejectors instead of Checkers in @endo/pass-style, and #2922 to do likewise for @endo/patterns.Security Considerations
Since this PR itself is only additive, if it is correct, none. If so, then #2917 and #2922 hopefully should improve security since code using Rejectors should overall be simpler and easier to review for correctness that code using Checkers.
Scaling Considerations
Most of the point. We hope that code using Rejectors will be faster than current code using Checkers. We should measure the speedups of #2917 and #2922 over
masterbaseline.Documentation Considerations
Most code uses only
isFooorassertFoofunctions, whose behavior should be identical. Only users or definers of the oldcheckFooor the newconfirmFoofunctions need to understand the difference. Not sure where these are or should be documented.Testing Considerations
The test included in this PR also demonstrate the
confirm/reject/is/assertpattern that Rejectors are designed to support.Compatibility Considerations
For this PR by itself, since it is only additive, none. Further, the design of Rejectors is intended to enable code to switch over without causing external compat considerations. The redo of #2851 will be a good first test.
For any old
checkFoocode using Checkers rather than Rejectors, if thosecheckFoofunctions were exported, we can deprecate and continue to export them during the transition to further avoid compat issues.Upgrade Considerations
None of this causes any change to data that is communicated or stored. Given that the
isFooandassertFoofunctions are behaviorally identical and that any exportedcheckFoofunctions continue to be exported (as deprecated), there should be no upgrade concerns.