feat: make SELChecker the single validation gate in evaluate#69
Merged
André L. (WipeAir) merged 7 commits intomainfrom Apr 1, 2026
Merged
feat: make SELChecker the single validation gate in evaluate#69André L. (WipeAir) merged 7 commits intomainfrom
André L. (WipeAir) merged 7 commits intomainfrom
Conversation
…Execution() Checker now runs first and validates parse, types, and lint rules in one pass. The runtime env.parse() is called only on valid expressions for execution. Parse and type errors now throw SELLintError with diagnostics instead of SELParseError/SELTypeError.
Remove the redundant ternary in the error-diagnostic guard. The false branch was dead code — errorDiags is empty only when checkResult.valid is true, in which case the if block is never entered. Rely on the checker's contract that valid: false always includes error diagnostics.
…ches to SELEvaluationError ParseError and CelTypeError from CEL runtime are now "should never happen" cases after the checker runs first, so they are wrapped as SELEvaluationError instead of the more specific SELParseError/SELTypeError.
These error classes are no longer used in sel-runtime after Tasks 1 and 2 removed all usages. Remove the re-exports from errors.ts and update the spec to cover the remaining error types.
Remove the two now-unused error classes and their tests. Update the errors barrel to stop re-exporting them, and fix stale JSDoc references in sel-editor and sel-runtime to use SELEvaluationError instead.
…rs.ts tombstone These error classes no longer exist; parse and type errors are now surfaced as SELLintError diagnostics via SELChecker. Clean up the README error table and the tombstone comment left in sel-common errors.ts.
André L. (WipeAir)
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors SELRuntime.evaluate() so that SELChecker.check() becomes the single validation gate (parse/type/lint), removing dedicated SELParseError/SELTypeError error types and shifting parse/type failures to SELLintError diagnostics for better positional reporting.
Changes:
- Route
evaluate()validation throughSELChecker.check()and remove duplicate runtime type-checking. - Remove
SELParseError/SELTypeErrorfrom@seljs/commonand runtime re-exports; update tests accordingly. - Simplify
wrapError()so unexpected CEL parse/type errors map toSELEvaluationError.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Removes SELParseError/SELTypeError from the public error table. |
| packages/sel-runtime/src/errors/errors.ts | Stops re-exporting removed error types from @seljs/common. |
| packages/sel-runtime/src/errors/errors.spec.ts | Updates error-type tests to reflect removed error classes. |
| packages/sel-runtime/src/environment/error-wrapper.ts | Maps CEL ParseError/TypeError into SELEvaluationError. |
| packages/sel-runtime/src/environment/error-wrapper.spec.ts | Updates wrapping tests to expect SELEvaluationError. |
| packages/sel-runtime/src/environment/environment.ts | Makes checker validation the gate for evaluate(); uses resolved checker type for encoding. |
| packages/sel-runtime/src/environment/environment.spec.ts | Updates evaluation error expectations to SELLintError diagnostics. |
| packages/sel-editor/src/linting/diagnostic-mapper.ts | Updates documentation comment for thrown error type from runtime check(). |
| packages/sel-common/src/errors/index.ts | Stops exporting removed error module. |
| packages/sel-common/src/errors/errors.ts | Deletes removed error classes. |
| packages/sel-common/src/errors/errors.spec.ts | Removes tests for deleted error classes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | `SELParseError` | Invalid CEL expression syntax | | ||
| | `SELEvaluationError` | Expression evaluation fails (undefined variables, etc.) | | ||
| | `SELTypeError` | Type checking fails | | ||
| | `SELLintError` | Lint rule with error severity violated (`.diagnostics`) | |
There was a problem hiding this comment.
The README no longer lists SELParseError/SELTypeError, but the remaining SELLintError description still implies it only covers lint-rule violations. Since evaluate() now throws SELLintError for parse and type-check failures too, update this row to reflect that SELLintError covers parse/type/lint diagnostics (with positions via .diagnostics).
| * @param expression - A CEL expression string to type-check | ||
| * @returns The type-check result containing validity, inferred type, and any errors | ||
| * @throws {@link SELTypeError} If the expression contains unrecoverable type errors | ||
| * @throws {@link SELEvaluationError} If the expression contains unrecoverable type errors |
There was a problem hiding this comment.
The check() method can throw SELEvaluationError for parse errors too (via wrapError(ParseError)), not only for "unrecoverable type errors". Consider broadening this @throws description (e.g., "unrecoverable parse/type-check errors") so it matches the actual behavior.
Suggested change
| * @throws {@link SELEvaluationError} If the expression contains unrecoverable type errors | |
| * @throws {@link SELEvaluationError} If the expression contains unrecoverable parse or type-check errors |
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
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.
Summary
SELChecker.check()the first and only validation gate inevaluate(), eliminating duplicate parse and type-check callsSELParseErrorandSELTypeErrorfrom the codebase — parse/type errors are now reported asSELLintErrorwith diagnostics (better UX with position info)wrapError()to map any unexpected CEL parse/type errors toSELEvaluationErrorTest plan
evaluate()throwSELLintErrorwith diagnosticsevaluate()throwSELLintErrorwith diagnosticsSELLintErroras before