Skip to content

Conversation

@11happy
Copy link
Contributor

@11happy 11happy commented Oct 2, 2025

Summary

This PR implements semantic syntax error where alternative patterns bind different names

Test Plan

I have written inline tests as directed in #17412

@11happy
Copy link
Contributor Author

11happy commented Oct 5, 2025

test /ruff/crates/ty_python_semantic/resources/mdtest/import/star.md is failing as now two syntax errors are emitted at same line line 295 & 366, Can we add multiple syntax errors on the same line ?

Thank you

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thank you! This looks good overall. I just had a couple of suggestions about test cases and a bit of a simplification/possible performance improvement.

@ntBre
Copy link
Contributor

ntBre commented Oct 6, 2025

test /ruff/crates/ty_python_semantic/resources/mdtest/import/star.md is failing as now two syntax errors are emitted at same line line 295 & 366, Can we add multiple syntax errors on the same line ?

Yes, I think you can stack the comments, although you may need to move the existing one to the previous line like this:

    # error: [invalid-syntax] "name capture `E` makes remaining patterns unreachable"
    # error: [invalid-syntax] "your error here"
    case E | F:  
        ...

@11happy
Copy link
Contributor Author

11happy commented Oct 8, 2025

test /ruff/crates/ty_python_semantic/resources/mdtest/import/star.md is failing as now two syntax errors are emitted at same line line 295 & 366, Can we add multiple syntax errors on the same line ?

Yes, I think you can stack the comments, although you may need to move the existing one to the previous line like this:

    # error: [invalid-syntax] "name capture `E` makes remaining patterns unreachable"
    # error: [invalid-syntax] "your error here"
    case E | F:  
        ...

Sure, have added accordingly.
Thank you : )

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
@11happy 11happy force-pushed the alternate_pattern_binding branch from 0035102 to 95e7a77 Compare October 16, 2025 17:15
@11happy
Copy link
Contributor Author

11happy commented Oct 16, 2025

rebased : )

Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thank you!

I pushed two small commits, one using the full match pattern's range like CPython does. I think the narrower range could be helpful in the future, but the whole thing seems okay for now. And one renaming the error to DifferentMatchPatternBindings. I still don't totally love the name, but it's not user-facing, so we can always iterate on it if needed.

@ntBre ntBre changed the title [syntax-errors]: implement semantic syntax error alternate_binded_pattern [syntax-errors] Alternative match patterns bind different names Oct 17, 2025
@ntBre ntBre enabled auto-merge (squash) October 17, 2025 21:35
@ntBre ntBre merged commit 7198e53 into astral-sh:main Oct 17, 2025
37 checks passed
@henryiii
Copy link
Contributor

This change seems to now cause a SyntaxError on the following valid code that doesn't use different names:

invalid-syntax: alternative patterns bind different names
   --> src/sp_repo_review/checks/ruff.py:127:17
    |
125 |           match ruff:
126 |               case (
127 | /                 {"lint": {"select": x} | {"extend-select": x}}
128 | |                 | {"select": x}
129 | |                 | {"extend-select": x}
    | |______________________________________^
130 |               ):
131 |                   return cls.code in x or "ALL" in x
    |

See scientific-python/cookie#663

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