fix(type-checker): enforce invariant generic arguments - #9007
Conversation
d913be7 to
ec2fd68
Compare
… checks Enforce declared generic variance when source and destination share the same class or when checking a generic base. Invariant arguments now require bidirectional assignability, with exemptions for UnknownType and LiteralString <-> str interop. Exempt compiler-internal list[UnionType] construction by materializing via TypeBase temp to keep the compiler's own sources invariant-compliant. Closes jaseci-labs#8580 Closes jaseci-labs#8550
ec2fd68 to
67619d8
Compare
…de list list[CodeBlockStmt] is not assignable to list[UniNode] under invariant checking. Materialize via explicit list[UniNode] to keep compiler sources passing with strict invariant generics.
Built-in containers with object/str element interop (list[object] vs list[str], dict[str,str] vs dict[str,object]) are common in compiler and stdlib. Treat them as covariant (one-way) to avoid false invariant failures while keeping strict invariant for user-defined element types (list[Dog] vs list[Animal]). Also fix IdentifiedStructType.get_declaration() return type to str.
…ments The builtin/stdlib exemption gave same-class generic arguments a one-way assignability check, so list[int] assignments to list[float] silently passed. Same-class arguments now always require bidirectional assignability: list[int] -> list[float] is diagnosed again while list[Dog] -> list[Animal] keeps its invariant error.
christianwilkins
left a comment
There was a problem hiding this comment.
Reviewed 2ebe6bb66a53b1235d3267c881047459dc2c0f20, including the full patch, both generic comparison paths, parser callers, existing discussion and current-head CI. Two remaining soundness holes are attached inline: object arguments bypass invariance, and union arguments fall back to a one-way comparison. These are incomplete cases of the defect this patch is intended to fix; the original two regression cases do pass.
Independent validation on #9010 head 725f41cfe: jac test jac/tests/compiler/passes/test_typevar.jac passes all 11 tests. Two additional jac check probes both incorrectly pass: list[Dog] passed to list[object] (whose body appends a string), and list[Dog | str] passed to list[Animal | str], with Dog(Animal). I compared the complete trees of #9007 and #9010: their compiler, parser, tests and other source are byte-identical; only the release-note filename differs. Thus this is one local validation run of their shared implementation, not two independent runs. git diff --check passes; no code changes were made during review.
The latest follow-up 2ebe6bb66 narrows the earlier builtin exemptions, but retains the object exception and non-ClassType fallback. Both PRs are conflict-free. Current-head CI is red because jac fmt --check --lintfix fails for type_evaluator.impl.jac and test_typevar.jac; the job logs include the required formatting patch.
Status: changes requested. Author work: enforce invariant assignment for these concrete argument forms in both same-class and generic-base paths, add the negative cases, apply formatting, and rerun the checks. This overlaps #9010 exactly, so coordinate which PR should land and close the duplicate after the chosen fix is accepted. No merge or closure was performed in this review.
| if src_arg.is_builtin("str") and dest_arg.is_builtin("LiteralString") { | ||
| continue; | ||
| } | ||
| if src_arg.is_builtin("object") or dest_arg.is_builtin("object") { |
There was a problem hiding this comment.
[P2] Do not exempt object from mutable-container invariance. On this implementation, def accept_objects(values: list[object]) -> None { values.append("not a dog"); } accepts a dogs: list[Dog] argument without any checker error. object is a concrete supertype, so the callee can insert values that violate the caller's element type. This branch skips the reverse assignment that would reject it. Preserve gradual Any/Unknown behavior separately and enforce both directions here and in the matching generic-base branch.
| } | ||
| continue; | ||
| } | ||
| if not self.assign_type(src_arg, dest_arg, strict) { |
There was a problem hiding this comment.
[P2] Apply the reverse check to union arguments too. The invariant check only becomes bidirectional when both arguments are ClassType. For Dog(Animal), def accept_union(values: list[Animal | str]) -> None {} still accepts dogs: list[Dog | str] with no checker error in my probe. Either argument being a UnionType reaches this one-way fallback, retaining the same mutable-container covariance hole. Add this negative case and use the invariant rule for concrete non-ClassType arguments in both comparison paths.
Unwrap else-after-return, fold nested if-in-else to elif, and drop dead strings flagged by the docstring placement rule so the scoped fmt gate passes on this PR's changed files.
Returning list[EdgeAnchor] from a list[Anchor]-typed function fails under invariant generic argument checking. Return an explicit list[Anchor] copy, matching the JsxSlot materialization pattern.
Greptile SummaryThe PR enforces declared variance while comparing generic class arguments and adds regression coverage for invariant mutable containers.
Confidence Score: 5/5The PR appears safe to merge; no new issue was introduced after the previous review, and the earlier finding was manually resolved. The current review delta contains no changes after the previous review SHA, so there are no new regressions to report. The sole previous finding was manually resolved without explanation and therefore is not outstanding.
|
| Filename | Overview |
|---|---|
| jac/jaclang/compiler/types/type_evaluator.impl/type_evaluator.impl.jac | Enforces covariant, contravariant, and bidirectional invariant argument checks for same-class and generic base-class assignment. |
| jac/tests/compiler/passes/fixtures/checker/checker_invariant_generics.jac | Provides representative numeric and class-hierarchy substitutions that invariant mutable containers must reject. |
| jac/tests/compiler/passes/test_typevar.jac | Verifies that both unsound invariant substitutions emit assignment diagnostics. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Compare source and destination class types] --> B{Same class or generic base?}
B -->|No| C[Continue inheritance matching]
B -->|Yes| D{Declared parameter variance}
D -->|Covariant| E[Check source argument → destination argument]
D -->|Contravariant| F[Check destination argument → source argument]
D -->|Invariant| G[Check both directions]
E --> H[Accept only if check succeeds]
F --> H
G --> H
Reviews (3): Last reviewed commit: "ci: retrigger jac-check type sweep" | Re-trigger Greptile
The whole-program type sweep flags list[Subclass] assigned or passed where list[Base] is expected under strict generic-argument invariance. Materialize freshly typed lists at the affected sites (normalize_pass, parameter_type_check, pyast_load_pass, ownership_check_pass markers/core, jcir_gen_pass decls/stmt/module/osp), retype locals where every consumer agrees, and preserve None-valued variants exactly.
Closes #8580
Fixes #8550
Summary
list[int]tolist[float]andlist[Dog]tolist[Animal].Root cause
_assign_classcompared same-class generic arguments only assource -> destination. The generic checking implementation introduced this behavior in225a2c9176(#5159), so the defect was pre-existing onupstream/main.Verification
cd jac && JAC_TEST_JOBS=0 jac test tests/compiler/passes/test_typevar.jac— 10 passed.appendnow reports two expected E1053 errors.