fix Support single-file scripts with PEP 723 metadata #3176#3180
fix Support single-file scripts with PEP 723 metadata #3176#3180asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
|
Diff from mypy_primer, showing the effect of this PR on open source code: scikit-build-core (https://github.com/scikit-build/scikit-build-core)
+ WARN While resolving PEP 723 metadata for `.../projects/scikit-build-core/noxfile.py`: No such file or directory (os error 2)
+ WARN While resolving PEP 723 metadata for `.../projects/scikit-build-core/tests/utils/download_wheels.py`: No such file or directory (os error 2)
+ WARN While resolving PEP 723 metadata for `.../projects/scikit-build-core/noxfile.py`: No such file or directory (os error 2)
+ WARN While resolving PEP 723 metadata for `.../projects/scikit-build-core/noxfile.py`: No such file or directory (os error 2)
+ WARN While resolving PEP 723 metadata for `.../projects/scikit-build-core/tests/utils/download_wheels.py`: No such file or directory (os error 2)
pywin32 (https://github.com/mhammond/pywin32)
+ WARN While resolving PEP 723 metadata for `.../projects/pywin32/Pythonwin/pywin/test/_dbgscript.py`: stream did not contain valid UTF-8
+ WARN While resolving PEP 723 metadata for `.../projects/pywin32/Pythonwin/pywin/test/_dbgscript.py`: stream did not contain valid UTF-8
|
There was a problem hiding this comment.
Pull request overview
Adds support for single-file Python scripts that declare PEP 723 # /// script metadata by augmenting the resolved config with an environment derived from uv run, enabling more accurate interpreter/platform/site-package settings for such scripts.
Changes:
- Introduce an
afterhook inConfigFinderto post-process a selected config on a per-file basis. - Add PEP 723 metadata detection + config override logic (with caching) in the standard config finder.
- Add helpers to query Python environment info either from an interpreter or via
uvfor PEP 723 scripts, and provide a way to disable interpreter discovery once overridden.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pyrefly/lib/commands/config_finder.rs | Detects PEP 723 metadata in scripts and overrides config environment via an after hook; adds tests for extraction/caching behavior. |
| crates/pyrefly_config/src/finder.rs | Adds after callback support to augment configs after they’re selected. |
| crates/pyrefly_config/src/environment/interpreters.rs | Adds disable_query() to freeze interpreter discovery when using script-derived environments. |
| crates/pyrefly_config/src/environment/environment.rs | Refactors interpreter env querying and adds uv-based env querying for PEP 723 scripts. |
Comments suppressed due to low confidence (1)
pyrefly/lib/commands/config_finder.rs:392
PythonEnvironmentis brought into this test module twice: once viause super::*;(because the parent module now importspyrefly_config::...::PythonEnvironment) and again viause crate::config::environment::environment::PythonEnvironment;. This will trigger a Rust duplicate-import error (E0252). Remove the explicit import or alias one of them to avoid the name collision.
use super::*;
use crate::config::config::ConfigSource;
use crate::config::environment::environment::PythonEnvironment;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cache_key = (script_path.to_path_buf(), metadata); | ||
| if let Some(config) = self.cache.lock().get(&cache_key) { | ||
| return Ok(config.dupe()); | ||
| } | ||
|
|
||
| let mut script_environment = (self.query_environment)(script_path)?; | ||
| // Preserve explicit user-specified site-package paths such as `typings/`, | ||
| // while replacing interpreter-derived environment data with the script's. | ||
| script_environment.site_package_path = config.python_environment.site_package_path.clone(); | ||
|
|
||
| let mut config = config.as_ref().clone(); | ||
| config.interpreters.disable_query(); | ||
| config.python_environment = script_environment; | ||
|
|
||
| let config = ArcId::new(config); | ||
| self.cache.lock().insert(cache_key, config.dupe()); | ||
| Ok(config) | ||
| } |
There was a problem hiding this comment.
override_config caches only successful environment queries. If uv is missing or the query fails, the same expensive command (and warning) will repeat on every lookup for that script/metadata. Consider caching negative results (e.g., store a sentinel / last error per (script_path, metadata) for a short TTL or until clear()), so failures don’t repeatedly impact performance and logs.
| })?; | ||
| if !python_info.status.success() { | ||
| let stderr = String::from_utf8(python_info.stderr) | ||
| .unwrap_or("<Failed to parse STDOUT from UTF-8 string>".to_owned()); |
There was a problem hiding this comment.
This fallback string is used when decoding stderr, but says "STDOUT". That makes error messages misleading when stderr isn’t valid UTF-8. Update it to refer to STDERR instead.
| .unwrap_or("<Failed to parse STDOUT from UTF-8 string>".to_owned()); | |
| .unwrap_or("<Failed to parse STDERR from UTF-8 string>".to_owned()); |
| fn script_path(path: &ModulePath) -> Option<&Path> { | ||
| match path.details() { | ||
| ModulePathDetails::FileSystem(path) | ModulePathDetails::Memory(path) => Some(path), | ||
| ModulePathDetails::Namespace(_) | ||
| | ModulePathDetails::BundledTypeshed(_) | ||
| | ModulePathDetails::BundledTypeshedThirdParty(_) | ||
| | ModulePathDetails::BundledThirdParty(_) => None, | ||
| } |
There was a problem hiding this comment.
ModulePathDetails::Memory is treated as a filesystem path for PEP 723 detection, which makes read_pep723_metadata read from disk for in-memory/unsaved buffers. This can cause incorrect/stale metadata detection and can emit warnings when the on-disk file doesn’t exist. Consider skipping PEP 723 overrides for Memory paths (treat them like None here), or changing the override hook to accept in-memory contents instead of reading from the filesystem.
| let Some(metadata) = Self::read_pep723_metadata(script_path)? else { | ||
| return Ok(config); | ||
| }; | ||
|
|
||
| let cache_key = (script_path.to_path_buf(), metadata); | ||
| if let Some(config) = self.cache.lock().get(&cache_key) { | ||
| return Ok(config.dupe()); | ||
| } |
There was a problem hiding this comment.
The cache lookup happens only after reading the script from disk and extracting the metadata, so repeated calls still pay the full file read + parse cost and allocate a new String for the cache key each time. If this runs on every ConfigFinder::python_file call, it can become noticeable for large scripts or frequent LSP updates. Consider extracting metadata via a streaming read that stops at the end marker, and/or caching the extracted metadata (or a hash + mtime) per path so cache hits don’t require rereading the whole file.
Summary
Fixes #3176
Test Plan