Repository navigation
fix: bugs found analysing the advent-of-code-ndc solutions 🎄 - #342
Conversation
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: 096dab5ff0
ℹ️ 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".
| self.groups | ||
| .iter() | ||
| .flatten() | ||
| .map(|&symbol| Candidate::Scalar(symbol)) | ||
| .collect(), |
There was a problem hiding this comment.
When a generic function is stored as a value, it runs its shared body rather than a specialized instance, so this candidate ordering determines the overload actually called. Resolver groups are newest-first, but the VM's find_scalar_overload takes the first matching candidate; flattening them without ordering by specificity lets a newer Any fallback override an older exact overload. For example, fn choose(x: Int) => "int"; fn choose(x: Any) => "any"; fn apply(x) { let g = choose; g(x) } let h: Fn(Int) -> String = apply; print(h(1)); selects the Int overload during analysis but prints "any" at runtime. Order candidates within each scope by specificity, as ordinary dynamic call resolution does.
Useful? React with 👍 / 👎.
…ity 🎯 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
…ments 📏 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ed947139d
ℹ️ 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".
| Constraint::FunctionValue(value) if waiting.origin.is_none() => { | ||
| let binding = self.every_overload(value); | ||
| self.picks.bindings.insert(value.id(), binding); |
There was a problem hiding this comment.
When a generic function is used as a value, its shared body now passes an Object::OverloadSet wherever an overloaded callback remains deferred, but native higher-order functions cannot consume that representation: NdcType::MutVmCallable in ndc_macros/src/vm_convert.rs accepts only Object::Function. For example, fn signs(xs) => xs.map(signum); let f: Fn(List<Int>) -> List<Int> = signs; print(f([1, -2])); passes analysis but fails at runtime instead of returning [1,-1]. Direct calls use specialized bodies, so the added signs test misses this path. Make deferred function values usable by native callbacks before storing them as dynamic bindings here.
Useful? React with 👍 / 👎.
| else { | ||
| return true; |
There was a problem hiding this comment.
When the argument's literal variable holds another unsettled literal, literal_defaults returns None, so this branch accepts the argument without establishing any relationship to the iterable parameter. For example, in len([1][0]), the indexed element initially holds the unsettled numeric literal and is accepted here, even though it subsequently defaults to Int. iterations_waited_for in solver.rs explicitly excludes literal variables, so no deferred iteration constraint validates it later; debug builds instead reach the early-overload consistency assertion, while release builds leave the invalid call to fail at runtime. Keep the iterable requirement pending until the held literal's type is known rather than returning unconditional success.
Useful? React with 👍 / 👎.
… to the program 🔁 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: 7d53248764
ℹ️ 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".
| Constraint::FunctionValue(value) if waiting.origin.is_none() => { | ||
| let binding = self.every_overload(value); | ||
| self.picks.bindings.insert(value.id(), binding); |
There was a problem hiding this comment.
When a deferred function value contains locally declared overloads and escapes into another function, the emitted OverloadSet retains frame-relative variable locations rather than the functions themselves. For example, fn make(x) { fn choose(y: Int) => "int"; fn choose(y: Float) => "float"; let g = choose; fn() => g(x) } let m: Fn(Int) -> Fn() -> String = make; let h = m(1); print(h()); uses the shared generic body and captures this set in h. However, find_scalar_overload resolves its candidates relative to the currently executing closure's frame, not make's frame, so it reads unrelated slots or runs out of bounds. Materialize or capture the candidate functions when constructing an escaping overload value.
Useful? React with 👍 / 👎.
| Object::Function(function) => return function.fit(target) == FunctionFit::Proven, | ||
| // The overloads of a function the analyser couldn't pick | ||
| // from, of which the program picks one for each call. | ||
| Object::OverloadSet { .. } => return true, |
There was a problem hiding this comment.
When a deferred overload value flows through Any, this unconditional success allows it to satisfy incompatible function annotations without checking parameters or return values. For example, fn bad(x) { let g = abs; g(x); let a: Any = g; let b: Fn(Int) -> String = a; b(1) } let h: Fn(Int) -> String = bad; print(h(1)); runs the shared generic body, accepts the overload set as Fn(Int) -> String, and returns an integer instead. converted_to only creates checked wrappers for Object::Function, so overload sets bypass the existing return-type enforcement. Resolve a compatible overload or retain a checked function-type wrapper instead of accepting every function type.
Useful? React with 👍 / 👎.
| Binding::Dynamic( | ||
| value |
There was a problem hiding this comment.
When a shared generic body passes a deferred overload value to a pure fn, memoization hashes that value before executing the callback, but Object::hash still panics for every OverloadSet. For example, pure fn invoke(g, x) => g(x); fn apply(x) => invoke(abs, x); let h: Fn(Int) -> Int = apply; print(h(1)); now passes analysis but reaches the Rust panic OverloadSet cannot be used as a map key in Vm::dispatch_call, rather than producing 1. Direct specialized calls avoid this representation, so they do not cover the failure. Give deferred function values a supported hash/identity representation, or resolve them before the memoization cache hashes arguments.
Useful? React with 👍 / 👎.
…rdinary function 🧩 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: 849bf4e177
ℹ️ 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".
| .find(|candidate| candidate.matches_value_args(arguments)) | ||
| .or_else(|| { | ||
| candidates | ||
| .iter() | ||
| .find(|candidate| candidate.conforms_value_args(arguments)) |
There was a problem hiding this comment.
When a deferred function value has a typed-container overload and an Any fallback, the first pass selects the fallback before the more-specific candidate receives its full conformance check. Value::matches_param deliberately rejects typed containers such as List<Int>, so even correctly ordered candidates produce the wrong result. For example, fn choose(x: List<Int>) => "ints"; fn choose(x: Any) => "any"; fn apply(x) { let g = choose; g(x) } let h: Fn(List<Int>) -> String = apply; print(h([1])); selects the list overload during analysis but prints "any" through the shared body. Perform the necessary container check before advancing to a lower-priority candidate rather than only after every fast-path candidate fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This matches how every call the program picks for works, not just function values: the cheap first pass looks only at what kind each value is, and an overload whose parameter needs a scan of a container is tried only when nothing takes the arguments without one. An ordinary call in the same shared body picks the same way: with fn direct(x) => choose(x); let k: Fn(List<Int>) -> String = direct; print(k([1])); it also prints "any". The manual documents this (troubleshooting, "Element types cost a scan"). Changing the order would make function values pick differently from calls, and changing it for calls too would put a container scan on every call the program picks for, so it is left as it is here.
Analysing every program in the advent-of-code-ndc repository turned up three things the type checker got wrong. Five programs that failed analysis now pass, including one master rejected too.
Iterable<T>became anIterableitself. Binding a literal's still-open type to anIterable<T>parameter unified the two, so the value's type becameIterable<String>, which only a parameter may be. Inlet cur, path = ("A", ["A"]); path.len; path ++ [cur]the++then found no overload. Such a literal is now settled as the sequence it holds (aList<String>here), and its elements are bound toT.fn signs(xs) => xs.map(signum)reported that nothing says whichsignumis meant, though each call's types do. A function value now waits like a call: it's part of the function's scheme, each instance picks the overload its types call for, and the body compiled forAnyarguments leaves the pick to the program. There the value is a new kind of function,Function::Overloads, made by a newMakeOverloadsinstruction from the overloads looked up where the value is made, most specific first; each call calls the first that accepts its arguments. Being an ordinary function, it can be passed to built-ins likemap, captured, cached by apure fn, and checked against a function type. Copying the scheme to a call now gives the value the call's types, and picking it counts as settling what it returns.firstandlastonly took lists. Their implementation takes any slice, and programs call them on pairs:.map(first)over(point, flag)tuples. They now also take tuples of one to twelve elements, typed by the element they return, andzipgives typed tuples for up to twelve sequences (it went up to four). A signature can't say "a tuple of any length", so each length gets its own overload; twelve is where Rust's standard library stops for tuples.The manual's section on overloaded function values covers generic functions.
🤖 Generated with Claude Code