fix AST dependency analysis coverage#523
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| async def exercise() -> str: | ||
| named = (result := named_value()) | ||
| callback = lambda: lambda_value() |
There was a problem hiding this comment.
Keeping this lambda intentionally: the fixture exists to exercise dependency traversal through Expr::Lambda; replacing it with lambda_value would remove that regression coverage while preserving only output behavior.
📊 Ecosystem Test Results📋 Test StatusTest Summary:
📈 Benchmark Results📊 View detailed benchmark report 📦 Package Bundling Metrics
Benchmark metrics are tracked via Bencher.dev 📊 View detailed performance trends and comparisons on the Bencher dashboard. Generated by ecosystem-tests workflow |
|
Bencher Report| Branch | codex/fix-509-ast-dependency-analysis |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| build_dependency_graph | 📈 view plot 🚷 view threshold | 612.93 ns(-92.26%)Baseline: 7,920.34 ns | 13,018.24 ns (4.71%) |
| bundle_simple_project | 📈 view plot 🚷 view threshold | 1,031,100.00 ns(-82.83%)Baseline: 6,004,598.12 ns | 25,934,677.17 ns (3.98%) |
| resolve_module_path | 📈 view plot 🚷 view threshold | 119.64 ns(+7.66%)Baseline: 111.13 ns | 144.31 ns (82.91%) |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the core AST dependency analysis logic within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the AST dependency collection logic in graph_builder.rs by introducing a DependencyCollector that implements Ruff's Visitor trait, replacing manual traversal helpers. A bug was identified in extract_dotted_name where reversing the collected parts of a dotted name results in an incorrect path, and a suggestion was made to simplify the logic by reusing build_full_dotted_name.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if let Some(full_name) = Self::extract_dotted_name(attribute) { | ||
| self.read_vars.insert(full_name.clone()); | ||
| if full_name.contains('.') { | ||
| let root = full_name | ||
| .split('.') | ||
| .next() | ||
| .expect("full_name should have at least one part"); | ||
| self.read_vars.insert(root.to_owned()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn build_full_dotted_name(expr: &Expr) -> Option<String> { | ||
| match expr { | ||
| Expr::Name(name) => Some(name.id.to_string()), | ||
| Expr::Attribute(attribute) => Self::build_full_dotted_name(&attribute.value) | ||
| .map(|base| format!("{}.{}", base, attribute.attr)), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| fn extract_dotted_name(attribute: &ast::ExprAttribute) -> Option<String> { | ||
| fn build_dotted_name(expr: &Expr, parts: &mut Vec<String>) -> bool { | ||
| match expr { | ||
| Expr::Name(name) => { | ||
| parts.push(name.id.to_string()); | ||
| true | ||
| } | ||
| Expr::Attribute(attribute) if build_dotted_name(&attribute.value, parts) => { | ||
| parts.push(attribute.attr.to_string()); | ||
| true | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| let mut parts = Vec::new(); | ||
| if build_dotted_name(&attribute.value, &mut parts) { | ||
| parts.reverse(); | ||
| Some(parts.join(".")) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a bug in extract_dotted_name where it calls parts.reverse(). Because build_dotted_name is recursive and processes the base Expr::Name first, the elements are already pushed in the correct left-to-right order (e.g., ["xml", "etree", "ElementTree"]). Reversing them results in a reversed path like "ElementTree.etree.xml", which causes incorrect dependency resolution and root module extraction.
Instead of fixing extract_dotted_name, we can completely eliminate it and reuse build_full_dotted_name(&attribute.value), which is already correct, simpler, and recursive.
| if let Some(full_name) = Self::extract_dotted_name(attribute) { | |
| self.read_vars.insert(full_name.clone()); | |
| if full_name.contains('.') { | |
| let root = full_name | |
| .split('.') | |
| .next() | |
| .expect("full_name should have at least one part"); | |
| self.read_vars.insert(root.to_owned()); | |
| } | |
| } | |
| } | |
| fn build_full_dotted_name(expr: &Expr) -> Option<String> { | |
| match expr { | |
| Expr::Name(name) => Some(name.id.to_string()), | |
| Expr::Attribute(attribute) => Self::build_full_dotted_name(&attribute.value) | |
| .map(|base| format!("{}.{}", base, attribute.attr)), | |
| _ => None, | |
| } | |
| } | |
| fn extract_dotted_name(attribute: &ast::ExprAttribute) -> Option<String> { | |
| fn build_dotted_name(expr: &Expr, parts: &mut Vec<String>) -> bool { | |
| match expr { | |
| Expr::Name(name) => { | |
| parts.push(name.id.to_string()); | |
| true | |
| } | |
| Expr::Attribute(attribute) if build_dotted_name(&attribute.value, parts) => { | |
| parts.push(attribute.attr.to_string()); | |
| true | |
| } | |
| _ => false, | |
| } | |
| } | |
| let mut parts = Vec::new(); | |
| if build_dotted_name(&attribute.value, &mut parts) { | |
| parts.reverse(); | |
| Some(parts.join(".")) | |
| } else { | |
| None | |
| } | |
| } | |
| if let Some(full_name) = Self::build_full_dotted_name(&attribute.value) { | |
| self.read_vars.insert(full_name.clone()); | |
| if full_name.contains('.') { | |
| let root = full_name | |
| .split('.') | |
| .next() | |
| .expect("full_name should have at least one part"); | |
| self.read_vars.insert(root.to_owned()); | |
| } | |
| } | |
| } | |
| fn build_full_dotted_name(expr: &Expr) -> Option<String> { | |
| match expr { | |
| Expr::Name(name) => Some(name.id.to_string()), | |
| Expr::Attribute(attribute) => Self::build_full_dotted_name(&attribute.value) | |
| .map(|base| format!("{}.{}", base, attribute.attr)), | |
| _ => None, | |
| } | |
| } |
There was a problem hiding this comment.
✅ Addressed in eaad8f6. track_attribute_access now reuses build_full_dotted_name(&attribute.value), and the reversed helper has been removed.
tinovyatkin
changed the title
📊 Source Code Metrics (this PR vs
|
| File | Cyclomatic | Cognitive | Functions | LLOC | MI |
|---|---|---|---|---|---|
| crates/cribo/src/graph_builder.rs | 255 (main: 303) 🟢 | 207 (main: 347) 🟢 | 40 (main: 37) 🔴 | 426 (main: 522) 🟢 | 0 ⚪ |
Generated by mehen v0.6.0 — the code quality watcher.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes a core correctness gap in Cribo’s tree-shaking dependency analysis by replacing incomplete, hand-written AST matching in GraphBuilder with Ruff’s exhaustive AST visitor traversal. This ensures runtime references nested under previously-unhandled AST variants are consistently collected, preventing valid code from losing required definitions during tree shaking.
Changes:
- Refactor dependency collection in
crates/cribo/src/graph_builder.rsto use aDependencyCollectorimplementing Ruff’sVisitor, eliminating silent fallback arms that skipped AST variants. - Preserve/encode dependency semantics for assignments, deletes, imports, globals, and attribute-access tracking while delegating the remainder of traversal to Ruff’s walker.
- Add a new snapshot fixture (
tree_shake_ast_dependency_coverage) plus snapshots covering the newly-supported AST shapes end-to-end (bundled code, execution, requirements, Ruff lint results).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cribo/src/graph_builder.rs | Replaces partial AST matching with a Ruff Visitor-based dependency collector for complete traversal and consistent dependency capture. |
| crates/cribo/tests/fixtures/tree_shake_ast_dependency_coverage/main.py | New fixture entrypoint to exercise dependency collection across additional AST variants. |
| crates/cribo/tests/fixtures/tree_shake_ast_dependency_coverage/operations.py | New fixture module containing constructs like named expressions, lambda/await/yield, starred/slice, annotated/augmented assignments, delete, and type alias. |
| crates/cribo/tests/snapshots/bundled_code@tree_shake_ast_dependency_coverage.snap | Snapshot of emitted bundle for the new AST-coverage fixture. |
| crates/cribo/tests/snapshots/execution_results@tree_shake_ast_dependency_coverage.snap | Snapshot asserting the bundled output executes and matches expected stdout. |
| crates/cribo/tests/snapshots/requirements@tree_shake_ast_dependency_coverage.snap | Snapshot confirming no third-party requirements are detected for the fixture. |
| crates/cribo/tests/snapshots/ruff_lint_results@tree_shake_ast_dependency_coverage.snap | Snapshot confirming Ruff reports no lint issues in the bundled output for this fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a719037c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(value) = &ann_assign.value { | ||
| self.visit_expr(value); | ||
| } | ||
| self.visit_annotation(&ann_assign.annotation); |
There was a problem hiding this comment.
When this visitor is called from collect_vars_in_body for function or method bodies, x: SomeType does not evaluate SomeType at runtime in Python, but this line visits the annotation and adds it to eventual_read_vars. For called functions with local variable annotations referencing otherwise unused first-party symbols, tree-shaking will now retain those symbols even though executing the function never needs them, so the AST coverage fix regresses pruning accuracy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in eaad8f6. Function and method bodies now skip local annotation reads; nested classes re-enable runtime annotation dependencies, with snapshot coverage for both pruning and retention.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaad8f6876
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.visit_assignment_target(&ann_assign.target); | ||
| } | ||
| Stmt::ClassDef(_) => self.visit_nested_scope(stmt, true), | ||
| Stmt::FunctionDef(_) => self.visit_nested_scope(stmt, false), |
There was a problem hiding this comment.
When a reachable function contains a nested def inner(x: make_type()): ... (or a method signature in a nested class) and does not otherwise read make_type, executing the outer function still evaluates that signature annotation on the nested definition under the default Python semantics. This branch sets annotations_are_runtime to false before walking the entire FunctionDef, so visit_parameters and the return annotation are skipped and tree-shaking can drop the annotation dependency, leaving bundled code that raises NameError or loses annotation side effects when the outer function runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in 66570da. Nested function and class definition-time metadata now traverses before switching local variable-annotation semantics for the body. The snapshot fixture now proves both nested function and nested method signature call annotations are retained, while function/method-local annotation-only helpers are still pruned. Validation: focused snapshot fixture, full clippy, and 173 non-ecosystem tests pass.
|
@codex review |
|
Summary
GraphBuilderwith Ruff's exhaustive AST visitorRoot cause
The dependency collector silently ignored AST variants through fallback match arms, so runtime references nested under those nodes never reached tree shaking and their definitions could be removed.
Impact
Valid Python constructs now retain every runtime definition they reference. The visitor also delegates unknown future AST structure to Ruff's walker instead of silently dropping it.
Validation
cargo clippy --workspace --all-targets --all-features -- -D warningscargo nextest run --workspace -E 'not test(test_ecosystem_all)'(173 passed)The full local suite was also attempted; only
test_ecosystem_allfailed because the pre-existing ecosystem package checkouts do not contain their expected source trees.Fixes #509