feat: ReplaceTypes: recurse on replacements, much deprecation #2442

acl-cqc · 2025-07-15T15:11:22Z

closes #2668 (by a combination of the two routes, minus the extension).

There are two(/3) goals here:

firstly, to improve compositionality - if you configure ReplaceTypes to convert type A to type B, and also type B to type C, then if it sees an A it "should" produce a C. (This is not the case at present, you'll get a B.) Likewise for op-replacements containing ops that themselves would be replaced.
secondly, to allow lowering "drop" without creating an invalid hugr - instead, you can now call copy_discard_op(ty, 0), as demonstrated in the test.
finally, as a driveby, fixes bug where linearization was applied (only) if the op or any previous op in the subtree had been changed

Note that the first is a breaking behaviour change (potentially very breaking, i.e. infinite loop), so to keep this PR non-breaking, I add new methods set_replace(_parametrized,)_(op,type) that process the RHS recursively. To satisfy the second goal, the op-replacing callback is passed the ReplaceTypes (hence rolling two breaking signature changes into one PR).. The naming of the new methods is a little odd, largely because the first choice replace_blah is already taken, and we keep those methods (with non-recursive behaviour) but deprecated.

(If later we really want to keep non-recursive replacement, we can add that later, but there are enough methods right now!)

Note for consts, there's no real equivalent to "recursive" replacement, because there is no way for the ReplaceTypes to look inside CustomConsts like lists, they are opaque. Instead, recursion must be done manually on the elements, as per e.g. array_const:

hugr/hugr-passes/src/replace_types/handlers.rs

Line 75 in fcd1026

repl.change_value(v)?;

codecov · 2025-07-15T15:13:40Z

Codecov Report

❌ Patch coverage is 82.50000% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.46%. Comparing base (3d1ea85) to head (55da1a1).
⚠️ Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-passes/src/replace_types.rs	80.19%	37 Missing and 3 partials ⚠️
hugr-passes/src/linearize_array.rs	95.45%	1 Missing ⚠️
hugr-passes/src/replace_types/linearize.rs	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2442      +/-   ##
==========================================
+ Coverage   83.41%   83.46%   +0.04%     
==========================================
  Files         262      264       +2     
  Lines       51176    51684     +508     
  Branches    46737    47149     +412     
==========================================
+ Hits        42689    43137     +448     
- Misses       6109     6171      +62     
+ Partials     2378     2376       -2

Flag	Coverage Δ
python	`91.53% <ø> (+0.07%)`	⬆️
rust	`82.68% <82.50%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

hugr-passes/src/replace_types.rs

acl-cqc · 2025-11-06T08:42:21Z

hugr-passes/src/replace_types.rs

        for region_root in regions {
-            for n in hugr.descendants(*region_root).collect::<Vec<_>>() {
-                changed |= self.change_node(hugr, n)?;
-                if n != hugr.entrypoint() && changed {


Note this is clearly wrong - it means linearization applies to the first node that changes and all nodes coming after that (!)

hugr-passes/src/replace_types.rs

…on, deprecate old

…test

acl-cqc · 2025-11-14T20:23:41Z

hugr-passes/src/replace_types.rs


 impl ReplacementOptions {
-    /// Specifies that all operations within the replacement should have their
+    fn recursive() -> Self {


I didn't feel any need to make this public because I'm hoping to drop the whole struct...

acl-cqc · 2025-11-14T20:26:32Z

hugr-passes/src/replace_types.rs

+/// May be specified by
+/// [ReplaceTypes::replace_op_with], [ReplaceTypes::replace_parametrized_op_with],
+/// [ReplaceTypes::replace_type_opts] or [ReplaceTypes::replace_parametrized_type_opts].
+/// Otherwise (the default), replacements are inserted as given (without further processing).


Of course this is only relevant to the deprecated methods...so I could deprecate this struct. That's pretty messy though as there are a lot of uses ATM (supporting the interface where clients can specify one)...it'll easier to clean up the ReplaceTypes struct and remove all the references to this when the deprecated methods are removed, and THEN we can deprecate this struct (which will be unused)

mark-koch

Nice! Very happy with allowing copy_discard_op now 👍

mark-koch · 2025-11-18T12:25:59Z

hugr-passes/src/replace_types/linearize.rs

+            let [TypeArg::Runtime(ty)] = args else {
+                panic!("Expected just one type")
+            };
+            Ok(Some(rt.get_linearizer().copy_discard_op(ty, 0)?))


hugr-passes/src/replace_types.rs

mark-koch · 2025-11-18T12:35:48Z

hugr-passes/src/replace_types.rs

+            } else if !linearize_unchanged_ops {
+                continue;
+            }
+            if n != root {


Can you also assume here that root is the first descendant and just skip it via assert_eq!(descs.next(), Some(root)); (similar to change_node below)?

mark-koch · 2025-11-18T12:40:55Z

hugr-passes/src/replace_types.rs

-                    self.linearize_outputs(hugr, n)?;
-                }
-            }
+            changed |= self.change_subtree(hugr, *region_root, false)?;


Why is linearize_unchanged_ops always false here?

It's not...rather, I'm assuming we should not add anything outside the root. (If the root has value outports that we've changed, then we've broken anyway...unless they were unconnected, yes, in which case we might want a discard. I think that's still "outside the region", though. A sensible choice of region would be, say, a FuncDefn.)

I'll comment to that effect.

Oh, sorry, I completely misunderstand....you mean the last parameter!

Answer: because there are no ReplacementOptions in scope here; we only have ReplacementOptions for the RHS of a replacement. (We could add set_replacement_options on the whole pass, or have a run_with_options, or something, but I'm not adding that at this point.)

acl-cqc · 2025-11-18T17:58:04Z

Test coverage looks poor because we no longer cover the deprecated methods, tests have been updated to use the new ones.

acl-cqc mentioned this pull request Jul 15, 2025

feat: ReplaceTypes allows linearizing inside Op replacements #2435

Merged

acl-cqc requested a review from mark-koch July 15, 2025 15:16

Base automatically changed from acl/replace_recursive to main July 16, 2025 12:45

acl-cqc mentioned this pull request Nov 4, 2025

fix: BorrowArray discard handler allows elements to be borrowed #2666

Merged

ReplaceTypes: option to recurse on replacement, including for types

1c9857f

acl-cqc force-pushed the acl/replace_recursive2 branch from 58a05b1 to 1c9857f Compare November 5, 2025 15:51

formatting

52e31f2

acl-cqc mentioned this pull request Nov 5, 2025

refactor: ReplaceTypes linearization interface #2668

Closed

clippy/reword comment

082eddb

acl-cqc commented Nov 5, 2025

View reviewed changes

hugr-passes/src/replace_types.rs Outdated Show resolved Hide resolved

acl-cqc marked this pull request as ready for review November 5, 2025 17:39

acl-cqc requested a review from a team as a code owner November 5, 2025 17:39

acl-cqc added 2 commits November 6, 2025 08:14

apply_custom: use mutation

9392eca

with_recursive also sets linearize, TODO update docs

6799dc0

acl-cqc marked this pull request as draft November 6, 2025 08:37

acl-cqc commented Nov 6, 2025

View reviewed changes

WIP: try to make linearization behaviour consistent, but breaks test

9a0bec2

acl-cqc commented Nov 6, 2025

View reviewed changes

hugr-passes/src/replace_types.rs Show resolved Hide resolved

acl-cqc added 2 commits November 11, 2025 09:11

Still linearize when replace_recursive is off

936b286

avoid specified root not entrypoint

94b02eb

acl-cqc marked this pull request as ready for review November 11, 2025 09:40

acl-cqc added 3 commits November 14, 2025 19:46

set_replace(_parametrized,)_(op,type) all recursive w/out linearizati…

99f8f3e

…on, deprecate old

op callback returns result; add get_linearizer; change drop-lowering …

0aed24b

…test

Update tests to avoid deprecations

dea15b0

acl-cqc changed the title ~~feat: ReplaceTypes: recursively replace on types too~~ feat: ReplaceTypes: recurse on replacements, much deprecation Nov 14, 2025

acl-cqc commented Nov 14, 2025

View reviewed changes

remove done TODO

0c93191

acl-cqc commented Nov 14, 2025

View reviewed changes

mark-koch approved these changes Nov 18, 2025

View reviewed changes

acl-cqc added 3 commits November 18, 2025 17:36

Remove bogus TODO re. consts

def5e73

refactor: rename ty(=>_and_opts)

e8f489e

change_subtree: optimize as root first; comments

55da1a1

acl-cqc enabled auto-merge November 18, 2025 17:58

acl-cqc added this pull request to the merge queue Nov 18, 2025

Merged via the queue into main with commit 3659749 Nov 18, 2025
31 checks passed

acl-cqc deleted the acl/replace_recursive2 branch November 18, 2025 18:11

hugrbot mentioned this pull request Nov 18, 2025

chore: release #2674

Draft

feat: ReplaceTypes: recurse on replacements, much deprecation #2442

feat: ReplaceTypes: recurse on replacements, much deprecation #2442

Uh oh!

Conversation

acl-cqc commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-koch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc commented Nov 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acl-cqc commented Jul 15, 2025 •

edited

Loading

codecov bot commented Jul 15, 2025 •

edited

Loading