bugfix(semantic): Detect non-instantiable array elements nested in a type. - #10175
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview
New attribute diagnostic tests cover Reviewed by Cursor Bugbot for commit 6271ff2. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
crates/cairo-lang-semantic/src/types.rs line 1003 at r1 (raw file):
/// `Array<T>` is only diagnosed once `T` is concretely disallowed. #[salsa::tracked(cycle_result=array_element_violation_cycle)] fn array_element_violation<'db>(
Consider removing the cyclic query entirely. is_phantom is already a query, and we can have the non-cyclic function be a query as well, covering most of the cases well
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed all commit messages and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi).
34a8d80 to
8da02ab
Compare
4934dd4 to
4d5cec3
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware).
crates/cairo-lang-semantic/src/types.rs line 1003 at r1 (raw file):
Previously, eytan-starkware wrote…
Consider removing the cyclic query entirely. is_phantom is already a query, and we can have the non-cyclic function be a query as well, covering most of the cases well
i considered - but i rather avoid this recalculation for most types.
the main case would not have that.
…type. Generalize the array-element check to recurse through a type's concrete structure - struct members, enum variants, tuple / snapshot / fixed-size-array / closure-capture components, the type wrapped by a `Box`/`Nullable`, and nested array elements - instead of inspecting only the top-level type. This catches a phantom or zero-sized array element reached through a wrapper, e.g. `Span<Ph>` and `Span<[(); 0]>` (which nest `Array<...>`), closing the gap where `.span()` of a zero-sized array gave no diagnostic (#9896). The search is a memoized query that recurses on itself for the common acyclic case. A type recursive through an array element makes the query cyclic; rather than a fixed `cycle_result` (which would cache a false negative for whichever participant is computed first), the cycle handler re-runs the same traversal with an explicit work-stack and visited set - it never re-enters the query, so every participant gets a complete result. Only concrete types are searched; a generic `Array<T>` is diagnosed once T is concretely a disallowed type.
8da02ab to
bb80200
Compare
4d5cec3 to
6271ff2
Compare
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on orizi).
Summary
Extends the phantom-type and zero-sized-array diagnostic checks to cover types that transitively contain a bad
Arrayelement, rather than only catching a directArray<Ph>orArray<()>. For example,Span<Ph>(which wrapsArray<Ph>) and mutually-recursive struct chains that eventually reach a phantom array element are now diagnosed correctly.The core change introduces a new
array_element_violationsalsa-tracked query that walks the full type structure — structs, enums, tuples, snapshots, fixed-size arrays, closures,Box/Nullablewrappers — looking for anArraywhose element is phantom or zero-sized. A dedicated cycle handler (array_element_violation_cycle) is added to handle types that are recursive through an array element (e.g.struct A { a: Array<A> }), using an explicit work-stack with a visited set to avoid salsa caching false negatives mid-cycle.New test cases cover:
Span<Ph>,Span<()>, a phantom array reachable through a cycle of array-recursive structs, and a self-recursive array type with no bad element (which must not produce a diagnostic).Type of change
Please check one:
Why is this change needed?
Previously, the diagnostic for phantom or zero-sized array elements was only triggered when the
Array<...>appeared directly as the type being checked. Wrapping the array in another type — most commonlySpan<Ph>, which is{base: Array<Ph>, len: usize}— silently bypassed the check, allowing invalid types to pass semantic validation without an error.What was the behavior or documentation before?
Span<Ph>and similar types that nest a badArrayinside a struct or other wrapper were accepted without any diagnostic.What is the behavior or documentation after?
Any type that transitively contains an
Arraywhose element is phantom or zero-sized is rejected with the appropriateE2019orE2053diagnostic, regardless of how deeply the array is nested. Self-recursive array types with no bad element continue to be accepted.Related issue or discussion (if any)
Additional context
The cycle handler is necessary because salsa's default cycle recovery would cache a
None(no violation) for the first type encountered in a cycle, even if a violation is reachable only after traversing the rest of the cycle. The iterative stack-based fallback avoids re-entering the query during cycle resolution.