Skip to content

feat(effects)!: Improve error messages by passing callee into check_call/synthesize_call#1846

Open
acl-cqc wants to merge 11 commits into
effectsfrom
acl/check_syn_call_target
Open

feat(effects)!: Improve error messages by passing callee into check_call/synthesize_call#1846
acl-cqc wants to merge 11 commits into
effectsfrom
acl/check_syn_call_target

Conversation

@acl-cqc

@acl-cqc acl-cqc commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

This significantly improves a lot of the error messages for effects violations, and perhaps might be applied to other errors too?

Note that tensor'd functions are gonna have terrible messages both before and after, I haven't got a test of these yet as they are marked experimental, note comment - we'd need significant refactoring to sort that.

BREAKING CHANGE: (guppylang-internals) top-level check_call and synthesize_call methods take a tuple of str | DefId | None to identify the callee

@acl-cqc acl-cqc changed the base branch from main to acl/max_effects June 11, 2026 14:29
@hugrbot

hugrbot commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

This PR contains breaking changes to the public Python API.

Breaking changes summary
guppylang-internals/src/guppylang_internals/tracing/state.py:0: TracingState.__init__(unused_undroppable_objs):
Positional parameter was moved
Details: position: from 5 to 6 (+1)

guppylang-internals/src/guppylang_internals/tracing/state.py:0: TracingState.__init__(max_effects):
Parameter was added as required

guppylang-internals/src/guppylang_internals/definition/overloaded.py:0: OverloadNoMatchError.__init__(max_effects_from):
Parameter was added as required

guppylang-internals/src/guppylang_internals/checker/cfg_checker.py:73: check_cfg(first_modifier_node):
Positional parameter was moved
Details: position: from 6 to 7 (+1)

guppylang-internals/src/guppylang_internals/checker/cfg_checker.py:73: check_cfg(max_effects_from):
Parameter was added as required

guppylang-internals/src/guppylang_internals/checker/cfg_checker.py:247: check_bb(max_effects_from):
Parameter was added as required

guppylang-internals/src/guppylang_internals/checker/expr_checker.py:1391: synthesize_call(func_ty):
Parameter was removed

guppylang-internals/src/guppylang_internals/checker/expr_checker.py:1391: synthesize_call(target):
Parameter was added as required

guppylang-internals/src/guppylang_internals/checker/expr_checker.py:1432: check_call(func_ty):
Parameter was removed

guppylang-internals/src/guppylang_internals/checker/expr_checker.py:1432: check_call(target):
Parameter was added as required

guppylang-internals/src/guppylang_internals/checker/modifier_checker.py:21: check_modified_block(max_effects_from):
Parameter was added as required


@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchacl/check_syn_call_target
TestbedLinux
Click to view all benchmark results
Benchmarkhugr_bytesBenchmark Result
bytes x 1e3
(Result Δ%)
Upper Boundary
bytes x 1e3
(Limit %)
hugr_nodesBenchmark Result
nodes
(Result Δ%)
Upper Boundary
nodes
(Limit %)
tests/benchmarks/test_big_array.py::test_big_array_compile📈 view plot
🚷 view threshold
154.02 x 1e3
(0.00%)Baseline: 154.02 x 1e3
155.56 x 1e3
(99.01%)
📈 view plot
🚷 view threshold
6,630.00
(0.00%)Baseline: 6,630.00
6,696.30
(99.01%)
tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile📈 view plot
🚷 view threshold
27.71 x 1e3
(0.00%)Baseline: 27.71 x 1e3
27.99 x 1e3
(99.01%)
📈 view plot
🚷 view threshold
1,051.00
(0.00%)Baseline: 1,051.00
1,061.51
(99.01%)
tests/benchmarks/test_queue_push_pop.py::test_queue_push_benchmark_compile📈 view plot
🚷 view threshold
10.09 x 1e3
(+0.01%)Baseline: 10.09 x 1e3
10.19 x 1e3
(99.02%)
📈 view plot
🚷 view threshold
301.00
(0.00%)Baseline: 301.00
304.01
(99.01%)
tests/benchmarks/test_queue_push_pop.py::test_queue_push_pop_benchmark_compile📈 view plot
🚷 view threshold
13.70 x 1e3
(0.00%)Baseline: 13.70 x 1e3
13.83 x 1e3
(99.01%)
📈 view plot
🚷 view threshold
420.00
(0.00%)Baseline: 420.00
424.20
(99.01%)
🐰 View full continuous benchmarking report in Bencher

Base automatically changed from acl/max_effects to effects June 11, 2026 14:30
@codecov-commenter

codecov-commenter commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (effects@708e4e5). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ls/src/guppylang_internals/checker/expr_checker.py 83.33% 4 Missing ⚠️
...s/src/guppylang_internals/std/_internal/checker.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             effects    #1846   +/-   ##
==========================================
  Coverage           ?   92.85%           
==========================================
  Files              ?      148           
  Lines              ?    13993           
  Branches           ?        0           
==========================================
  Hits               ?    12993           
  Misses             ?     1000           
  Partials           ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@acl-cqc acl-cqc force-pushed the acl/check_syn_call_target branch from 38cf7e6 to 812ed8f Compare June 11, 2026 14:32
@codspeed-hq

codspeed-hq Bot commented Jun 11, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 15.11%

❌ 1 regressed benchmark
✅ 10 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_circuit_comptime_compile 1.4 s 1.7 s -15.11%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing acl/check_syn_call_target (e285257) with main (3c19192)1

Open in CodSpeed

Footnotes

  1. No successful run was found on effects (708e4e5) during the generation of this report, so main (3c19192) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@acl-cqc acl-cqc requested review from mark-koch and tatiana-s June 11, 2026 15:53
@acl-cqc acl-cqc marked this pull request as ready for review June 11, 2026 15:53
@acl-cqc acl-cqc requested a review from a team as a code owner June 11, 2026 15:53
@maximilianruesch maximilianruesch changed the title feat(effects)!: improve error messages by passing callee into check_call/synthesize_call feat(effects)!: Improve error messages by passing callee into check_call/synthesize_call Jun 12, 2026


def _check_effects(
target: tuple[str | DefId | None, FunctionType], ctx: Context, node: AstNode

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion it would be better to have a class instead of tuples for readability - especially given the call graph will just store this information instead of using it here directly so this would also simplify the type of the graph

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem with making this a class, but is it the same as the kind of "node" in the callgraph? Might you want all callgraph nodes to have concrete DefIds, as only such could have outgoing edges?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turned into extra parameter!

@acl-cqc acl-cqc force-pushed the acl/check_syn_call_target branch from fb8cbae to ff68e85 Compare June 19, 2026 09:47
@acl-cqc acl-cqc requested a review from tatiana-s June 22, 2026 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants