[red-knot] Detect semantic syntax errors #17463

ntBre · 2025-04-18T15:37:03Z

Summary

This PR extends semantic syntax error detection to red-knot. The main changes here are:

Adding SemanticSyntaxChecker and Vec<SemanticSyntaxError> fields to the SemanticIndexBuilder
Calling SemanticSyntaxChecker::visit_stmt and visit_expr in the SemanticIndexBuilder's visit_stmt and visit_expr methods
Implementing SemanticSyntaxContext for SemanticIndexBuilder
Adding new mdtests to test the context implementation and show diagnostics

(3) is definitely the trickiest and required (I think) a minor addition to the SemanticIndexBuilder. I tried to look around for existing code performing the necessary checks, but I definitely could have missed something or misused the existing code even when I found it.

There's still one TODO around global statement handling. I don't think there's an existing way to look this up, but I'm happy to work on that here or in a separate PR. This currently only affects detection of one error (LoadBeforeGlobalDeclaration or PLE0118 in ruff), so it's not too big of a problem even if we leave the TODO.

Test Plan

New mdtests, as well as new errors for existing mdtests

crates/red_knot_python_semantic/resources/mdtest/comprehensions/basic.md

github-actions · 2025-04-18T15:39:13Z

`mypy_primer` results

No ecosystem changes detected ✅

ntBre · 2025-04-18T15:43:19Z

The mypy_primer results look like true positives to me, although it's a bit unfortunate because it seems clear that this file is meant to be split up somehow:

https://github.com/laowantong/paroxython/blob/44e14b17965c6b02871db4c9ed6acc8716c2740d/examples/idioms/programs_with_labels.py#L690-L695

Summary -- This PR extends semantic syntax error detection to red-knot. The main changes here are: 1. Adding `SemanticSyntaxChecker` and `Vec<SemanticSyntaxError>` fields to the `SemanticIndexBuilder` 2. Calling `SemanticSyntaxChecker::visit_stmt` and `visit_expr` in the `SemanticIndexBuilder`'s `visit_stmt` and `visit_expr` methods 3. Implementing `SemanticSyntaxContext` for `SemanticIndexBuilder` 4. Adding new mdtests to test the context implementation and show diagnostics (3) is definitely the trickiest and required (I think) some additions to the `SemanticIndexBuilder`. I tried to look around for existing code performing the necessary checks, but I definitely could have missed something or misused the existing code even when I found it. Test Plan -- New mdtests

This tracking looks a bit more complicated in the ruff `Checker`, but that's because it does some other checks at the same time. Every arm of its `match` where this flag is set ends up setting it.

github-actions · 2025-04-18T16:09:22Z

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

AlexWaygood

🥳

crates/red_knot_python_semantic/resources/mdtest/annotations/invalid.md

crates/red_knot_python_semantic/resources/mdtest/comprehensions/basic.md

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood

This is great, thank you!

crates/red_knot_python_semantic/src/semantic_index/builder.rs

crates/red_knot_python_semantic/resources/mdtest/annotations/invalid.md

crates/red_knot_python_semantic/resources/mdtest/diagnostics/semantic_syntax_errors.md

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

in favor of tracking this state in the SemanticSyntaxChecker itself

codspeed-hq · 2025-04-22T15:54:57Z

CodSpeed Performance Report

Merging #17463 will not alter performance

_{Comparing brent/semantic-errors-red-knot (931fcf1) with main (5407249)}

Summary

✅ 33 untouched benchmarks

ntBre · 2025-04-22T16:07:13Z

I'm guessing (hoping) the horrible performance degradation was from my naive use of is_file_open. I started caching that on the builder and also merged main in case it was something else.

ntBre · 2025-04-22T16:15:25Z

One more guess and then I'll set up for local benchmarks 😅 I'm not getting much from the salsa flamegraphs on codspeed.

MichaReiser · 2025-04-22T16:51:57Z

crates/red_knot_python_semantic/src/semantic_index/builder.rs

