Skip to content

Comments

[semantic-syntax-tests] IrrefutableCasePattern, SingleStarredAssignment, WriteToDebug, InvalidExpression#17748

Merged
ntBre merged 9 commits intoastral-sh:mainfrom
maxmynter:semantic-test-coverage
May 9, 2025
Merged

[semantic-syntax-tests] IrrefutableCasePattern, SingleStarredAssignment, WriteToDebug, InvalidExpression#17748
ntBre merged 9 commits intoastral-sh:mainfrom
maxmynter:semantic-test-coverage

Conversation

@maxmynter
Copy link
Contributor

@maxmynter maxmynter commented Apr 30, 2025

Re: #17526

Summary

Add integration test for semantic syntax for IrrefutableCasePattern, SingleStarredAssignment, WriteToDebug, and InvalidExpression.

Notes

  • Following @ntBre's suggestion, I will keep the test coming in batches like this over the next few days in separate PRs to keep the review load per PR manageable while also not spamming too many.

  • I did not add a test for del __debug__ which is one of the examples in crates/ruff_python_parser/src/semantic_errors.rs:1051.
    For python version <= 3.8 there is no error and for >=3.9 the error is not WriteToDebug but SyntaxError: cannot delete __debug__ on Python 3.9 (syntax was removed in 3.9).

  • The blacken-docs bypass is necessary because otherwise the test does not pass pre-commit checks; but we want to check for this faulty syntax.

Test Plan

This is a test.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 30, 2025

mypy_primer results

No ecosystem changes detected ✅

@maxmynter maxmynter changed the title (tests) Add linter.rs and red knot integration tests [semantic-syntax-tests] Integration tests in linter and red knot for IrrefutableCasePattern, SingleStarredAssignment, WriteToDebug, and InvalidExpression Apr 30, 2025
@maxmynter maxmynter changed the title [semantic-syntax-tests] Integration tests in linter and red knot for IrrefutableCasePattern, SingleStarredAssignment, WriteToDebug, and InvalidExpression [semantic-syntax-tests] IrrefutableCasePattern, SingleStarredAssignment, WriteToDebug, InvalidExpression Apr 30, 2025
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

red-knot tests look good

@ntBre ntBre added the testing Related to testing Ruff itself label Apr 30, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 30, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

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.

Thanks! I had one suggestion about the yield from cases and a nit you can ignore if you prefer it this way.

@maxmynter maxmynter force-pushed the semantic-test-coverage branch from 1325207 to 18132e7 Compare May 6, 2025 21:40
maxmynter added 4 commits May 6, 2025 17:55
for "Irrefutable Case Pattern", "Single Starred Assignment",
"Write to Debug", and "Invalid Expression"
as tests were renamed to avoid name collision.
@maxmynter maxmynter force-pushed the semantic-test-coverage branch from 18132e7 to b2374b1 Compare May 6, 2025 21:59
@maxmynter
Copy link
Contributor Author

I went with the generic type parameter and pinned the necessary Python version, 3.12. Also rebased on top of main and resolved merge conflicts that came from the other PRs adding tests.

@AlexWaygood AlexWaygood removed their request for review May 6, 2025 22:17
@maxmynter maxmynter requested a review from ntBre May 6, 2025 22:22
@maxmynter
Copy link
Contributor Author

If this is good, it also closes #17526, I guess.

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.

Thanks! I was just going to push the small tweak to the parens in the ruff case, but then I noticed the mdtest code block issue. Could you update those? Then this should be ready to go.

maxmynter added 3 commits May 7, 2025 18:38
instead of python; so the code blocks actually run
@maxmynter maxmynter requested a review from ntBre May 7, 2025 22:52
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.

Thanks again! The new errors prompted one question, mostly for Alex or someone else on the ty team. I also had one suggestion about the "outside of a function" errors, but this is just about ready to go.

```py
# error: [invalid-type-form] "`yield` expressions are not allowed in type expressions"
# error: [invalid-syntax] "yield expression cannot be used within a TypeVar bound"
# error: [invalid-syntax] "`yield` statement outside of a function"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd probably just wrap this in an outer function to suppress these outside of a function errors since we only intend to test the other errors here. I think a lot of other mdtests use a function named _ to indicate this:

def _():
    type X[T: (yield 1)] = int

but it's not a huge deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is still something here. With

def _():
# error: [invalid-type-form] "`yield` expressions are not allowed in type expressions"
# error: [invalid-syntax] "yield expression cannot be used within a TypeVar bound"
    type X[T: (yield 1)] = int

def _():
# error: [invalid-type-form] "`yield` expressions are not allowed in type expressions"
# error: [invalid-syntax] "yield expression cannot be used within a type alias"
    type Y = (yield 1)

I still get the outside of a function errors

semantic_syntax_errors.md - Semantic syntax error diagnostics - Invalid expression

  crates/ty_python_semantic/resources/mdtest/diagnostics/semantic_syntax_errors.md:304 unexpected error: 16 [invalid-syntax] "`yield` statement outside of a function"
  crates/ty_python_semantic/resources/mdtest/diagnostics/semantic_syntax_errors.md:309 unexpected error: 15 [invalid-syntax] "`yield` statement outside of a function"

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's a bug, thank you! We can merge this as is, and I'll tackle it in a follow-up.

If you're curious, ty has separate scope kinds for type aliases and annotations, so the check for function scope needs to skip those instead of just checking that the immediate scope is a function. At least that looks like the easy fix to me.

Comment on lines +306 to +307
# error: [invalid-type-form] "`yield` expressions are not allowed in type expressions"
# error: [invalid-syntax] "yield expression cannot be used within a type alias"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't necessarily need to block this PR since you're just reporting the current state of things, but we might need to open a follow-up issue here to avoid duplicate diagnostics. @AlexWaygood do you know if the type error is separate from the syntax error here, or should we only pick one of these to emit?

Copy link
Contributor

Choose a reason for hiding this comment

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

I talked to Alex, and this is a symptom of a more general need to suppress type-checking errors when there are syntax errors present. I'll spin off an issue for this too, so no worries here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue already exists in the ty repo! astral-sh/ty#119

@maxmynter
Copy link
Contributor Author

One duplicate test case had slipped in, which i removed in 96aeedc.

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.

Thanks! And thanks for all of your help on #17526, I'll close that now.

@ntBre ntBre merged commit b4a1ebd into astral-sh:main May 9, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to testing Ruff itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants