Skip to content

Conversation

@giladchase
Copy link
Contributor

fix(semantic): incorrect diagnostics for var redefinition in patterns

Either no diagnostics was printed, or an incorrect diagnostic "Missing variable in pattern."
was printed.

fix(semantic): add diagnostic for duplicate var names in pattern

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion


-- commits line 2 at r2:
Added this commit before the fix to demonstrate the previous, incorrect diagnostics.

I'm BLOCKING to remind myself to squash it before merging.

Code quote:

- 73f5bd7: fix(semantic): incorrect diagnostics for var redefinition in patterns

Copy link
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.

:lgtm:

@orizi reviewed 1 of 2 files at r1, 2 of 4 files at r2.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @giladchase and @TomerStarkware)


-- commits line 2 at r2:

Previously, giladchase wrote…

Added this commit before the fix to demonstrate the previous, incorrect diagnostics.

I'm BLOCKING to remind myself to squash it before merging.

PR adding failing tests is fine.


crates/cairo-lang-semantic/src/expr/test_data/pattern line 319 at r1 (raw file):

error: Missing variable in pattern.
 --> lib.cairo:3:9
        (a, a) => a,

is this caused by the duplication - or by the usage - unclear from the test.

Code quote:

        (a, a) => a,

@giladchase giladchase force-pushed the gilad/10-29-parser_add_parser_test_for_double_reference_to_t branch from 3f2fede to 829cbf3 Compare November 4, 2025 15:11
@giladchase giladchase force-pushed the gilad/10-30-fix_semantic_incorrect_diagnostics_for_var_redefinition_in_patterns branch from 8467b32 to bc645d4 Compare November 4, 2025 15:11
Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)


crates/cairo-lang-semantic/src/expr/test_data/pattern line 319 at r1 (raw file):

Previously, orizi wrote…

is this caused by the duplication - or by the usage - unclear from the test.

Right, removed unnecessary stuff to clarify.

Copy link
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.

:lgtm:

@orizi reviewed 4 of 4 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware)

Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@TomerStarkware reviewed 1 of 4 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase changed the base branch from gilad/10-29-parser_add_parser_test_for_double_reference_to_t to main November 5, 2025 10:55
Gilad Chase added 2 commits November 5, 2025 13:30
Either no diagnostics was printed, or an incorrect diagnostic "Missing variable in pattern."
was printed.
@giladchase giladchase force-pushed the gilad/10-30-fix_semantic_incorrect_diagnostics_for_var_redefinition_in_patterns branch from bc645d4 to 5c3054c Compare November 5, 2025 11:30
@giladchase giladchase added this pull request to the merge queue Nov 5, 2025
Merged via the queue into main with commit 36f67cc Nov 5, 2025
54 checks passed
@orizi orizi deleted the gilad/10-30-fix_semantic_incorrect_diagnostics_for_var_redefinition_in_patterns branch November 5, 2025 14:44
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.

5 participants