-
Notifications
You must be signed in to change notification settings - Fork 280
fix More expressive sys.platform gating #795 #2579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ use pyrefly_python::module_name::ModuleName; | |
| use pyrefly_python::module_path::ModulePath; | ||
| use pyrefly_python::module_path::ModulePathDetails; | ||
| use pyrefly_python::sys_info::SysInfo; | ||
| use pyrefly_python::sys_info::module_platform_guard; | ||
| use pyrefly_types::type_alias::TypeAliasIndex; | ||
| use pyrefly_util::arc_id::ArcId; | ||
| use pyrefly_util::events::CategorizedEvents; | ||
|
|
@@ -365,6 +366,7 @@ impl ModuleDeps { | |
| struct ModuleData { | ||
| handle: Handle, | ||
| config: ArcId<ConfigFile>, | ||
| effective_sys_info: SysInfo, | ||
| state: ModuleState, | ||
| imports: HashMap<ModuleName, FindingOrError<ModulePath>, BuildNoHash>, | ||
| deps: HashMap<Handle, ModuleDeps>, | ||
|
|
@@ -375,6 +377,7 @@ struct ModuleData { | |
| struct ModuleDataMut { | ||
| handle: Handle, | ||
| config: RwLock<ArcId<ConfigFile>>, | ||
| effective_sys_info: RwLock<SysInfo>, | ||
| state: ModuleStateMut, | ||
| /// Import resolution cache: module names from import statements → resolved paths. | ||
| /// Only contains deps that were resolved via `find_import`. | ||
|
|
@@ -389,12 +392,21 @@ struct ModuleDataMut { | |
| rdeps: Mutex<HashSet<Handle>>, | ||
| } | ||
|
|
||
| fn module_sys_info_override( | ||
| sys_info: &SysInfo, | ||
| ast: Option<&ruff_python_ast::ModModule>, | ||
| ) -> Option<SysInfo> { | ||
| let ast = ast?; | ||
| module_platform_guard(&ast.body).map(|platform| sys_info.with_platform(platform)) | ||
| } | ||
|
|
||
| impl ModuleData { | ||
| /// Make a copy of the data that can be mutated. | ||
| fn clone_for_mutation(&self) -> ModuleDataMut { | ||
| ModuleDataMut { | ||
| handle: self.handle.dupe(), | ||
| config: RwLock::new(self.config.dupe()), | ||
| effective_sys_info: RwLock::new(self.effective_sys_info.dupe()), | ||
| state: self.state.clone_for_mutation(), | ||
| imports: RwLock::new(self.imports.clone()), | ||
| deps: RwLock::new(self.deps.clone()), | ||
|
|
@@ -405,9 +417,11 @@ impl ModuleData { | |
|
|
||
| impl ModuleDataMut { | ||
| fn new(handle: Handle, require: Require, config: ArcId<ConfigFile>, now: Epoch) -> Self { | ||
| let effective_sys_info = handle.sys_info().dupe(); | ||
| Self { | ||
| handle, | ||
| config: RwLock::new(config), | ||
| effective_sys_info: RwLock::new(effective_sys_info), | ||
| state: ModuleStateMut::new(require, now), | ||
| imports: Default::default(), | ||
| deps: Default::default(), | ||
|
|
@@ -421,6 +435,7 @@ impl ModuleDataMut { | |
| let ModuleDataMut { | ||
| handle, | ||
| config, | ||
| effective_sys_info, | ||
| state, | ||
| imports, | ||
| deps, | ||
|
|
@@ -433,13 +448,26 @@ impl ModuleDataMut { | |
| ModuleData { | ||
| handle: handle.dupe(), | ||
| config: config.read().dupe(), | ||
| effective_sys_info: effective_sys_info.read().dupe(), | ||
| state, | ||
| imports, | ||
| deps, | ||
| rdeps, | ||
| } | ||
| } | ||
|
|
||
| fn effective_sys_info(&self) -> SysInfo { | ||
| if let Some(ast) = self.state.get_ast().as_deref() { | ||
| let base = self.handle.sys_info(); | ||
| let effective = | ||
| module_sys_info_override(base, Some(ast)).unwrap_or_else(|| base.dupe()); | ||
| *self.effective_sys_info.write() = effective.dupe(); | ||
| effective | ||
| } else { | ||
| self.effective_sys_info.read().dupe() | ||
| } | ||
| } | ||
|
|
||
| /// Look up how this module depends on a specific source handle. | ||
| /// Returns the `ModuleDep` if this module depends on `source_handle`, or `None` if not found. | ||
| fn get_depends_on(&self, source_handle: &Handle) -> Option<ModuleDeps> { | ||
|
|
@@ -1014,11 +1042,12 @@ impl<'a> Transaction<'a> { | |
| let require = guard.require(); | ||
| let stdlib = self.get_stdlib(&module_data.handle); | ||
| let config = module_data.config.read(); | ||
| let sys_info = module_data.effective_sys_info(); | ||
| let ctx = Context { | ||
| require, | ||
| module: module_data.handle.module(), | ||
| path: module_data.handle.path(), | ||
| sys_info: module_data.handle.sys_info(), | ||
| sys_info: &sys_info, | ||
| memory: &self.memory_lookup(), | ||
| uniques: &self.data.state.uniques, | ||
| stdlib: &stdlib, | ||
|
|
@@ -1402,13 +1431,17 @@ impl<'a> Transaction<'a> { | |
| .dupe() | ||
| } | ||
|
|
||
| pub fn get_stdlib(&self, handle: &Handle) -> Arc<Stdlib> { | ||
| pub fn get_stdlib_for_sys_info(&self, sys_info: &SysInfo) -> Arc<Stdlib> { | ||
| if self.data.stdlib.len() == 1 { | ||
| // Since we know our one must exist, we can shortcut | ||
| return self.data.stdlib.first().unwrap().1.dupe(); | ||
| } | ||
|
|
||
| self.data.stdlib.get(handle.sys_info()).unwrap().dupe() | ||
| self.data.stdlib.get(sys_info).unwrap().dupe() | ||
| } | ||
|
Comment on lines
+1434
to
+1441
|
||
|
|
||
| pub fn get_stdlib(&self, handle: &Handle) -> Arc<Stdlib> { | ||
| self.get_stdlib_for_sys_info(handle.sys_info()) | ||
| } | ||
|
|
||
| fn compute_stdlib(&mut self, sys_infos: SmallSet<SysInfo>) { | ||
|
|
@@ -1983,14 +2016,11 @@ impl<'a> TransactionHandle<'a> { | |
| path: Option<&ModulePath>, | ||
| dep: ModuleDep, | ||
| ) -> FindingOrError<ArcId<ModuleDataMut>> { | ||
| let sys_info = self.module_data.effective_sys_info(); | ||
| let handle = match path { | ||
| Some(path) => { | ||
| // Explicit path — already resolved. Bypass imports entirely. | ||
| FindingOrError::new_finding(Handle::new( | ||
| module, | ||
| path.dupe(), | ||
| self.module_data.handle.sys_info().dupe(), | ||
| )) | ||
| FindingOrError::new_finding(Handle::new(module, path.dupe(), sys_info.dupe())) | ||
| } | ||
| None => { | ||
| // No path — needs find_import. Check imports cache first. | ||
|
|
@@ -2010,13 +2040,7 @@ impl<'a> TransactionHandle<'a> { | |
| finding | ||
| } | ||
| }; | ||
| path.map(|path| { | ||
| Handle::new( | ||
| module, | ||
| path.dupe(), | ||
| self.module_data.handle.sys_info().dupe(), | ||
| ) | ||
| }) | ||
| path.map(|path| Handle::new(module, path.dupe(), sys_info.dupe())) | ||
| } | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module_platform_guardscans the entire module body (iter().find_map(...)) and will treat any matchingassert sys.platform == .../if sys.platform != ...: raiseanywhere in the file as a module-level guard. That can incorrectly overrideSysInfofor modules that merely perform a later runtime check. Consider restricting the scan to the leading top-level statements (e.g., after an optional module docstring and possiblyimport sys) so only true module-guard patterns trigger an override.