Sitelet https://github.com/starkware-libs/cairo/pull/10183
Skip to content

bugfix(semantic): don't ICE on a Deref impl with the wrong number of associated types. - #10183

Merged
orizi merged 1 commit into
mainfrom
orizi/07-01-bugfix_semantic_don_t_ice_on_a_deref_impl_with_the_wrong_number_of_associated_types
Jul 3, 2026
Merged

bugfix(semantic): don't ICE on a Deref impl with the wrong number of associated types.#10183
orizi merged 1 commit into
mainfrom
orizi/07-01-bugfix_semantic_don_t_ice_on_a_deref_impl_with_the_wrong_number_of_associated_types

Conversation

@orizi

@orizi orizi commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

When resolving method calls that walk the deref chain, try_get_deref_func_and_target previously used unwrap() calls that would panic (ICE) if a Deref impl was missing its associated Target type. The function now uses ?-based error propagation and gracefully returns None when the impl definition data is unavailable or the associated type list does not contain exactly one entry.

A test case was added covering the scenario where a Deref impl omits type Target, verifying that the compiler emits a proper diagnostic (Not all trait items are implemented. Missing: 'Target'.) rather than crashing.

Type of change

Please check one:

  • Bug fix (fixes incorrect behavior)
  • New feature
  • Performance improvement
  • Documentation change with concrete technical impact
  • Style, wording, formatting, or typo-only change

Why is this change needed?

When a Deref impl was written without the required type Target associated type, the compiler would ICE (internal compiler error / panic) instead of reporting a user-facing diagnostic. This made it impossible to get actionable feedback from the compiler in this situation.

What was the behavior or documentation before?

Calling a method on a type whose Deref impl was missing type Target caused the compiler to panic with an unwrap failure inside try_get_deref_func_and_target while walking the deref chain.

What is the behavior or documentation after?

The compiler now emits a proper E0004 diagnostic (Not all trait items are implemented. Missing: 'Target'.) and continues to report method resolution failures normally, without crashing.

Related issue or discussion (if any)

N/A

Additional context

The fix replaces unwrap() calls with ?-based propagation and uses exactly_one() to guard against unexpected numbers of associated types, returning Ok(None) in error cases so the caller can handle the missing deref target gracefully.

@reviewable-StarkWare

reviewable-StarkWare commented Jul 1, 2026

Copy link
Copy Markdown

This change is Reviewable

@orizi Graphite App

orizi commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@orizi
bugfix(semantic): don't ICE on a Deref impl with the wrong number of …
c527ae0 
…associated types.

Resolving a method through the deref chain calls `try_get_deref_func_and_target`,
which assumed the resolved `Deref`/`DerefMut` impl has exactly one associated type
and `unwrap()`/`panic!`-ed otherwise. A `Deref` impl that omits `type Target` (or
adds an extra type) — reported elsewhere as a trait-item mismatch (E0004/E2014) —
crashed the compiler when the type was used through deref, before that diagnostic
could surface. Return `Ok(None)` (no usable deref)
@orizi
orizi force-pushed the orizi/07-01-bugfix_semantic_don_t_ice_on_a_deref_impl_with_the_wrong_number_of_associated_types branch from 366f39e to c527ae0 Compare July 1, 2026 16:02
@orizi
orizi marked this pull request as ready for review July 1, 2026 16:40
@cursor

cursor Bot commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Summary

Low Risk
Localized error-handling change in deref chain resolution during semantic analysis; behavior degrades gracefully instead of panicking, with a regression test.

Overview
Fixes an internal compiler error when method resolution walks a deref chain through a Deref impl that does not define type Target.

In try_get_deref_func_and_target, loading impl definition data, requiring exactly one associated type in item_type_asts, resolving that type, and applying generic substitution now use ? / maybe_as_ref() instead of unwrap() and a panic when there is not exactly one type. Invalid or incomplete impls return Ok(None) so deref lookup stops without crashing; users still get the existing missing trait item diagnostic (e.g. E0004 for missing Target) and normal method-resolution errors.

A deref diagnostic test was added for a Deref<MyBox> impl with only deref and no Target, calling .get() on MyBox.

Reviewed by Cursor Bugbot for commit c527ae0. Bugbot is set up for automated code reviews on this repo. Configure here.

TomerStarkware
TomerStarkware approved these changes Jul 2, 2026
View reviewed changes

@TomerStarkware TomerStarkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@TomerStarkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on eytan-starkware).

@orizi
orizi added this pull request to the merge queue Jul 3, 2026
Merged via the queue into main with commit 4315590 Jul 3, 2026
55 checks passed
@orizi
orizi deleted the orizi/07-01-bugfix_semantic_don_t_ice_on_a_deref_impl_with_the_wrong_number_of_associated_types branch July 5, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomerStarkware TomerStarkware TomerStarkware approved these changes
@eytan-starkware eytan-starkware Awaiting requested review from eytan-starkware

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@orizi @reviewable-StarkWare @TomerStarkware