Fix public-only coverage for excluded re-export origins#4048
Fix public-only coverage for excluded re-export origins#4048WilliamK112 wants to merge 1 commit into
Conversation
|
@jorenham could use your eyes on this |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
jorenham
left a comment
jorenham
left a comment
There was a problem hiding this comment.
I checked that this doesn't affect numpy of scipy-stubs when --project-excludes isn't passed, so at least it doesn't regress in that way.
There are, however, some opportunities to improve code quality and reduce code duplication here.
I also am not sure if all the heuristics in is_project_reexport_origin are a good idea, and there's no motivation explaining why it would be. It, for example, returns false when for site-packages, which could be problematic if you're running pyrefly coverage directly on an installed library in your .venv.
| fn public_reexport_origin_handles( | ||
| handles: &[Handle], | ||
| transaction: &Transaction, | ||
| config_finder: &ConfigFinder, | ||
| ) -> Vec<Handle> { | ||
| let mut origin_handles = Vec::new(); | ||
| for handle in handles.iter().filter(|h| is_public_module(h.module())) { | ||
| for name in public_exported_names(handle, transaction) { | ||
| if EXCLUDED_MODULE_DUNDERS.contains(&name.as_str()) { | ||
| continue; | ||
| } | ||
| let Some((origin_handle, _)) = trace_export_origin(handle, name, transaction) else { | ||
| continue; | ||
| }; | ||
| if same_handle(handle, &origin_handle) | ||
| || contains_handle(handles, &origin_handle) | ||
| || contains_handle(&origin_handles, &origin_handle) | ||
| || !is_project_reexport_origin(handle, &origin_handle, config_finder) | ||
| { | ||
| continue; | ||
| } | ||
| origin_handles.push(origin_handle); | ||
| } | ||
| } | ||
| origin_handles | ||
| } |
There was a problem hiding this comment.
This iterates the same public modules as compute_public_fqns below, applies the same EXCLUDED_MODULE_DUNDERS filter, and traces the same export. You could produce both the origin-handle list and the FQN set in a single pass, and avoid this introduced code duplication.
| fn same_handle(left: &Handle, right: &Handle) -> bool { | ||
| left.module() == right.module() && left.path() == right.path() | ||
| } | ||
|
|
||
| fn contains_handle(handles: &[Handle], candidate: &Handle) -> bool { | ||
| handles.iter().any(|handle| same_handle(handle, candidate)) | ||
| } |
There was a problem hiding this comment.
Handle already derives PartialEq, Eq, and Hash, so these are not needed, and even hurt performance (+ O(n^2)). Assuming that you ignored sys_info by mistake, then both functions can be removed and a SmallSet of Handle could be used instead.
| fn is_project_reexport_origin( | ||
| public_handle: &Handle, | ||
| origin_handle: &Handle, | ||
| config_finder: &ConfigFinder, | ||
| ) -> bool { | ||
| if origin_handle.path().is_bundled() | ||
| || !matches!( | ||
| origin_handle.path().details(), | ||
| ModulePathDetails::FileSystem(_) | ModulePathDetails::Memory(_) | ||
| ) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| let origin_path = origin_handle.path().as_path(); | ||
| let config = config_finder.python_file(public_handle.module_kind(), public_handle.path()); | ||
| if config | ||
| .site_package_path() | ||
| .any(|site_package| origin_path.starts_with(site_package)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if public_handle.module() == ModuleName::unknown() { | ||
| return public_handle | ||
| .path() | ||
| .as_path() | ||
| .parent() | ||
| .is_some_and(|parent| origin_path.starts_with(parent)); | ||
| } | ||
|
|
||
| if let (Some(public_root), Some(origin_root)) = ( | ||
| public_handle.path().root_of(public_handle.module()), | ||
| origin_handle.path().root_of(origin_handle.module()), | ||
| ) { | ||
| return public_root == origin_root; | ||
| } | ||
|
|
||
| config | ||
| .search_path() | ||
| .any(|search_root| origin_path.starts_with(search_root)) | ||
| } |
There was a problem hiding this comment.
There are a whole bunch of unmotivated heuristics here: a bundle check, path-details matching, site-package rejection, __unknown__ special-casing, parent-dir special-casing, double root_of comparisons, and then a search-path fallback.
Did you verify that each of these heuristics is actually needed here? If so, then please explain why exactly that is (as a comment). If not, then it should be removed.
| report_handles.push(origin_handle); | ||
| } | ||
| } | ||
| if report_handles.len() != handles.len() { |
There was a problem hiding this comment.
| if report_handles.len() != handles.len() { | |
| if !public_origin_handles.is_empty() { |
3 participants
Summary:
coverage check --public-onlyeven when the origin file is matched by--project-excludes__init__.pymay be loaded as__unknown__Closes #4034.
Duplicate/ownership checks:
pyrefly coverage check --public-only:--project-excludesshouldn't exclude public re-exports #4034 is open and unassignedgh search prsforpyrefly coverage check --public-only:--project-excludesshouldn't exclude public re-exports #4034 / public re-export / project-excludes / public-only returned no open duplicatesgh pr list --search 4034only returned unrelated Fix: Handle decorated methods from stubs (Type::Callable) in is_method() Fixes #3465 #3597Validation:
cargo +stable-x86_64-pc-windows-gnu test -p pyrefly --lib commands::coverage::collect::tests::test_public_only -- --nocapturecargo +stable-x86_64-pc-windows-gnu test -p pyrefly --lib commands::coverage::collect::tests::test_compute_public_fqns -- --nocapturecargo +stable-x86_64-pc-windows-gnu test -p pyrefly --lib coverage -- --nocapture(65 passed)pyrefly coverage check foo --public-only --project-excludes foo/_something_private.pynow reportsfoo._something_private.barascoverage-partialand exits 1 at 50% coverage instead of reporting100.00% (0 of 0 typable)cargo +stable-x86_64-pc-windows-gnu fmt --all --checkgit diff --checkI also ran
cargo +stable-x86_64-pc-windows-gnu test -p pyrefly --lib; it reached 6185 passed before 5 existing shape_dsl tests hit their internal> 20swatchdog under the full Windows run. Each of those five tests passed when rerun individually.