Skip to content

Filter Mapper by source inference#9854

Open
Keemosty12 wants to merge 5 commits intostarkware-libs:mainfrom
Keemosty12:sourceguard
Open

Filter Mapper by source inference#9854
Keemosty12 wants to merge 5 commits intostarkware-libs:mainfrom
Keemosty12:sourceguard

Conversation

@Keemosty12
Copy link
Copy Markdown
Contributor

Mapper was mapping type/const/impl/negative-impl vars by local id alone, without checking mapping.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.

@reviewable-StarkWare
Copy link
Copy Markdown

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

@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.

@Keemosty12
Copy link
Copy Markdown
Contributor Author

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).

Copy link
Copy Markdown
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

@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.

@Keemosty12
Copy link
Copy Markdown
Contributor Author

@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…
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.

Copy link
Copy Markdown
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

@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:

  1. CanonicalImpl::new → via mapping.to_canonic: the value being mapped comes from a single inference context by construction
  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants