Filter Mapper by source inference#9854
Filter Mapper by source inference#9854Keemosty12 wants to merge 5 commits intostarkware-libs:mainfrom
Conversation
|
This change is |
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on Keemosty12).
a discussion (no related file):
why? add an affected test if this does anything.
Sure, I added regression tests for this. They cover exactly this case: Mapper now only rewrites vars from the current source inference context, and leaves vars from other inference contexts unchanged (even if local ids overlap). |
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on Keemosty12).
a discussion (no related file):
Previously, orizi wrote…
why? add an affected test if this does anything.
If no Cairo code can create this instance, this cannot happen and shouldn't be handled.
Making a case in rust by hand is meaningless.
I updated the regression tests to use variables allocated through real inference flows (new_*_var_raw) with overlapping local IDs across different inference_ids, so this now validates the actual runtime path rather than a hand-crafted Rust-only case. |
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on Keemosty12).
a discussion (no related file):
Previously, Keemosty12 (Keemo Styler) wrote…
I updated the regression tests to use variables allocated through real inference flows (new_*_var_raw) with overlapping local IDs across different inference_ids, so this now validates the actual runtime path rather than a hand-crafted Rust-only case.
Thanks for updating the tests to use real inference machinery. But the core concern remains: this scenario is structurally impossible via real code paths.
Mapper::map is only invoked in two places:
- CanonicalImpl::new → via mapping.to_canonic: the value being mapped comes from a single inference context by construction
- CanonicalImpl::embed → via mapping.from_canonic: the value is in canonical form, so all vars have InferenceId::Canonical
Neither path can produce a value containing vars from a different inference context. Your tests bypass this by calling Mapper::map directly, which is not a real usage scenario.
Adding these checks introduces dead code and, worse, silently changes the behavior when the invariant is violated — instead of surfacing the bug (via MapperError), it quietly leaves a foreign var unmapped. The right handling of an invariant violation is a panic or an error, not a no-op passthrough.
The PR should be closed.
3 participants
Mapperwas mapping type/const/impl/negative-impl vars by local id alone, without checkingmapping.source_inference_id.That is a correctness issue: vars from a different inference context can be treated as missing mappings (
FreeVariable) or be mismapped when local ids overlap.