+    }
+
+    fn report_semantic_error(&self, error: SemanticSyntaxError) {
+        if self.db.is_file_open(self.file) {


You could consider changing the semantic_checker field on the SemanticIndexBuilder to be an Option<SemanticSyntaxChecker> and only initialize it with Some if the file is open. with_semantic_checker is a no-op if the checker is None.

MichaReiser · 2025-04-22T16:54:34Z

crates/red_knot_python_semantic/src/semantic_index/builder.rs

+    python_version: PythonVersion,
+    semantic_checker: SemanticSyntaxChecker,


We should move those fields into their own group (or to the builder state group at the top). The fields at the bottom are reserved for fields that match the fields on SemanticIndex

This reverts commit e32c188.

…tructor * origin/main: [red-knot] Trust module-level undeclared symbols in stubs (#17577) [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355) [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090) [red-knot] Detect semantic syntax errors (#17463) Fix stale diagnostics in Ruff playground (#17583) [red-knot] Early return from `project.is_file_open` for vendored files (#17580)

…var-instance * dcreager/generic-constructor: Revert FunctionLiteral type [red-knot] Trust module-level undeclared symbols in stubs (#17577) [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355) [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090) Clean this up a bit clippy [red-knot] Detect semantic syntax errors (#17463) Fix stale diagnostics in Ruff playground (#17583) [red-knot] Early return from `project.is_file_open` for vendored files (#17580)

Status -- This is a pretty minor change, but it was breaking a red-knot mdtest until #17463 landed. Now this should close #11934 as the last syntax error being tracked there! Summary -- Moves `Parser::validate_parameters` to `SemanticSyntaxChecker::duplicate_parameter_name`. Test Plan -- Existing tests, with `## Errors` replaced with `## Semantic Syntax Errors`.

@carljm

## Summary This is a first step toward `global` support in red-knot (#15385). I went through all the matches for `global` in the `mypy/test-data` directory, but I didn't find anything too interesting that wasn't already covered by @carljm's suggestions on Discord. I still pulled in a couple of cases for a little extra variety. I also included a section from the [PLE0118](https://docs.astral.sh/ruff/rules/load-before-global-declaration/) tests in ruff that will become syntax errors once #17463 is merged and we handle `global` statements. I don't think I figured out how to use `@Todo` properly, so please let me know if I need to fix that. I hope this is a good start to the test suite otherwise. --------- Co-authored-by: Carl Meyer <carl@astral.sh>

ntBre added the ty Multi-file analysis & type inference label Apr 18, 2025

ntBre commented Apr 18, 2025

View reviewed changes

crates/red_knot_python_semantic/resources/mdtest/comprehensions/basic.md Outdated Show resolved Hide resolved

ntBre added 8 commits April 18, 2025 12:00

add SemanticIndexBuilder::seen_module_docstring_boundary

74353f0

This tracking looks a bit more complicated in the ruff `Checker`, but that's because it does some other checks at the same time. Every arm of its `match` where this flag is set ends up setting it.

implement in_async_context and in_generator_scope

b09fcee

add failing mdtest

f37022a

emit diagnostics and pass mdtest

46b022e

pass existing mdtests

2764c45

add mdtests covering all context methods

507b737

upcast explicitly for msrv tests

60acba6

ntBre force-pushed the brent/semantic-errors-red-knot branch from bf6cc0c to 60acba6 Compare April 18, 2025 16:00

ntBre marked this pull request as ready for review April 18, 2025 16:50

ntBre requested review from AlexWaygood, MichaReiser, carljm, dcreager, dhruvmanila and sharkdp as code owners April 18, 2025 16:50

AlexWaygood reviewed Apr 18, 2025

View reviewed changes

ntBre and others added 2 commits April 18, 2025 16:27

avoid unrelated errors in invalid.md

63fd71d

avoid unrelated errors in comprehensions/basic.md

773e7a1

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood approved these changes Apr 18, 2025

View reviewed changes

remove backticks on async comprehension and test

c916bc3

This was referenced May 7, 2025

Add more tests for inference of async comprehensions with invalid syntax astral-sh/ty#121

Open

Update star.md tests to avoid syntax errors astral-sh/ty#120

Open

ntBre and others added 2 commits April 21, 2025 09:18

replace current_scope_is_global_scope with context method

82f848c

update notebook check

f6a4213

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

ntBre added 3 commits April 22, 2025 09:00

skip checking a file if it's not open

add4f6b

cache SemanticIndexBuilder::python_version

9ac43a9

add TypeCheckDiagnostic::extend_diagnostics

7db6685

ntBre mentioned this pull request Apr 22, 2025

[syntax-errors] await outside async functions #17363

Merged

remove SemanticSyntaxContext::seen_module_docstring_boundary

3d11ef7

in favor of tracking this state in the SemanticSyntaxChecker itself

ntBre added 2 commits April 22, 2025 12:03

cache is_file_open call

5e172cc

Merge branch 'main' into brent/semantic-errors-red-knot

af43757

revert is_file_open check entirely

7a1e1ab

restore is_file_open check, but only for pushing errors

b32372c

MichaReiser reviewed Apr 22, 2025

View reviewed changes

ntBre added 4 commits April 22, 2025 12:58

move semantic checker fields to their own group

02dfc23

make semantic_checker optional

e32c188

restore SemanticSyntaxContext::source method and use OnceCell

ebccf41

delete is_file_open check in favor of Option approach

57866a0

ntBre mentioned this pull request Apr 22, 2025

[red-knot] Add mdtests for global statement #17563

Merged

carljm removed their request for review April 23, 2025 05:46

MichaReiser mentioned this pull request Apr 23, 2025

Calling is_file_open degrades current query to durability LOW astral-sh/ty#114

Open

Revert "make semantic_checker optional"

931fcf1

This reverts commit e32c188.

ntBre merged commit e7f38fe into main Apr 23, 2025
34 checks passed

ntBre deleted the brent/semantic-errors-red-knot branch April 23, 2025 13:53

ntBre mentioned this pull request Apr 23, 2025

[syntax-errors] Make duplicate parameter names a semantic error #17131

Merged

ntBre mentioned this pull request May 8, 2025

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

Merged

		python_version: PythonVersion,
		semantic_checker: SemanticSyntaxChecker,

[red-knot] Detect semantic syntax errors #17463

[red-knot] Detect semantic syntax errors #17463

Uh oh!

Conversation

ntBre commented Apr 18, 2025

Summary

Test Plan

Uh oh!

Uh oh!

github-actions bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

ntBre commented Apr 18, 2025

Uh oh!

github-actions bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codspeed-hq bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #17463 will not alter performance

Summary

Uh oh!

ntBre commented Apr 22, 2025

Uh oh!

ntBre commented Apr 22, 2025

Uh oh!

MichaReiser Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Apr 18, 2025 •

edited

Loading

`mypy_primer` results

github-actions bot commented Apr 18, 2025 •

edited

Loading

`ruff-ecosystem` results

codspeed-hq bot commented Apr 22, 2025 •

edited

Loading