fix(analyser): use type overlap instead of subtyping for dispatch feasibility ➿ - #209
fix(analyser): use type overlap instead of subtyping for dispatch feasibility ➿#209timfennis wants to merge 1 commit into
Conversation
![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: daa3ba4b70
ℹ️ 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".
| (StaticType::Tuple(left), right) => left.iter().all(|left| right.is_subtype(left)), | ||
| (left, right) => right.is_subtype(left), | ||
| (StaticType::Tuple(left), right) => left.iter().all(|left| right.overlaps(left)), | ||
| (left, right) => right.overlaps(left), |
There was a problem hiding this comment.
When the RHS is a supertype of the target, overlap is insufficient to preserve the target's concrete type. For example, let values: List<Int> = [1]; let rhs: List<Number> = [0.5]; values ++= rhs; now passes this gate because List<Int> overlaps List<Number>, but the native ++= implementation appends the float without checking element types while the analyser continues treating values as List<Int>. This silently violates the annotation and can make subsequent statically resolved operations receive the wrong runtime values; only unknown types such as Any should be deferred, while known RHS types still need the previous subtype guarantee.
Useful? React with 👍 / 👎.
| | Self::MaxHeap(u) | ||
| | Self::Deque(u), | ||
| Self::Sequence(t), | ||
| ) => t.overlaps(u), |
There was a problem hiding this comment.
For a widened Sequence<Int> argument and a List<Number> parameter, this branch now creates a dynamic candidate because a runtime List<Int> could satisfy both. However, Value::matches_param explicitly returns false for every typed List, Sequence, Map, or Tuple parameter during dynamic dispatch. Thus a value widened from an iterator and then assigned [1], for example, is accepted by the analyser when passed to fn consume(xs: List<Number>) but is guaranteed to fail overload resolution even though its runtime value matches. The runtime matcher must be able to verify the typed-container cases enabled here, or this overlap must not advertise them as dispatchable.
Useful? React with 👍 / 👎.
daa3ba4 to
0e43d20
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e43d20079
ℹ️ 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".
| matches!((key.as_ref(), value.as_ref()), (Self::Any, Self::Any)) | ||
| } | ||
| Self::Tuple(elements) => elements.is_empty(), | ||
| _ => true, |
There was a problem hiding this comment.
The newly added runtime-checkability guard still falls through to true for Iterator<T>, MinHeap<T>, and MaxHeap<T> with concrete T. For example, let xs: Sequence<Int> = 1..3; consume(xs) with an Iterator<Number> parameter is retained as a dynamic candidate because the types overlap, but Value::matches_param falls back to the runtime type Iterator<Any>, which cannot match Iterator<Number>, so the call is guaranteed to fail at runtime rather than being rejected by the analyser. The heap runtime types similarly erase their elements to Any; these variants should only be considered checkable when their element type is Any.
Useful? React with 👍 / 👎.
…sibility ➿ Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0e43d20 to
1ab78d3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab78d352d
ℹ️ 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::String.overlaps(t) | ||
| } | ||
| (Self::Sequence(t), Self::Tuple(elems)) | (Self::Tuple(elems), Self::Sequence(t)) => { | ||
| elems.iter().all(|elem| elem.overlaps(t)) |
There was a problem hiding this comment.
When elems is empty, all is vacuously true, so Sequence<Int> is considered to overlap (). A call such as fn consume(x: ()) => x; let xs: Sequence<Int> = [1]; consume(xs) is therefore retained for dynamic dispatch, although no Sequence<Int> value can satisfy the unit parameter and Value::matches_param rejects every Tuple parameter. This turns a statically provable mismatch into a guaranteed runtime “no matching overload” error; special-case the empty tuple before applying the element-wise test.
Useful? React with 👍 / 👎.
|
Superseded by #215, which combines this with the other branch so the precursor lands first. All review findings from this PR are addressed there. |
Replaces #209 and #213. ## What changed The `as` operator asserts that a value has a type without converting it. The analyser removes checks it can prove from the operand's static type. The VM checks the remaining casts at the cast site. Container checks scan nested elements, stop on cyclic containers, and validate map defaults because a missing-key lookup inserts the default. The analyser rejects a cast only when the operand and target types cannot share a value. `List<Any>` and `Sequence<Int>` can both contain the same `List<Int>`, so that cast reaches the runtime check. `StaticType::overlaps` models this rule. `Never` overlaps no type because it has no values. Dispatch keeps its existing subtype rules. Users can cast widened values before calls that need a concrete container type. The analyser adds a cast hint when a same-arity overload could accept narrower argument types; unknown names, wrong arity, and disjoint argument types keep the existing error. The language grammar, tree-sitter grammar, TextMate grammar, CLI highlighter, LSP traversal, completion list, and diagnostics all understand `as`. Both parsers prefer generic arguments when the tokens form a complete type, and otherwise leave `<`, `>`, `>=`, and `>>` to the expression parser. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Context
Running the advent-of-brian suite against 0.3.0 surfaced two analyser regressions — programs that ran fine on 0.2.1 were rejected at compile time:
2025/11/part1+2:line = line.split(" ")widenslinetoSequence<String>, after whichline.remove(0)hard-errors with "No function called 'remove' found" (regressed in feat: add struct definitions with field access and assignment 🏗️ #197'scould_accept_callfilter).2025/09/part2:new ++= ranges.pop()[1]has anAny-typed RHS and is rejected with "found Any but expected List" (regressed in fix: fixed augmented assignment 🦾 #192's eligibility gate).Both share one root cause:
is_incompatible_withwas defined as "no subtype relation in either direction", but dispatch feasibility needs type overlap — "could any runtime value satisfy both types?". Those differ in the mixed-direction case:Sequence<String>andList<Any>are not subtypes of each other, yetList<String>inhabits both. AndAnyoverlaps everything, so an unknown operand must defer to runtime dispatch rather than be rejected.Changes
ndc_core: newStaticType::overlaps— structural common-inhabitant test mirroringis_subtype, handling mixed-direction sequence-family relationships. Element types are treated as inhabited, soList<Int>vsList<String>stays disjoint.is_incompatible_withis now!overlaps(this makesfind_function_candidatesandcould_accept_calloverlap-based).ndc_analyser:augmented_rhs_is_compatibleusesoverlapsinstead ofis_subtype, so specializedop=bindings are only rejected on a provable mismatch.overlaps, plus two functional tests covering both reported failures.Notes for reviewers
The deliberately-deferred strictness from the #192 review is untouched:
[1] ++= ["two"]and the map-=/&=cases still reject, because those types are provably disjoint. All 552 tests pass and every advent-of-brian puzzle now runs on this branch.🤖 Generated with Claude Code