Feat: Add CFG support for Walrus operator #322
Conversation
PolLamothe
changed the title
DaisukeYoda
left a comment
DaisukeYoda
left a comment
There was a problem hiding this comment.
Thank you for working on this! Please check the comments I left.
internal/analyzer/cfg_builder.go
Outdated
| case parser.NodeNamedExpr: | ||
| // Handle named expressions (walrus operator) | ||
| // Check if the value is a comprehension | ||
| if stmt.Value != nil { | ||
| if valNode, ok := stmt.Value.(*parser.Node); ok { | ||
| if valNode.Type == parser.NodeListComp || valNode.Type == parser.NodeDictComp || | ||
| valNode.Type == parser.NodeSetComp || valNode.Type == parser.NodeGeneratorExp { | ||
| // Process the comprehension | ||
| b.processComprehension(valNode) | ||
| // Add the statement after comprehension processing | ||
| b.currentBlock.AddStatement(stmt) | ||
| } | ||
| } | ||
| } | ||
| // Regular named expression - just add to current block | ||
| b.currentBlock.AddStatement(stmt) | ||
|
|
There was a problem hiding this comment.
I think the case parser.NodeNamedExpr branch is unreachable. processStatement receives statement-level nodes from node.Body, but walrus expressions like (x := [...]) come wrapped in expression_statement → parenthesized_expression → named_expression. The switch receives expression_statement, not NodeNamedExpr directly, so this code path is never executed.
internal/analyzer/cfg_builder.go
Outdated
| } | ||
| } | ||
| // Regular named expression - just add to current block | ||
| b.currentBlock.AddStatement(stmt) |
There was a problem hiding this comment.
Missing return after processing comprehension causes AddStatement(stmt) to be called twice:
once inside the if block (line 474) and again unconditionally (line 479).
| foundWalrus := false | ||
| for _, block := range cfg.Blocks { | ||
| for _, stmt := range block.Statements { | ||
| stmt.Walk(func(n *parser.Node) bool { | ||
| if n.Type == parser.NodeNamedExpr { | ||
| foundWalrus = true | ||
| return false | ||
| } | ||
| return true | ||
| }) | ||
| if foundWalrus { | ||
| break | ||
| } | ||
| } | ||
| if foundWalrus { | ||
| break | ||
| } | ||
| } | ||
| assert.True(t, foundWalrus, "Should find walrus operator") |
There was a problem hiding this comment.
This test only checks for the existence of the walrus node, but doesn't verify that comprehension CFG blocks (comp_init, comp_header, comp_exit) and edges are actually created.
Consider adding assertions for block labels and edge types.
|
Thanks for the comments, sorry for these mistakes, I will fix them! |
|
No worries at all! CFG + AST traversal is genuinely hard. Appreciate you taking this on! |
- implement named expression parsing in ast builder - add comprehension processing in return and assign statements - add detailed structural tests for walrus and comprehensions
|
I tried to understand better AST and CFG and how we manipulate them. |
|
I just saw that the test failed, I'm sorry i forgot to run all of them, i only ran my own. |
|
The problem seem to be that i'm wrapping every expression in a NodeExpr container even the simple assignation (ex : x = 10), I'm working on fixing this ! |
|
Hi! Your approach works, but you might find it simpler to change only cfg_builder instead of ast_builder. This avoids breaking existing tests. In the case parser.NodeExpr:
if stmt.Value != nil {
if valNode, ok := stmt.Value.(*parser.Node); ok {
// Direct comprehension
if isComprehension(valNode) {
b.processComprehension(valNode)
b.currentBlock.AddStatement(stmt)
return
}
// Walrus containing comprehension: (x := [i for i in ...])
if valNode.Type == parser.NodeNamedExpr {
if inner, ok := valNode.Value.(*parser.Node); ok && isComprehension(inner) {
b.processComprehension(inner)
b.currentBlock.AddStatement(stmt)
return
}
}
}
}
b.currentBlock.AddStatement(stmt)This way the AST structure stays unchanged and the fix is localized to CFG building. |
|
Thanks for your help ! I applied what you told me, hope it's good this time 😄 ! |
|
@PolLamothe Thanks! It looks good now. Tip: When ready for re-review, you can click the 🔄 icon next to the reviewer's name. |
|
Ok ! i saw it earlier but didn't know if i should use it, thanks for letting me know ! |
2 participants
Summary
This PR addresses Issue #145 by implementing support for comprehensions and the Walrus operator (
:=) in the Control Flow Graph (CFG) generation.Type of Change
Related Issues
Closes #145
Changes
AssignmentExpressions.Testing
go test ./...passes locallytestdata/python/edge_cases/python310_features.pyComments
Hi! This task was a bit more challenging than the previous one due to the way the Walrus operator affects scope. I've implemented the parsing and added dedicated tests for it. Let me know if the CFG representation for comprehensions aligns with the project's standards!