Default to latest supported Python version for version-related syntax errors

[ntBre]

Summary

This PR partially addresses #16418 via the following:

• LinterSettings::unresolved_python_version is now a TargetVersion, which is a thin wrapper around an Option<PythonVersion>
• Checker::target_version now calls TargetVersion::linter_version internally, which in turn uses unwrap_or_default to preserve the current default behavior
• Calls to the parser now call TargetVersion::parser_version, which calls unwrap_or_else(PythonVersion::latest)
• The Checker's implementation of SemanticSyntaxContext::python_version also uses TargetVersion::parser_version to use PythonVersion::latest for semantic errors

In short, all lint rule behavior should be unchanged, but we default to the latest Python version for the new syntax errors, which should minimize confusing version-related syntax errors for users without a version configured.

Test Plan

Existing tests, which showed no changes (except for printing default settings).

[codspeed-hq]

CodSpeed Performance Report

Merging #17529 will not alter performance

_{Comparing brent/default-python-version (3ef26d0) with main (a4c8e43)}

Summary

✅ 33 untouched benchmarks

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -6 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

facebookresearch/chameleon (+0 -6 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

- chameleon/viewer/backend/models/chameleon_distributed.py:405:13: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
- chameleon/viewer/backend/models/chameleon_distributed.py:818:17: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
- chameleon/viewer/backend/models/chameleon_local.py:633:13: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
- chameleon/viewer/backend/models/service.py:131:21: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
- chameleon/viewer/backend/models/service.py:154:17: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
- chameleon/viewer/backend/models/service.py:226:25: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
SyntaxError:	6	0	6	0	0

[MichaReiser]

I haven't started reviewing the PR yet but just a remark on the design.

My only concern with this approach is that it won't work for Red Knot because it currently always requires to know exactly which Python version it is checking. Having said that, I'm okay with ignoring this limitation for now because we may be able to lift this Red Knot restriction in the future by supporting multi-version checking.

[ntBre]

As mentioned above, I updated the PLC2801 and UP035 checks to avoid as many false negatives as possible. It didn't change the ecosystem results for UP035, but it will affect imports from the collections or pipes modules, which are not version-dependent.

Thanks for the additional context on red-knot's approach. I think this is ready for review if the design sounds reasonable otherwise.

[MichaReiser]

I think I'd find it useful to have a small write up that walks through the different places where we now change the default and explains why it's important that we pick a different default and why that specific default.

I've a slight concern that we now have multiple defaults and it's unclear to even me when we pick which one. I worry that it will be very hard for users to understand what's going on and even harder for us to make this self explanatory in the tool (e.g. by attaching notes in diagnostics). Could we come up with a simpler approach that assumes fewer different versions or maybe simply disables certain checks if no version's specified?

[ntBre]

I think this should now be up to date with our internal discussions. In summary (I'll update the PR summary to something like this once this is up to date):

• LinterSettings::unresolved_python_version is now an Option
• Checker::target_version now calls unwrap_or_default internally, so the behavior in most cases is unchanged
• Calls to the parser now call unwrap_or_else(PythonVersion::latest)
• The Checker's implementation of SemanticSyntaxContext::python_version also calls unwrap_or_else(PythonVersion::latest)

TODOs

I left two literal TODOs in configuration.rs for the target_version fields on FormatterSettings and AnalyzeSettings. Do we want to change anything for those, or should I just delete the comments?

My other question is: should we go ahead and merge this? I put the breaking label on it when I thought it would be a larger change that needed to wait for a minor release. Now that it only affects syntax errors, I think it could be more reasonable to merge anyway. Alternatively, we could just merge the Option<PythonVersion> changes here and only save the smaller syntax error changes for a future release. That might be easier than keeping this whole branch updated.

[MichaReiser]

I left two literal TODOs in configuration.rs for the target_version fields on FormatterSettings and AnalyzeSettings. Do we want to change anything for those, or should I just delete the comments?

I don't think we need to change anything here (assuming they still default to lowest supported)

My other question is: should we go ahead and merge this? I put the breaking label on it when I thought it would be a larger change that needed to wait for a minor release. Now that it only affects syntax errors, I think it could be more reasonable to merge anyway. Alternatively, we could just merge the Option changes here and only save the smaller syntax error changes for a future release. That might be easier than keeping this whole branch updated.

I think landing this without breaking should be fine because it only impacts version-specific syntax and semantic errors, both are behind preview.

[ntBre]

I don't think we need to change anything here (assuming they still default to lowest supported)

Yep, they still unwrap_or_default. I'll delete the comments.

I think landing this without breaking should be fine because it only impacts version-specific syntax and semantic errors, both are behind preview.

Ah of course, I keep forgetting they're still in preview. Thanks!

[MichaReiser]

Overall looks good but I think we should try to find a better place for the default handling. I'm worried by the amount of places where we now have to call unwrap_or_default.

[MichaReiser]

It would be nice if we could handle the default elsewhere. E.g. on the call site. I would prefer to have the default handling in as few places as possible.

[ntBre]

I was able to move the unwrap in this case up to check_path, but I went through the others and didn't see any more we could fix. Anything that goes through check_ast needs to have the Option because check_ast constructs a Checker, which also contains an Option. That brings us down to only two unwrap_or_default calls, both in check_path.

I think the unwrap_or_latest calls are as restricted as possible too, with only four of them in non-test code, and two of those are outside of the linter in ruff_wasm and ruff_server.

I agree in general, though, so I'm happy to update more of these if I'm missing a nicer solution.

[ntBre]

Actually, maybe we could make ParseOptions::with_target_version take an option and handle the unwrap once internally, if that would be preferable. I haven't tried it yet, but I don't think that would cause problems anywhere.

[MichaReiser]

I'm not too concerned about passing Option. That's just what we need to do. I'm more concerned about the number of places where we need to change to get consistent behavior. It seems very easy to miss updating one of those places.

Actually, maybe we could make ParseOptions::with_target_version take an option and handle the unwrap once internally, if that would be preferable

I think I'd prefer changing the ParseOptions::default to initialize to the latest version and to either:

• Only call with_target_version if target_version is Some
• Change with_target_version to take an Option and only set it if it isn't None

Unless changing the default results in a too big fall out with tests?

[MichaReiser]

You also need to update the PR summary.

[ntBre]

You also need to update the PR summary.

Will do! I was just waiting to see if there were any other big changes before I rewrote it.

[MichaReiser]

I'm not too concerned about passing Option. That's just what we need to do. I'm more concerned about the number of places where we need to change to get consistent behavior. It seems very easy to miss updating one of those places.

Actually, maybe we could make ParseOptions::with_target_version take an option and handle the unwrap once internally, if that would be preferable

I think I'd prefer changing the ParseOptions::default to initialize to the latest version and to either:

• Only call with_target_version if target_version is Some
• Change with_target_version to take an Option and only set it if it isn't None

Unless changing the default results in a too big fall out with tests?

[MichaReiser]

Would it help if check_path (et al) would take a TargetVersion struct instead that has two methods (or even in LinterSettings or for all tools?):

• linter_version
• parser_version
• (formatter_version)

It's a thin wrapper around an Option<PythonVersion>

It would give us a place to document the decision why we use one version for a specific tool and it doesn't repeat the decision in many different places.

That makes me wonder if we should also pass this struct to the formatter and analyze so that they can initialize the parser with the right version.

[ntBre]

Oh yeah, I think that's a great idea. That was roughly my intention earlier with two Checker methods, but the parser didn't get a whole Checker, so I couldn't get it to work out nicely. Wrapping Option<PythonVersion> seems like it would help a lot.

That makes me wonder if we should also pass this struct to the formatter and analyze so that they can initialize the parser with the right version.

Ooh good catch. Although it may not be a big deal in these cases if they aren't emitting the errors. It looks like both of these don't actually pass any Python version to the parser currently, so it's just using the default.

[ntBre]

Could I get one more quick review here? I think the TargetVersion changes helped a lot, but I want to make sure it's what you had in mind.

I think the three (minor) things I'm not sure about are

• The Display impl for TargetVersion - is there a way to do this with display_settings! directly? (inline the inner option while also using | optional)
• The Checker::target_version method - I feel like this should return a TargetVersion, but then every lint rule would have to call .linter_version() itself. So I think this is still a nice convenience method
• Should TargetVersion be in settings::types instead of settings?

We could also reuse TargetVersion in the formatter and analyze settings, but I think that would make sense in a follow-up PR or two.

As an even smaller note, I know we usually prefer calling TargetVersion::from, but tacking the into calls on here was so much easier. Hopefully that's okay!

[MichaReiser]

Looks good. Thanks for following up on this.

I haven't used display_settings myself but I think it also supports calling the debug implementation instead of Display. But I think what you have is fine.

[MichaReiser]

I agree that this method is still useful. It's somewhat confusing that it's called target-version and returns a PythonVersion but renaming it to python_version might be equally confusing because of the method on SemanticSyntaxContext.

[ntBre]

I double-checked the ecosystem results, and chameleon has a pyproject.toml but no project-level requires-python setting (only a build-system.requires-python and a tool.mypy.python_version, which I don't think we read), so I believe we are accurately falling back to a default value, which is now high enough to avoid the match syntax error.