Start detecting version-related syntax errors in the parser

[ntBre]

Summary

This PR builds on the changes in #16220 to pass a target Python version to the parser. It also adds the Parser::syntax_errors field, which collects version-related syntax errors while parsing. These syntax errors are then turned into Messages in ruff (in preview mode) or SyntaxDiagnostics in red-knot.

This PR only detects one syntax error (match statement before Python 3.10), but it has been pretty quick to extend to several other simple errors (see #16308 for example).

Test Plan

The current tests are CLI tests in the linter crate, but these could be supplemented with inline parser tests after #16357.

I also tested the display of these syntax errors in VS Code:

[github-actions]

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.

[MichaReiser]

We shouldn't use LintName here. The idea of the other error codes is to be mainly static.

[ntBre]

Do you mean this should be Option<&'static str>, for example, or that I should use a new enum variant entirely?

[MichaReiser]

That's probably no longer relevant. But the idea is that the ErrorCodes struct explicitly lists all error codes. The one exception to this are lint rules, because we want to define them in different crates.

So what I'm suggesting is that you change this to ParenthesizedWithItemsPre310 (or whatever the error code should be). But I think this is no longer necessary, now that we map all version related syntax error to syntax error.

[MichaReiser]

I haven't reviewed the rest of the PR but I'd be interested to understand what the motivation is for InvalidSyntax(Option<LintName>) over just using SyntaxError.

[ntBre]

I don't really remember the motivation, which is probably not a good sign 😅 I think I just needed a DiagnosticId to implement Diagnostic for SyntaxDiagnostic and this looked like the best way to go. I think I was still picturing giving each one a name/rule code when I wrote this originally like in some of the other prototypes.

Should I just reuse the original, unit InvalidSyntax variant? Or did you mean something else by "over just using SyntaxError?"

This has been confusing to me for some reason, sorry about that. I guess I don't fully understand what these are used for, or at least I certainly didn't understand when I wrote this initially. Seeing the mdtest errors helped to clear that up a bit, I think.

[AlexWaygood]

One reason why we might want to have different error codes for different syntax errors is so that we can provide more detailed documentation for some syntax errors. This isn't much of a concern with the specific syntax error that @ntBre is adding here, as there's not much more to say than "you can't use the match statement on Python <3.10".¹. However, as we discussed on Brent's design proposal, it will be a concern with other syntax errors that we'll want to detect in the future -- for example, we have very nice docs for F404 currently in Ruff, and it would be a shame to provide worse docs for red-knot users when we start detecting that syntax error in our brand new tool.

Some errors that we may want to provide per-error docs for are errors that only appear on older or newer Python versions. For example, the details around when you're able to parenthesize context managers on Python <3.9 are pretty subtle. So are the details on when match patterns are considered irrefutable (it's a syntax error to have more than one irrefutable pattern in a match statement on Python 3.10+).

@MichaReiser also pointed out in the comments on Brent's design doc that there are existing syntax errors the parser detects that probably might also benefit from better docs.

(To be clear, I'm not saying that we have to have multiple error codes for these syntax errors. And there are obviously costs as well as benefits, which Micha outlined well. But this is one possible motivation for why it might help to have different error codes for different syntax errors: it would allow users to easily look them up in our documentation.)

Footnotes

1. That said, I actually don't think it would hurt to have a documentation page that links to the relevant PEPs that introduced the match statement, and/or the Python docs for the match statement. ↩

[MichaReiser]

yeah, that's one reason I'd see benefits in using different codes. But I think we should do so consistently between Ruff and Red Knot.

The nice thing about the errors not being suppressable or configurable is that splitting syntax error into more codes in the future isn't a breaking change. I'm leaning towards using a single (or two) error codes for a first version, considering that Ruff doesn't support documenting error codes other than rules (and Red Knot doesn't have the infrastructure but it's at least designed so that we could)

[AlexWaygood]

Providing good documentation for the more complex syntax errors (for both Ruff and red-knot) is quite important to me. I'm happy to leave it out of this first PR, but I do think we should make sure that we have a solution for this before stabilising these rules.

[MichaReiser]

Makes sense. It would probably be good to create an issue for this so that we can explore different options on how we can accomplish this (e.g. a non-error-code specific solution could be to maintain the documentation on our website and link to them from the diagnostic)

[ntBre]

I opened an issue with Alex's comment above: #16377

I don't have strong feelings either way right now, but I think it's a good idea to keep this in mind, especially as we get to the more complicated errors.

[ntBre]

Just rebased onto main to get the PythonVersion and Span changes. I still need to work in the PyFileSource or ParserOptions idea and remove the semantic error detection, but my hope is that this branch will eventually be the usable prototype for version-specific errors.

[ntBre]

Good call @dhruvmanila. I think I must have missed wiring this up in the LSP because I'm not getting any errors with a match with a target version of 3.8 in my ruff.toml. I'll take a look at that now.

[ntBre]

I think I've addressed all of the feedback here, but one thing Micha and I discussed in our 1:1 is checking on resetting diagnostics on speculative parsing. I have not done that yet.

[MichaReiser]

Thanks for addressing the feedback.

One last thing that I missed in my last review. We need to ensure that we handle version related syntax errors correctly when doing speculative parsing -- that means, we have to drop them if it turns out that the parser took the wrong branch.

You can do this by capturing the length of the unsupported syntax errors vec in the checkpoint method and truncate the errors in the rewind method

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

Lines 657 to 682 in 97d0659

	fn checkpoint(&self) -> ParserCheckpoint {
	ParserCheckpoint {
	tokens: self.tokens.checkpoint(),
	errors_position: self.errors.len(),
	current_token_id: self.current_token_id,
	prev_token_end: self.prev_token_end,
	recovery_context: self.recovery_context,
	}
	}
	/// Restore the parser to the given checkpoint.
	fn rewind(&mut self, checkpoint: ParserCheckpoint) {
	let ParserCheckpoint {
	tokens,
	errors_position,
	current_token_id,
	prev_token_end,
	recovery_context,
	} = checkpoint;
	self.tokens.rewind(tokens);
	self.errors.truncate(errors_position);
	self.current_token_id = current_token_id;
	self.prev_token_end = prev_token_end;
	self.recovery_context = recovery_context;
	}

[MichaReiser]

I hope the checkpointing doesn't regress performance... No, it all looks green. Nice

[ntBre]

Thanks again everyone for the reviews! I'll leave this open until tomorrow or so in case @dhruvmanila wants to have another look, but as far as I know this is ready to merge!

[dhruvmanila]

This is great. Thank you for waiting on me and I like the updated test plan with the editor screenshots. Just a minor nit, feel free to ignore it.

[dhruvmanila]

nit: I think we can merge this into the above chain of parsed.errors() as we've already checked the show_syntax_errors flag. Like:

parsed.errors().iter().map(...).chain(parsed.unsupported_syntax_errors().iter().map(...))

[ntBre]

Nice catch!