Codex/typechecker contract audit - #652
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (48)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR is a broad “typechecker contract audit” pass that tightens WFL’s static typing to better match runtime behavior, adds extensive regression coverage, and documents the intended type-system invariants for future contributions.
Changes:
- Expanded the type system and parser/fixer surface grammar to support richer, parity-driven typing (e.g.,
Optional<T>, colon-framed action return types,List of Tproperty annotations, date/time keywords). - Hardened runtime parity around builtins,
Nothing/Nullhandling, container/static-member behavior, and flow-sensitive joins (try/finally, loops, definite bindings). - Added a large suite of new integration/unit tests plus new developer documentation and a dev diary entry capturing the audited contract.
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/typechecker_try_finally_join_test.rs | Extends try/when/finally flow-join tests; adds runtime checks and parsing helper. |
| tests/typechecker_statement_completion_parity_test.rs | New: verifies statement “completion” types match runtime expectations. |
| tests/typechecker_runtime_binding_test.rs | New: asserts typechecker preserves concrete types for runtime-created bindings. |
| tests/typechecker_reuse_test.rs | New: ensures analyzer/typechecker state does not leak across runs. |
| tests/typechecker_response_stream_scope_test.rs | Updates response-stream scoping tests to use real request bindings and operand traversal. |
| tests/typechecker_response_stream_join_test.rs | Expands join/backedge tests (including repeat-until semantics and maybe-empty loop behavior). |
| tests/typechecker_response_contract_test.rs | New: checks respond statement operand traversal and runtime-supported content contracts. |
| tests/typechecker_loop_runtime_parity_test.rs | New: validates loop fixed-point/iteration-scope rules against runtime behavior. |
| tests/typechecker_legacy_list_property_test.rs | New: enforces legacy list-property mutation/type contracts and shadowing precedence. |
| tests/typechecker_expression_coverage_test.rs | New: ensures operands are traversed and checked even for fixed-result expressions. |
| tests/typechecker_definite_binding_test.rs | New: definite-binding escape rules for if/single-line-if and mutability joins. |
| tests/typechecker_container_contract_test.rs | New: comprehensive container contract coverage (init, inheritance, methods, events, properties). |
| tests/typechecker_builtin_contract_test.rs | New: validates builtin arity/type contracts, aliasing, and “unimplemented reserved” behavior. |
| tests/typechecker_alias_provenance_residual_test.rs | New: regression tests for alias/provenance and diagnostic residual behavior. |
| tests/stream_handle_type_test.rs | Adds tests for concrete HttpStream field typing and end-of-stream Nothing guarding. |
| tests/static_container_member_test.rs | New: end-to-end + runtime tests for static members, inheritance, and stored method refs. |
| tests/overload_test.rs | Adds overload resolution coverage for temporal/branded types and adjusts describe-nested test expectations. |
| tests/open_file_local_type_test.rs | Updates test scenario for outer binding type preservation across loop-local file handle rebinding. |
| tests/nothing_reassign_widen_test.rs | Adds coverage that change ... to nothing updates flow types (including conditional cases). |
| tests/fixer_return_type_roundtrip_test.rs | New: ensures CodeFixer emits action return types that reparse back to the same AST types. |
| tests/container_parsing_fixes.rs | Extends parsing regression suite for temporal keywords and list property annotations. |
| tests/action_return_type_residuals_test.rs | Expands action return inference tests and adds runtime checks for loop/try/finally return behavior. |
| TestPrograms/docs_examples/keyword_reference/pattern_examples.wfl.ast.txt | Adds AST snapshot output for a keyword-reference docs example. |
| src/stdlib/random.rs | Adds test ensuring random_seed preserves legacy Nothing identity. |
| src/stdlib/list.rs | Strengthens find “not found” behavior tests to assert legacy Nothing identity semantics. |
| src/stdlib/json.rs | Adds test ensuring JSON null maps to legacy Nothing identity. |
| src/stdlib/core.rs | Treats Nothing like Null in isnothing, and adds unit tests for both variants. |
| src/parser/stmt/containers.rs | Implements recursive colon-style property type parsing (notably List / List of T). |
| src/parser/stmt/actions.rs | Adds framed (double-colon) recursive action return type parsing and colon-style type token helpers. |
| src/parser/ast.rs | Extends Type with Date, Time, DateTime, and Optional<T>. |
| src/interpreter/value.rs | Adds static-method runtime context support, shares static properties via Rc<RefCell<_>>, and aligns Null/Nothing equality. |
| src/interpreter/memory_tests.rs | Updates interpreter memory tests for the new function/static-method context fields. |
| src/interpreter/environment.rs | Adds helpers for method-local shadowing behavior and temporary alias binding save/restore. |
| src/fixer/tests.rs | Adds fixer roundtrip tests for temporal types and action return type syntax. |
| src/fixer/mod.rs | Updates parameter formatting, emits framed return types, and adds formatting support for new Type variants. |
| src/builtins.rs | Separates “recognized” vs “implemented” builtin inventories and adds runtime arity-range API. |
| src/analyzer/static_analyzer.rs | Adds analyzer guidance text for inherited-property compatibility warnings. |
| Docs/development/type-system-design.md | New: documents the audited type-system contract and runtime parity expectations. |
| Docs/development/index.md | Links the new type system design document from development index. |
| Docs/development/compiler-internals.md | Cross-references the new type-system contract doc from compiler internals. |
| Dev diary/2026-07-26-typechecker-gradual-contract-audit.md | New: dev diary record of audit scope, findings, and verification steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The type checker must reconstruct `out` as File while checking the loop, | ||
| // then expose the original outer Text binding after leaving that scope. | ||
| let source = "store out as \"outer.txt\"\n\ | ||
| // then expose the original outer Number binding after leaving that scope. | ||
| // Text is intentionally accepted as an opaque runtime file-handle ID. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a3a88c04e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for prop in static_properties { | ||
| let value = match &prop.default_value { | ||
| Some(expression) => { | ||
| self._evaluate_expression(expression, env.clone()).await? |
There was a problem hiding this comment.
This activates static properties and methods at runtime, but the user-facing container guide and language specification contain no static-member syntax, access, mutation, inheritance, or lifetime semantics; the only new explanation is the contributor-oriented type-system design note. Add validated examples and reference documentation so users can discover and safely use this newly supported behavior.
AGENTS.md reference: AGENTS.md:L195-L200
Useful? React with 👍 / 👎.
| Type::Pattern => "Pattern".to_string(), | ||
| Type::Date => "date".to_string(), | ||
| Type::Time => "time".to_string(), | ||
| Type::DateTime => "datetime".to_string(), |
There was a problem hiding this comment.
🔍 Type::DateTime return annotations do not round-trip through the fixer
format_type/format_action_return_type emit "datetime" (lowercase) for Type::DateTime (src/fixer/mod.rs:1176). On reparse, type_from_token/parse_action_return_type only map the keyword tokens date/time to Type::Date/Type::Time; datetime lexes as a plain Identifier and resolves to Type::Custom("datetime") (src/parser/stmt/actions.rs:26-34). So a Type::DateTime would not survive a fix+reparse cycle, silently degrading to Custom("datetime"), which contradicts the runtime-parity checklist item 'formatting or auto-fixing preserves type identity on reparse'. In practice the parser never produces Type::DateTime for a parameter or return annotation (it always yields Custom("datetime")), so the fixer, which operates on the parsed AST, should not encounter it today. The added tests deliberately only cover Type::Custom("datetime"), not Type::DateTime, so this gap is untested. Worth confirming no code path hands the fixer a Type::DateTime.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn register_json(analyzer: &mut Analyzer) { | ||
| register(analyzer, &["parse_json"], vec![Type::Text], Type::Any); | ||
|
|
||
| analyzer.register_builtin_function("sha256", param_types, return_type); | ||
| let json_values = [ | ||
| Type::Nothing, | ||
| Type::Boolean, | ||
| Type::Number, | ||
| Type::Text, | ||
| list(Type::Any), | ||
| map(Type::Text, Type::Any), | ||
| ]; | ||
| for value_type in json_values { | ||
| register( | ||
| analyzer, | ||
| &["stringify_json", "stringify_json_pretty"], | ||
| vec![value_type], | ||
| Type::Text, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔍 stringify_json contract narrowed from Any to a fixed set of value kinds
register_json previously registered stringify_json/stringify_json_pretty with a single Type::Unknown parameter (accepting any value), but now registers six explicit overloads over Nothing, Boolean, Number, Text, List<Any>, and Map<Text, Any> (src/stdlib/typechecker.rs:327-345). Any program that statically passes another concrete type (e.g. a Date/temporal value or a container instance) to stringify_json will now be rejected by the type checker where it previously type-checked. Whether this is a backward-compatibility regression depends on whether native_stringify_json actually errors on those inputs at runtime; the audit claims 'a statically rejected concrete type would also fail at runtime', but this pairing should be verified against the runtime implementation before relying on it.
Was this helpful? React with 👍 or 👎 to provide feedback.
1a3a88c to
3938edb
Compare
Adding the expected syntax tree for the pattern matching reference code. This serves as a "blueprint" that allows automated tests to verify the examples shown in the documentation are valid and remain correct as the language evolves.
The #642 repeat-until backedge regression tests retype `out` by opening a response stream in the loop body. After rebasing onto main, the gradual type-checking contract added by this PR requires the streaming target to be a request object, so the previous text-literal target ("req") is now a type error that masked the softening behavior under test. Bind `req` via `wait for request comes in on srv as req` and stream `to req`, matching the convention this PR already applies to the sibling response-stream tests. The softening assertions are unchanged and still pass. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| Type::Pattern => "Pattern".to_string(), | ||
| Type::Date => "date".to_string(), | ||
| Type::Time => "time".to_string(), | ||
| Type::DateTime => "datetime".to_string(), |
There was a problem hiding this comment.
The fixer's Type::DateTime arm is unreachable for parsed programs: WFL has no DateTime/datetime keyword, so datetime annotations parse to Type::Custom("datetime") (handled by the Custom arm), and both datetime and DateTime reparse to Custom and bridge back to Type::DateTime via case-insensitive comparison, so identity is preserved either way. Lowercase datetime is the codebase's canonical spelling (analyzer type_to_string, temporal_kind); WFL explicitly has no unambiguous capital DateTime source form, so emitting it would only diverge without improving round-trip.
| /// An inferred value that may be `Nothing` because an action can fall | ||
| /// through without executing a value-returning `return`. | ||
| Optional(Box<Type>), |
Re-add the test-only `RunState::fresh(call_depth)` constructor that an earlier rebase dropped. The inline concurrent-handler unit tests reference it, so `cargo test` failed to compile the lib test target even though a plain `cargo build` (which never compiles that cfg(test) code) succeeded. Stop downgrading WebSocket handler event bindings to `Unknown`. `bind_runtime_value` already recreates the binding in the handler scope with its concrete runtime map type, shadowing any outer same-named symbol (#642), so keeping that map type leaves field/index access on the event object permissive while still rejecting misuse such as arithmetic on it. This restores the runtime-binding contract test. Also broaden the `Type::Optional` doc comment to cover every `T | Nothing` source (find-style lookups, control-flow joins), not just action fall-through, and correct a stale comment in the open-file local-type test. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
3938edb to
b8acbdf
Compare
is nothing: Value::Null and Value::Nothing are crossed and never compare equal
#651
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Summary by CodeRabbit
New Features
Bug Fixes
Nothingvalues.Documentation
Tests