[ty] Ignore unsupported editor-selected Python versions#24498
[ty] Ignore unsupported editor-selected Python versions#24498charliermarsh merged 2 commits intomainfrom
Conversation
4bcd9f8 to
8e892c6
Compare
8e892c6 to
9437d79
Compare
| } else { | ||
| Some(RelativePathBuf::python_extension(executable.sys_prefix)) | ||
| }; | ||
|
|
There was a problem hiding this comment.
What's the reason for not setting fallback_python if it's too new or too old? Does this match the behavior when specifying too-old or too-new Python version in the configuration or on the CLI?
There was a problem hiding this comment.
This actually revealed a bug, thanks. If we kept the fallback Python, then we'd end up trying to re-infer the version later on; and at that site, we weren't filtering by supported version. I added that filtering, and now preserve the fallback Python but ignore the version.
9437d79 to
413d0f6
Compare
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 87.72%. The percentage of expected errors that received a diagnostic held steady at 82.85%. The number of fully passing files held steady at 74/132. |
Memory usage reportMemory usage unchanged ✅ |
0f5c60b to
a08d136
Compare
|
306c0a0 to
6ff5134
Compare
6ff5134 to
aa3a8af
Compare
788e7d7 to
9236531
Compare
| tracing::debug!( | ||
| "Ignoring unsupported inferred Python version: {}", | ||
| python_version.version | ||
| ); |
There was a problem hiding this comment.
Per #24499 (comment), were you expecting us to error here?
There was a problem hiding this comment.
I don't think we should error here. It feels overly aggressive in an IDE setup. I would rather have an LSP that assumes 3.14 rather than no LSP at all when using a venv with Python 3.15 installed (whether that's intentional or not). Ideally, we'd make the user aware of the environment mismatch by emitting a warning Diagnostic here. Unfortunately, we don't really have the infrastructure in place to do this here in to_program_settings (we support it in to_settings by returning the settings alongside a vec of diagnostics). I don't think we have to solve it today. However, I suggest increasing the log severity to at least info or even warn. Users can suppress the warning by explicitly setting a Python version.
You could consider clamping the Python version to ensure that ty uses the "closest" Python version it knows (e.g. don't default to PythonVersion::latest when the environment uses Python 3.6)
| .map(|version| format!("`{version}`")) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ); |
| tracing::debug!( | ||
| "Ignoring unsupported inferred Python version: {}", | ||
| python_version.version | ||
| ); |
There was a problem hiding this comment.
I don't think we should error here. It feels overly aggressive in an IDE setup. I would rather have an LSP that assumes 3.14 rather than no LSP at all when using a venv with Python 3.15 installed (whether that's intentional or not). Ideally, we'd make the user aware of the environment mismatch by emitting a warning Diagnostic here. Unfortunately, we don't really have the infrastructure in place to do this here in to_program_settings (we support it in to_settings by returning the settings alongside a vec of diagnostics). I don't think we have to solve it today. However, I suggest increasing the log severity to at least info or even warn. Users can suppress the warning by explicitly setting a Python version.
You could consider clamping the Python version to ensure that ty uses the "closest" Python version it knows (e.g. don't default to PythonVersion::latest when the environment uses Python 3.6)
| let Some(python_version) = u8::try_from(version.major) | ||
| .ok() | ||
| .zip(u8::try_from(version.minor).ok()) | ||
| .map(PythonVersion::from) |
There was a problem hiding this comment.
Why not PythonVersion::try_from((version.major, version.minor))? It would also remove the need for this zip, map, filter chain, which I struggled to reason about (unless there's more to it that I don't understand)
There was a problem hiding this comment.
This was intentional -- it's the easiest way to ensure that we only need one block for emitting the warning. But I can add a try_from on i64 to keep it simple...
There was a problem hiding this comment.
oh, these are i64. I thought they're &str slices.
| .ok() | ||
| .zip(u8::try_from(version.minor).ok()) | ||
| .map(PythonVersion::from) | ||
| .filter(|version| !PythonVersion::iter().any(|supported| supported > *version)) |
There was a problem hiding this comment.
Why do we use > here but = in options? Should we add a PythonVersion::is_known helper? This would ensure that we don't need to update all iter call sites if we happen to add Python 3.16 support to Ruff first (which is something we did in the past)
9236531 to
5fe2cb8
Compare
5fe2cb8 to
364811c
Compare
* main: [ty] Fix bad diagnostic range for incorrect implicit `__init_subclass__` calls (#24541) [ty] Add a `SupportedPythonVersion` enum (#24412) [ty] Ignore unsupported editor-selected Python versions (#24498) [ty] Add snapshots for `__init_subclass__` diagnostics (#24539) [ty] Minor fix in tests (#24538) [ty] Allow `Final` variable assignments in `__post_init__` (#24529) [ty] Expand test suite for assignment errors (#24537) [ty] Use `map`, not `__map`, as the name of the mapping parameter in `TypedDict` `__init__` methods (#24535) [ty] Rework logic for synthesizing `TypedDict` methods (#24534) [flake8-bandit] Fix S103 false positives and negatives in mask analysis (#24424) [ty] mdtest.py: update dependencies (#24533) Rename patterns and arguments source order iterator method (#24532) [ty] Omit invalid keyword arguments from `TypedDict` signature (#24522) [ty] support super() in metaclass methods (#24483) [ty] Synthesize `__init__` for `TypedDict` (#24476)
* main: Bump typing conformance suite commit to latest upstream (#24553) [ty] Reject deleting`Final` attributes (#24508) [ty] Respect property deleters in attribute deletion checks (#24500) [ty] stop unioning Unknown into types of un-annotated attributes (#24531) [ty] Fix bad diagnostic range for incorrect implicit `__init_subclass__` calls (#24541) [ty] Add a `SupportedPythonVersion` enum (#24412) [ty] Ignore unsupported editor-selected Python versions (#24498) [ty] Add snapshots for `__init_subclass__` diagnostics (#24539) [ty] Minor fix in tests (#24538) [ty] Allow `Final` variable assignments in `__post_init__` (#24529) [ty] Expand test suite for assignment errors (#24537) [ty] Use `map`, not `__map`, as the name of the mapping parameter in `TypedDict` `__init__` methods (#24535) [ty] Rework logic for synthesizing `TypedDict` methods (#24534) [flake8-bandit] Fix S103 false positives and negatives in mask analysis (#24424) [ty] mdtest.py: update dependencies (#24533) Rename patterns and arguments source order iterator method (#24532) [ty] Omit invalid keyword arguments from `TypedDict` signature (#24522) [ty] support super() in metaclass methods (#24483) [ty] Synthesize `__init__` for `TypedDict` (#24476)
Summary
Like #24402, we want to ignore unsupported Python versions that come from the editor. Instead, we'll fall back to the default version (if there's no other configuration set).
One nuance here is that we don't actively show the user a popup if we ignore this version; we just use
tracing::warn!("{message}"). It seems undesirable to show a popup at the conversion site, since we'd then be showing it even if the fallback version were never used. Is it desirable to show a popup ever?