[syntax-errors] Type parameter lists before Python 3.12 #16479

ntBre · 2025-03-03T19:11:10Z

Summary

Another simple one, just detect type parameter lists in type statements, functions, and classes. pyright only reports the type statement in the alias case, whereas we'll currently report both diagnostics, in combination with #16478.

Test Plan

Inline tests.

Summary -- Another simple one, just detect type parameter lists in `type` statements, functions, and classes. [pyright] only reports the `type` statement in the alias case, whereas we'll currently report both diagnostics, in combination with Test Plan -- Inline tests. [pyright]: https://pyright-play.net/?pythonVersion=3.8&strict=true&code=C4TwDgpgBAHg2gFQLpQLxQJYDthA

github-actions · 2025-03-03T19:20:07Z

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

NeilGirdhar · 2025-03-03T22:38:05Z

This is W2604 from Pylint. You may want to add it here and tick it off if you want 😄 I think your other PRs are similarly listed in Pylint.

ntBre · 2025-03-03T23:50:50Z

Ah, thank you! I've only been tracking them in #6591. I'll update #970 too.

MichaReiser · 2025-03-04T08:07:15Z

For reference: https://docs.python.org/3/reference/compound_stmts.html#type-parameter-lists

MichaReiser · 2025-03-04T08:08:08Z

crates/ruff_python_parser/src/error.rs

    Match,
    Walrus,
    ExceptStar,
+    TypeParams,


Should this be TypeParameterList or should we rename the message to type parameters?

It might also be helpful to start documenting the variants. E.g. by adding a link to the Python documentation https://docs.python.org/3/reference/compound_stmts.html#type-parameter-lists

TypeParameterList sounds good. I'll start documenting them too, I think that's a great idea. As in #16404, I included something similar to ruff rule docs in case we want to show these to users eventually.

dhruvmanila · 2025-03-04T11:24:59Z

pyright only reports the type statement in the alias case, whereas we'll currently report both diagnostics, in combination with #16478.

I think this is because the type statement itself was added in 3.12 so maybe it might be worth not raising this error for type statement as the entire statement is invalid?

dhruvmanila · 2025-03-04T11:26:02Z

crates/ruff_python_parser/src/parser/statement.rs

+        // test_ok type_params_py312
+        // # parse_options: {"target-version": "3.12"}
+        // type List[T] = list | set
+        // def foo[T](): ...
+        // class Foo[T]: ...
+
+        // test_err type_params_py311
+        // # parse_options: {"target-version": "3.11"}
+        // type List[T] = list | set
+        // def foo[T](): ...
+        // class Foo[T]: ...


Can we add a case with multiple type parameters? Or, we can just mix them up by using single type parameter in function and multiple type parameters in class.

I added an example from the UP046 tests with bounds and constraints, as well as an unpacked TypeVarTuple and a ParamSpec.

We just mark the whole list at once, so there aren't additional errors for each type parameter.

ntBre · 2025-03-04T17:52:35Z

pyright only reports the type statement in the alias case, whereas we'll currently report both diagnostics, in combination with #16478.

I think this is because the type statement itself was added in 3.12 so maybe it might be worth not raising this error for type statement as the entire statement is invalid?

I think that's fair. I can just hoist the check to where try_parse_type_params is called for classes and functions instead of keeping it in parse_type_params.

dhruvmanila · 2025-03-05T02:47:52Z

crates/ruff_python_parser/src/parser/statement.rs

