Fix: [for cherry-picking] Updated suspension walkers to inspect call/ne - #200
Closed
qodo-code-review[bot] wants to merge 2 commits into
Closed
Fix: [for cherry-picking] Updated suspension walkers to inspect call/ne#200qodo-code-review[bot] wants to merge 2 commits into
qodo-code-review[bot] wants to merge 2 commits into
Conversation
- Traverse call and new spread arguments - Traverse dynamic import awaits
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates generator suspension walkers to recognize yields and awaits nested in call/new spread arguments and dynamic import expressions, while clarifying the required G3 root-gate linkage for regression evidence. Flow diagram for suspension analysis of nested expressionsflowchart TD
Expr[Expression] --> Kind{Expression kind}
Kind --> Call[Call or new expression]
Call --> Args[Inspect arguments]
Args --> Spread[Spread argument]
Spread --> Nested[count_yields or contains_await]
Kind --> Import[Dynamic import expression]
Import --> Source[Inspect import source]
Import --> Options[Inspect import options]
Source --> Await[contains_await]
Options --> Await
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/bamts-compiler/src/emitter/transforms.rs" line_range="7306-7309" />
<code_context>
fn call_argument_suspends(argument: &CallArgument) -> bool {
- matches!(argument, CallArgument::Expression(value) if contains_yield(value))
+ match argument {
+ CallArgument::Expression(value) => contains_yield(value),
+ CallArgument::Spread(spread) => contains_yield(&spread.argument),
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The new match is non-exhaustive because `CallArgument` also has the `Missing` variant, as handled by `rewrite_call_arguments`; the crate fails to compile with a non-exhaustive-pattern error.
**Suggested fix:** Handle `CallArgument::Missing(_)` as non-suspending, or retain a wildcard arm.
```suggestion
match argument {
CallArgument::Expression(value) => contains_yield(value),
CallArgument::Spread(spread) => contains_yield(&spread.argument),
CallArgument::Missing(_) => false,
}
```
</issue_to_address>
### Comment 2
<location path="crates/bamts-compiler/src/emitter/transforms.rs" line_range="7058" />
<code_context>
.iter()
.map(|argument| match argument {
CallArgument::Expression(value) => count_yields(value),
+ CallArgument::Spread(spread) => count_yields(&spread.argument),
_ => 0,
})
</code_context>
<issue_to_address>
**issue (broader_impact):** Spread arguments are now counted as containing yields, but `contains_yield` still ignores `CallArgument::Spread`; the early `eval` guard therefore treats calls such as `f(...(yield value))` as non-suspending and leaves the yield inside the emitted call instead of lowering it.
**Triggers:** When a generator contains a yield expression inside a call or constructor spread argument.
**Suggested fix:** Traverse `CallArgument::Spread(spread).argument` in `contains_yield` as well, keeping the suspension predicates consistent.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: crates/bamts-compiler/src/emitter/transforms.rs:7309, crates/bamts-compiler/src/emitter/transforms.rs:7058
Comment on lines
+7306
to
+7309
| match argument { | ||
| CallArgument::Expression(value) => contains_yield(value), | ||
| CallArgument::Spread(spread) => contains_yield(&spread.argument), | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): The new match is non-exhaustive because CallArgument also has the Missing variant, as handled by rewrite_call_arguments; the crate fails to compile with a non-exhaustive-pattern error.
Suggested fix: Handle CallArgument::Missing(_) as non-suspending, or retain a wildcard arm.
Suggested change
| match argument { | |
| CallArgument::Expression(value) => contains_yield(value), | |
| CallArgument::Spread(spread) => contains_yield(&spread.argument), | |
| } | |
| match argument { | |
| CallArgument::Expression(value) => contains_yield(value), | |
| CallArgument::Spread(spread) => contains_yield(&spread.argument), | |
| CallArgument::Missing(_) => false, | |
| } |
| .iter() | ||
| .map(|argument| match argument { | ||
| CallArgument::Expression(value) => count_yields(value), | ||
| CallArgument::Spread(spread) => count_yields(&spread.argument), |
There was a problem hiding this comment.
issue (broader_impact): Spread arguments are now counted as containing yields, but contains_yield still ignores CallArgument::Spread; the early eval guard therefore treats calls such as f(...(yield value)) as non-suspending and leaves the yield inside the emitted call instead of lowering it.
Triggers: When a generator contains a yield expression inside a call or constructor spread argument.
Suggested fix: Traverse CallArgument::Spread(spread).argument in contains_yield as well, keeping the suspension predicates consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixed Findings
Automated fix from agentic review of #199
Summary by Sourcery
Extend suspension walkers to cover spread arguments and dynamic import expressions while clarifying regression-gate evidence requirements.
Bug Fixes:
Documentation:
Summary by cubic
Fixes suspension walkers so yields in spread arguments and awaits in dynamic imports are now detected during suspension analysis. Previously, these constructs were skipped, potentially missing suspension points in generated generators.
Bug Fixes
count_yields,call_argument_suspends, andcontains_awaitnow inspectCallArgument::Spreadarguments.contains_awaitnow checks dynamic import sources and options viaExpression::Import.Documentation
Written for commit 5bbb1c6. Summary will update on new commits.