Skip to content

fix: Remove outdated barrier when automatically inserting drops#1558

Merged
maximilianruesch merged 2 commits intoQuantinuum:mainfrom
kkoci:maintenance/issue-1516
Mar 23, 2026
Merged

fix: Remove outdated barrier when automatically inserting drops#1558
maximilianruesch merged 2 commits intoQuantinuum:mainfrom
kkoci:maintenance/issue-1516

Conversation

@kkoci
Copy link
Contributor

@kkoci kkoci commented Mar 11, 2026

Remove stale FuncDefn skip in insert_drops (#1516)

Removes a workaround in insert_drops (core.py) that skipped all FuncDefn nodes during drop insertion. The skip was
added to work around a num_out_ports bug in hugr (Quantinuum/hugr#2438), which has since been fixed in hugr 0.15.4 —
the version guppylang already depends on. The guard is also redundant because FuncDefn outputs are FunctionKind, never ValueKind, so no drop would have been inserted anyway.
Changes:

  • Remove 4 lines from insert_drops in compiler/core.py (the stale comment referencing #2438, the isinstance(data.op, ops.FuncDefn) check, and the continue)
  • Add regression test test_drop_with_multiple_funcdefns to tests/integration/test_drop_insertion.py

Closes #1516

@kkoci kkoci requested a review from a team as a code owner March 11, 2026 18:42
@kkoci kkoci requested a review from mark-koch March 11, 2026 18:42
@kkoci kkoci changed the title maintenance: Closed issue mentioned in a comment fix: Closed issue mentioned in a comment Mar 11, 2026
@maximilianruesch maximilianruesch changed the title fix: Closed issue mentioned in a comment fix: Remove outdated barrier when automatically inserting drops Mar 12, 2026
@maximilianruesch
Copy link
Collaborator

Please remember that the issue title often does not suffice as a descriptive enough PR title, and they serve different purposes: An issue title often describes the problem or symptom, and the PR title should describe the solution / broader goal. This is in line with the knowledge about a task at the respective creation times.

I have amended your PR title for this one as an example.

@kkoci
Copy link
Contributor Author

kkoci commented Mar 12, 2026

Thank You @maximilianruesch , so it is not about reproducing the issue title, rather a more descriptive (briefly) title about the approach/solution.

@maximilianruesch
Copy link
Collaborator

Yes. In essence, this title directly goes into the changelog, and it appears in the commit message in the git history (which is essentially a more detailed changelog). Whenever someone reads that, they want to have a summary of what has been changed or at least of what has been fixed (to a reasonable level of detail). "Closed issue mentioned in a comment" is neither descriptive nor helpful to someone reading the history, as it offers no explanation as to what has been done and why.

On the other hand, the current title offers several points: "Remove redundant" signals that something that is no longer needed was removed, and the change should have no impact on the semantics of the program; "when automatically inserting drops" signals the place of the change, thus making it easier to find for people looking for changes related to drop semantics.

Copy link
Collaborator

@mark-koch mark-koch left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good, however the CI checks are failing due to formatting. Can you please run ruff format and check that the CI passes?

@maximilianruesch maximilianruesch added this pull request to the merge queue Mar 23, 2026
Merged via the queue into Quantinuum:main with commit 02b87cc Mar 23, 2026
9 checks passed
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.

[Maintenance]: Closed issue mentioned in a comment

3 participants