Skip to content

fix!: Dagger flags cancelation inside daggarable functions - #2003

Merged
nicolaassolini-qntm merged 3 commits into
mainfrom
na/1984-flags
Jul 7, 2026
Merged

fix!: Dagger flags cancelation inside daggarable functions#2003
nicolaassolini-qntm merged 3 commits into
mainfrom
na/1984-flags

Conversation

@nicolaassolini-qntm

@nicolaassolini-qntm nicolaassolini-qntm commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Closes #1984.

This PR fixes an issue where dagger blocks incorrectly cancelled the daggerable property of the enclosing function: a daggerable function should always enforce the daggerable constraint within its body. Previously, a dagger block inside a daggerable function was treated as non-daggerable because the block's dagger modifier cleared the daggerable flag inherited from the function decorator. However, since the context in which a function is called is not known when checking its body, the unitary constraints declared in the function signature must always be enforced.

The bug was generating a bad cfg.unitary_flags in the modifier blocks, affecting both the metadata and the type checking.

BREAKING CHANGE: guppylang-internals: remaned parameter unitary_flags CFGBuilder.build to function_unitary_flags

@hugrbot

hugrbot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

This PR contains breaking changes to the public Python API.

Breaking changes summary
guppylang-internals/src/guppylang_internals/cfg/builder.py:90: CFGBuilder.build(unitary_flags):
Parameter was removed


@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchna/1984-flags
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
120.67 x 1e3
(0.00%)Baseline: 120.67 x 1e3
121.87 x 1e3
(99.01%)
📈 view plot
🚷 view threshold
5,659.00
(0.00%)Baseline: 5,659.00
5,715.59
(99.01%)
tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile📈 view plot
🚷 view threshold
36.19 x 1e3
(0.00%)Baseline: 36.19 x 1e3
36.55 x 1e3
(99.01%)
📈 view plot
🚷 view threshold
1,582.00
(0.00%)Baseline: 1,582.00
1,597.82
(99.01%)
tests/benchmarks/test_queue_push_pop.py::test_queue_push_benchmark_compile📈 view plot
🚷 view threshold
9.95 x 1e3
(0.00%)Baseline: 9.95 x 1e3
10.05 x 1e3
(99.01%)
📈 view plot
🚷 view threshold
347.00
(0.00%)Baseline: 347.00
350.47
(99.01%)
tests/benchmarks/test_queue_push_pop.py::test_queue_push_pop_benchmark_compile📈 view plot
🚷 view threshold
14.11 x 1e3
(0.00%)Baseline: 14.11 x 1e3
14.25 x 1e3
(99.01%)
📈 view plot
🚷 view threshold
470.00
(0.00%)Baseline: 470.00
474.70
(99.01%)
🐰 View full continuous benchmarking report in Bencher

@nicolaassolini-qntm nicolaassolini-qntm added the B-backport-nominated A PR (or an issue pending a PR) that is nominated for backporting to the previous minor series. label Jul 6, 2026

Copilot AI left a comment

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.

Pull request overview

This PR fixes unitary-flag inference for modifier (with ...) blocks so that unitary flags declared on a function via the @guppy(...) decorator are preserved inside nested modifier blocks (preventing accidental cancellation via UnitaryFlags.accumulate, e.g. with dagger).

Changes:

  • Split function-level decorator unitary flags from block-accumulated unitary flags in CFGBuilder, and ensure decorator flags are preserved when constructing modifier-block CFGs.
  • Add a regression test ensuring a @guppy(daggerable=True) function containing a with dagger: block is still treated as daggerable for type checking.
  • Add the corresponding expected error output for the new regression test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
tests/error/modifier_errors/decorator_dagger_is_preserved.py New regression test exercising daggerable decorator preservation through a dagger modifier block.
tests/error/modifier_errors/decorator_dagger_is_preserved.err Expected error output confirming the dagger-context constraint is enforced.
guppylang-internals/src/guppylang_internals/cfg/builder.py Adjust CFG construction to track decorator flags separately from accumulated modifier flags and preserve decorator flags in nested blocks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/error/modifier_errors/decorator_dagger_is_preserved.py Outdated
Comment thread tests/error/modifier_errors/decorator_dagger_is_preserved.py Outdated
Comment thread guppylang-internals/src/guppylang_internals/cfg/builder.py Outdated
Comment thread guppylang-internals/src/guppylang_internals/cfg/builder.py Outdated
Comment thread guppylang-internals/src/guppylang_internals/cfg/builder.py Outdated
@codecov-commenter

codecov-commenter commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.02%. Comparing base (f711512) to head (d5ae581).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2003   +/-   ##
=======================================
  Coverage   93.02%   93.02%           
=======================================
  Files         152      152           
  Lines       14609    14615    +6     
=======================================
+ Hits        13590    13596    +6     
  Misses       1019     1019           

☔ 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.

@nicolaassolini-qntm nicolaassolini-qntm changed the title fix: Dagger flags cancelation inside daggarable functions fix!: Dagger flags cancelation inside daggarable functions Jul 6, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 6, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 11 untouched benchmarks


Comparing na/1984-flags (d5ae581) with main (f711512)

Open in CodSpeed

@nicolaassolini-qntm
nicolaassolini-qntm marked this pull request as ready for review July 6, 2026 11:48
@nicolaassolini-qntm
nicolaassolini-qntm requested a review from a team as a code owner July 6, 2026 11:48
@nicolaassolini-qntm
nicolaassolini-qntm requested review from mark-koch and qartik and removed request for qartik July 6, 2026 11:48
Comment thread tests/error/modifier_errors/decorator_dagger_is_preserved.py
Comment on lines -367 to +395
cfg = CFGBuilder().build(node.body, True, self.globals, accumulated_flags)
cfg = CFGBuilder().build(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wouldn't the easier fix be to just pass self.unitary_flags here instead of accumulated_flags?

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.

do you mean self.function_unitary_flags?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I mean keep the single unitary_flags field on this class as before and set accumulated_flags = self.unitary_flags | modifiers.flags() here?

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.

When there are more nested modifier blocks, I need one variable to store the outer function flag and another one to store the "dynamic" flag of the modifiers.
If I have only one variable, how do I know if self.unitary_flags is related to the function (thus we need to do self.unitary_flags | modifiers.flags()) or to the previous modifier (we need to use the .accumulate method that deletes even daggers)

@mark-koch mark-koch Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we need to delete even daggers? If I add with dagger, then the body ought to be daggerable imo, even if it's an even dagger

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.

with dagger:
  with control:
    with dagger:
       Does the dagger constraint apply here?

@mark-koch mark-koch Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Imo it should

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nevermind, I changed my mind. I think this is ok as is 👍

Comment thread guppylang-internals/src/guppylang_internals/cfg/builder.py
@nicolaassolini-qntm
nicolaassolini-qntm added this pull request to the merge queue Jul 7, 2026
Merged via the queue into main with commit f83c410 Jul 7, 2026
16 checks passed
@nicolaassolini-qntm
nicolaassolini-qntm deleted the na/1984-flags branch July 7, 2026 12:02
@maximilianruesch maximilianruesch added B-backport-accepted A PR (or issue pending a PR) that has been accepted for backporting to the previous minor series. and removed B-backport-nominated A PR (or an issue pending a PR) that is nominated for backporting to the previous minor series. labels Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-backport-accepted A PR (or issue pending a PR) that has been accepted for backporting to the previous minor series.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Dagger flags are wrongly inferred with nested functions

6 participants