Repository navigation
fix(vm): check a function from Any against the function type it's used as 🔒 - #329
Conversation
…d as 🔒 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c79999a8
ℹ️ 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".
| let returned = self.stack.last().expect("a call leaves its result"); | ||
| if returned.conforms_to(&return_type) { |
There was a problem hiding this comment.
When a checked function returns another function whose runtime return type is unknown, conforms_to rejects the result instead of wrapping it with the required check. For example, fn id(x) => x; fn make() => id; let a: Any = make; let f = a as Fn() -> Fn(Int) -> Int; print(f()(1)); now fails when make returns id, even though converting id directly to Fn(Int) -> Int succeeds. Use the conversion path for the returned stack value so valid higher-order functions can acquire checks recursively.
Useful? React with 👍 / 👎.
| if !takes || !could_be(return_type, expected_return) { | ||
| FunctionFit::No |
There was a problem hiding this comment.
For a function annotated to return Never, could_be falls back to equality and rejects every other expected return type. Thus fn forever() -> Never { forever() } let a: Any = forever; let f = a as Fn() -> Int; now fails at the cast, although such a function cannot return an invalid value and StaticType::matches explicitly allows Never to fit any type. Handle Never as a compatible source return type rather than rejecting this valid function conversion.
Useful? React with 👍 / 👎.
| let depth = self.frames.len(); | ||
| if self.dispatch_call(Rc::unwrap_or_clone(function), args)? == Dispatch::FramePushed { | ||
| self.run_to_depth(depth)?; |
There was a problem hiding this comment.
When recursion passes through a checked function on each iteration, this nested run_to_depth invocation adds Rust stack frames instead of only growing the VM's heap-backed frame stack. For example, initialize let f: Fn(Int) -> Int = fn(x: Int) => 0;, declare fn step(x: Int) { if x == 0 { 0 } else { f(x - 1) } }, then execute let a: Any = step; f = a; f(10_000);. Each recursive call re-enters call_checked, causing a host stack overflow and process abort, whereas the previous implementation dispatched these calls iteratively. The existing 005_functions/027_10k_recursion_depth.ndc explicitly establishes support for this recursion depth. Record the pending return check on a VM frame rather than recursively running the interpreter.
Useful? React with 👍 / 👎.
| } = self.vars.resolve(&self.types.callable_type(only)) => | ||
| { | ||
| Binding::Resolved(Candidate::Scalar(only)) |
There was a problem hiding this comment.
For a named function with an Iterable<T> parameter, promoting the sole dynamic candidate to a direct call drops the runtime check of that annotation. For example, fn consume(xs: Iterable<Int>) => 0; let a: Any = true; print(consume(a)); now accepts the non-sequence and prints 0, whereas dynamic dispatch previously rejected it. lower_parameter_annotation represents this parameter as a type variable plus an iteration constraint, so commit_call binds its instantiated parameter to Any; expected_of_any consequently produces no effective guard. The copied iteration constraint also checks with no expression node and cannot emit a runtime guard. Preserve the callable's Iterable<Int> shape when generating argument checks before bypassing dynamic dispatch.
Useful? React with 👍 / 👎.
…rned functions 🔒 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abb6d79f8a
ℹ️ 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".
| FunctionFit::No | ||
| } else if **expected_return == StaticType::Any | ||
| || **return_type == StaticType::Never | ||
| || (return_type == expected_return && !has_unknowns(return_type)) |
There was a problem hiding this comment.
When an Any holds a fully typed callback returning a concrete sequence, dynamic overload dispatch now rejects it even though its signature proves compatibility. For example, fn h(f: Fn(Int) -> Iterable<Int>) => 1; fn h(x: Int) => 0; let a: Any = fn(x: Int) => [x]; print(h(a)); previously printed 1 but now finds no matching overload. List<Int> satisfies Iterable<Int> in both StaticType::matches and TypeVars::bind_argument, but this equality-only test classifies the callback as IfChecked; Value::is_a consequently rejects it during dispatch. Recognize provably compatible iterable return types without requiring a wrapper.
Useful? React with 👍 / 👎.
| let StaticType::Function { return_type, .. } = typ else { | ||
| unreachable!("a checked function is used as a function type"); | ||
| }; | ||
| match self.dispatch_call(Rc::unwrap_or_clone(function), args)? { |
There was a problem hiding this comment.
When a checked cast widens a parameter to Any, the wrapper advertises that widened signature to dynamic dispatch, but this call never validates arguments against the underlying function's actual parameters. For example, let n: Int = 0; fn store(x: Int) { n = x; 0 } let a: Any = store; let g: Any = a as Fn(Any) -> Int; g(true); print(n); now prints true, corrupting the Int binding. Previously, dynamic dispatch of g(true) rejected the argument against store's original Int parameter. Either reject incompatible parameter widening when constructing the wrapper or check the underlying parameters before executing its body.
Useful? React with 👍 / 👎.
| Self::Function(f) => { | ||
| state.write_u8(9); | ||
| match f { | ||
| match f.unchecked() { |
There was a problem hiding this comment.
Stripping the checked type makes differently checked versions of one function hash identically, and Vm::dispatch_call uses that hash alone as its memoization key. Consequently, even a pure identity function can return a wrapper with the wrong contract: pure fn cached(f: Any) => f; fn id(x) => x; let a: Any = id; let ints = a as Fn(Int) -> Int; let strs = a as Fn(String) -> String; cached(ints); let s: Fn(String) -> String = cached(strs); print(s("ok")); fails because the second call retrieves the cached ints wrapper instead of strs. Include checked-contract information in the memoization-specific key while preserving the intended language-level equality and hash consistency.
Useful? React with 👍 / 👎.
…urns 🔒 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…out 📦 (#353) Closes #331. `Any` was gradual: a value of type `Any` went anywhere a type was expected, with a check the analyser inserted, and a call with an `Any` argument picked its overload as the program ran. Deciding where those checks went was spread over many places, and every new feature opened another hole (#328, #329, the `op=` and slice-write holes, function values through `Any`). Now `Any` is a top type, like Rust's `Box<dyn Any>`, Kotlin's `Any` or TypeScript's `unknown`: ```ndc let a: Any = 5; // anything goes into Any let n: Int = a; // error: found Any but expected Int let m = a as Int; // the way out: fails the program if it isn't one print((a as? Int).is_some(), a); // a test, and anything that takes Any ``` - A value of type `Any` only goes where `Any` is taken. Passing, storing, iterating, calling it or using an operator on it is an error unless an overload takes `Any` there, with help to cast it with `as` or test it with `as?`. A call with an `Any` argument only considers overloads that take `Any` at that position. Inside containers `Any` is exact, as before. - Enforced in `bind_argument`, `may_fit`, `type_parameters_agree`, `could_take`/`could_be_callable` and iteration. Failures give the error type from #348, so each mistake is one error. - `print`, `dbg` and the variadic `zip` take each argument as its own type instead of turning it into `Any`; no built-in signature changed. - Two weaknesses the permissive `Any` had hidden are fixed: a generic function's own body (what runs for a function value) picks its calls with type parameters rather than `Any`, which kept `+` on tuples working there; and a literal whose element type became a parameter's type no longer falls back to `Any`. Tests: 16 became error tests, 26 now cast with `as`, 4 got an annotation instead of a stray `Any`, and 6 that would have silently changed which overload runs were rewritten with a type test. The manual's sections on `Any` (types, casts, augmented assignment, function values, the overload troubleshooting page) describe the explicit rule. All advent-of-brian solutions give their answers; in advent-of-code-ndc two more programs need an annotation (a loop that falls off the end into `print`, and a map default `%{:fn() => []}` whose list doesn't learn from later pushes). Analysis time and VM instruction counts are unchanged. Known gaps, for follow-ups: values the analyser can't type still fall back to `Any` and then show up in errors (a lambda nothing calls, a REPL line `let xs = [];`), and a user function whose body ties two parameters together accepts `f(a, 1)` but not `f(1, a)` with `a: Any`. The runtime checks the analyser used to insert for `Any` (`ExpectType`, `type_checks`, the element-scanning pass of runtime overload picking) are no longer reachable and go next. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
A value of type
Anyused as a function type was only checked for how many arguments the function takes, so a call through it could return anything:A function is now checked against what its own type says it takes and returns, and when what it returns isn't known, its calls are checked.
let fwith "expected Fn(Int) -> Int but found Fn(Any) -> String", as does a lambda that takes a String. AnAnyin the function's own type, as an unannotated parameter has, fits anything.Any, so most lambdas are decided without checking their calls.Function::Checkedwhen it's cast or passed where the function type is expected. Each call through it runs the function and checks its result: "expected the function to return Int but it returned String". It equals the function it checks.Anyarguments checked where they're passed. That's what gives a function argument passed to such a call its checks. A variable of typeAnybeing called, and a function with anIterable<T>parameter, whose kind isn't in its declared type, still have the program check the arguments.A checked call records the return type it expects on the callee's VM frame, which checks the result when it returns, so recursion through a checked function goes as deep as any other. A function it returns may get its own calls checked, and a function returning
Neverfits any return type. The new variant's type is boxed so that function values stay the same size; a call-heavy benchmark (fib(27)) runs in the same time as before (1.3% more instructions).The manual's casts page describes the checks.
🤖 Generated with Claude Code