refactor: narrow ASTNode types to check_call and _synthesize_binary#1726
refactor: narrow ASTNode types to check_call and _synthesize_binary#1726acl-cqc wants to merge 3 commits into
Conversation
|
Bencher Report| Branch | acl/check_synth_types |
| Testbed | Linux |
Click to view all benchmark results
| Benchmark | hugr_bytes | Benchmark Result bytes x 1e3 (Result Δ%) | Upper Boundary bytes x 1e3 (Limit %) | hugr_nodes | Benchmark Result nodes (Result Δ%) | Upper Boundary nodes (Limit %) |
|---|---|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | 📈 view plot 🚷 view threshold | 158.77 x 1e3(0.00%)Baseline: 158.77 x 1e3 | 160.36 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 6,641.00(0.00%)Baseline: 6,641.00 | 6,707.41 (99.01%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 27.53 x 1e3(0.00%)Baseline: 27.53 x 1e3 | 27.81 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 1,074.00(0.00%)Baseline: 1,074.00 | 1,084.74 (99.01%) |
| tests/benchmarks/test_queue_push_pop.py::test_queue_push_benchmark_compile | 📈 view plot 🚷 view threshold | 10.91 x 1e3(0.00%)Baseline: 10.91 x 1e3 | 11.02 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 308.00(0.00%)Baseline: 308.00 | 311.08 (99.01%) |
| tests/benchmarks/test_queue_push_pop.py::test_queue_push_pop_benchmark_compile | 📈 view plot 🚷 view threshold | 14.84 x 1e3(0.00%)Baseline: 14.84 x 1e3 | 14.99 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 435.00(0.00%)Baseline: 435.00 | 439.35 (99.01%) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1726 +/- ##
==========================================
- Coverage 93.51% 93.50% -0.01%
==========================================
Files 133 133
Lines 12767 12772 +5
==========================================
+ Hits 11939 11943 +4
- Misses 828 829 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
qartik
requested review from
a team,
Copilot and
nicolaassolini-qntm
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR refactors internal type annotations to make CallableDef.check_call accept only ast.Call nodes (instead of a broader AstNode), and tightens the accepted node types for _synthesize_binary in the expression checker. To support the narrower check_call contract in a couple of internal non-call contexts, it introduces an ast_util.fake_call() helper and uses it to manufacture “call-like” nodes for error reporting/spans.
Changes:
- Narrow
CallableDef.check_call(..., node=...)fromAstNodetoast.Calland update all implementers accordingly. - Narrow
ExprSynthesizer._synthesize_binary(..., node=...)toast.BinOp | ast.Compare. - Add
fake_call()helper and use it intry_coerce_to()andto_sized_iter()to provide anast.Callnode tocheck_call.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| guppylang-internals/src/guppylang_internals/std/_internal/checker.py | Use a “fake” ast.Call node when calling check_call for __new__ in to_sized_iter. |
| guppylang-internals/src/guppylang_internals/definition/value.py | Narrow abstract CallableDef.check_call node parameter to ast.Call. |
| guppylang-internals/src/guppylang_internals/definition/traced.py | Update check_call override signature to node: ast.Call. |
| guppylang-internals/src/guppylang_internals/definition/pytket_circuits.py | Update check_call override signature to node: ast.Call. |
| guppylang-internals/src/guppylang_internals/definition/overloaded.py | Update check_call override signature to node: ast.Call. |
| guppylang-internals/src/guppylang_internals/definition/function.py | Update check_call override signature to node: ast.Call. |
| guppylang-internals/src/guppylang_internals/definition/declaration.py | Update check_call override signature to node: ast.Call. |
| guppylang-internals/src/guppylang_internals/definition/custom.py | Update check_call override signature to node: ast.Call. |
| guppylang-internals/src/guppylang_internals/checker/expr_checker.py | Narrow _synthesize_binary node type; create a “fake” call node for numeric coercions before check_call. |
| guppylang-internals/src/guppylang_internals/ast_util.py | Add fake_call() helper for manufacturing ast.Call nodes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def fake_call(name: str, args: list[ast.expr]) -> ast.Call: | ||
| """Creates a fake call node with the given name and arguments.""" | ||
| return ast.Call(func=ast.Name(id=name, ctx=ast.Load()), args=args, keywords=[]) |
There was a problem hiding this comment.
All the time we call fake_call, we wrap the result with with_loc, thus this comment is not right. However, since we always use with_loc, maybe can be useful to include the with_loc call in fake_call directly
There was a problem hiding this comment.
I was wrong in the first comment. The issue underlined by copilot is right, however, I'm not sure if exceptions regarding node.func (like check_num_args) are possible on a fake_call function. @acl-cqc any thoughts?
|
@acl-cqc, I think the PR is fine, but I'm missing the reason why these changes are needed in the first place |
For check_call, a couple of cases here requires building a "fake" ast.Call node to represent the call, using
with_locto identify the same location.For synthesize_call the problem is much worse: there are many more such non-ast.Call nodes (e.g. BinOp and Compare), and moreover there is
guppylang/guppylang-internals/src/guppylang_internals/tracing/function.py
Line 186 in e4324d5
where the AST location
state.nodecan be a FunctionDef or a variety of other things.synthesize_callthen calls intoCustomCallCheckerso I couldn't update that either....which is to say: I didn't really get to where I wanted, but it seemed worthwhile to keep even just this.