+            // Only emit the `ParseError` for an empty parameter list instead of also including an
+            // `UnsupportedSyntaxError`.
+            if !type_params.is_empty() {


What's the reason for this condition? As a user I'd find it confusing if Ruff told me to first add the type parameters and then tell me to remove the entire list. Sorry if I missed this in my initial review if it was already present.

Oh that's a good point... I was just trying to avoid duplicates, but if anything, it might make sense to suppress the ParseError in this case. Removing the condition is certainly easiest though.

And you didn't miss it, I added it here in the revision when I added the empty test cases.

I think I'd prefer keep it both. I don't think we should suppress any syntax errors.

I know this goes against what I said earlier in #16479 (comment) but my argument for that would be that it's a new statement altogether. Maybe for now we can just avoid suppressing any syntax errors based on other syntax errors and we can act on it based on user feedback? Happy to hear others thoughts on this.

The parser does have some logic to avoid multiple errors at the same location for invalid syntax errors.

ruff/crates/ruff_python_parser/src/parser/mod.rs

Lines 424 to 438 in 37fbe58

fn inner(errors: &mut Vec<ParseError>, error: ParseErrorType, range: TextRange) {

// Avoid flagging multiple errors at the same location

let is_same_location = errors

.last()

.is_some_and(|last| last.location.start() == range.start());

if !is_same_location {

errors.push(ParseError {

error,

location: range,

});

}

}

inner(&mut self.errors, error, ranged.range());

But I agree that we should solve this holisticly because it will otherwise be very error prone to avoid all possible errors at the same location

Ah nice! I'll remove these checks for now, and then maybe we can add UnsupportedSyntaxErrors to that check or a similar one later.

dhruvmanila · 2025-03-05T02:47:59Z

crates/ruff_python_parser/src/parser/statement.rs

+            // Only emit the `ParseError` for an empty parameter list instead of also including an
+            // `UnsupportedSyntaxError`.
+            if !type_params.is_empty() {


Same as above.

dhruvmanila · 2025-03-05T02:59:21Z

crates/ruff_python_parser/src/error.rs

+    /// [type parameter list]:
+    /// https://docs.python.org/3/reference/compound_stmts.html#type-parameter-lists


Does this work? Shouldn't the link be after [..]: on the same line?

It appeared to work in Emacs, but I can put it on one line to be safe!

MichaReiser · 2025-03-05T07:27:56Z

crates/ruff_python_parser/src/parser/statement.rs

+            // Only emit the `ParseError` for an empty parameter list instead of also including an
+            // `UnsupportedSyntaxError`.
+            if !type_params.is_empty() {


The parser does have some logic to avoid multiple errors at the same location for invalid syntax errors.

ruff/crates/ruff_python_parser/src/parser/mod.rs

Lines 424 to 438 in 37fbe58

fn inner(errors: &mut Vec<ParseError>, error: ParseErrorType, range: TextRange) {

// Avoid flagging multiple errors at the same location

let is_same_location = errors

.last()

.is_some_and(|last| last.location.start() == range.start());

if !is_same_location {

errors.push(ParseError {

error,

location: range,

});

}

}

inner(&mut self.errors, error, ranged.range());

But I agree that we should solve this holisticly because it will otherwise be very error prone to avoid all possible errors at the same location

ntBre added parser Related to the parser preview Related to preview mode features labels Mar 3, 2025

ntBre requested review from MichaReiser and dhruvmanila as code owners March 3, 2025 19:11

MichaReiser approved these changes Mar 4, 2025

View reviewed changes

dhruvmanila reviewed Mar 4, 2025

View reviewed changes

ntBre added 5 commits March 4, 2025 12:29

document variant

5686e97

TypeParams -> TypeParameterList

1b259f1

include multiple type parameters (and all kinds) in class test

6bc9876

Merge branch 'main' into brent/syn-type-params

ae50ba8

revert default addition after merge

ed689c7

ntBre added 3 commits March 4, 2025 13:02

don't add a second error for type alias statements, reorganize tests

83b8d56

test that empty cases only give a ParseError

fc35635

clean up unused snapshots

61f2ad7

dhruvmanila approved these changes Mar 5, 2025

View reviewed changes

MichaReiser approved these changes Mar 5, 2025

View reviewed changes

ntBre added 2 commits March 5, 2025 08:11

remove is_empty checks and accompanying comments

1d6a6df

join reference link

a35bf2c

ntBre enabled auto-merge (squash) March 5, 2025 13:16

ntBre merged commit 81bcdce into main Mar 5, 2025
20 checks passed

ntBre deleted the brent/syn-type-params branch March 5, 2025 13:19

ntBre mentioned this pull request Mar 5, 2025

Emit diagnostics for new syntax as per the target Python version #6591

Closed

16 tasks

BrewTestBot mentioned this pull request Mar 7, 2025

ruff 0.9.10 Homebrew/homebrew-core#210160

Merged

	fn inner(errors: &mut Vec<ParseError>, error: ParseErrorType, range: TextRange) {
	// Avoid flagging multiple errors at the same location
	let is_same_location = errors
	.last()
	.is_some_and(\|last\| last.location.start() == range.start());

	if !is_same_location {
	errors.push(ParseError {
	error,
	location: range,
	});
	}
	}

	inner(&mut self.errors, error, ranged.range());

		/// [type parameter list]:
		/// https://docs.python.org/3/reference/compound_stmts.html#type-parameter-lists

[syntax-errors] Type parameter lists before Python 3.12 #16479

[syntax-errors] Type parameter lists before Python 3.12 #16479

Uh oh!

Conversation

ntBre commented Mar 3, 2025

Summary

Test Plan

Uh oh!

github-actions bot commented Mar 3, 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!

NeilGirdhar commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre commented Mar 3, 2025

Uh oh!

MichaReiser commented Mar 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvmanila commented Mar 4, 2025 • edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre commented Mar 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 Mar 3, 2025 •

edited

Loading

`ruff-ecosystem` results

NeilGirdhar commented Mar 3, 2025 •

edited

Loading

dhruvmanila commented Mar 4, 2025 •

edited by AlexWaygood

Loading

ntBre Mar 4, 2025 •

edited

Loading