refactor: remove EXTENSION_OPS_WITH_SIDE_EFFECTS, pass effects alongside DataflowOp

[acl-cqc]

closes #1746

• Removes the EXTENSION_OPS_WITH_SIDE_EFFECTS blacklist
• Instead add_op takes an OpWithEffects i.e. a (DataflowOp, Iterable[Effect]). Using a tuple so that factory methods returning both are straightforward.
• Zero change to hugrs, verified just prior to daf68f6 i.e. with the assert that the old blacklist was the same as the value provided with the op.
• Adds a @abstractproperty def call_effects to CallableDef allowing to override for @hugr_op / @custom_function, but this PR maintains the same effects as before including that every user-defined @guppy function has all effects". (We can improve upon both later, for user-defined functions using a callgraph).
• Also add a couple of tests documenting current behaviour of arrays

About 40 tests require the fallback to Effect.ANY if we cannot resolve target of a LocalCall :-(. There are ways we can try and do better but they would add significant complexity, beyond #1748, so no plans yet.

[github-actions]

Bencher Report

Project	guppylang
Branch	acl/order_by_effects
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	66.41 x 1e3 (+0.00%) Baseline: 66.41 x 1e3	67.07 x 1e3 (99.01%)	📈 view plot 🚷 view threshold	4,595.00 (0.00%) Baseline: 4,595.00	4,640.95 (99.01%)
tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile	📈 view plot 🚷 view threshold	28.78 x 1e3 (+0.01%) Baseline: 28.77 x 1e3	29.06 x 1e3 (99.02%)	📈 view plot 🚷 view threshold	1,301.00 (0.00%) Baseline: 1,301.00	1,314.01 (99.01%)
tests/benchmarks/test_queue_push_pop.py::test_queue_push_benchmark_compile	📈 view plot 🚷 view threshold	7.84 x 1e3 (0.00%) Baseline: 7.84 x 1e3	7.92 x 1e3 (99.01%)	📈 view plot 🚷 view threshold	310.00 (0.00%) Baseline: 310.00	313.10 (99.01%)
tests/benchmarks/test_queue_push_pop.py::test_queue_push_pop_benchmark_compile	📈 view plot 🚷 view threshold	10.77 x 1e3 (0.00%) Baseline: 10.77 x 1e3	10.88 x 1e3 (99.01%)	📈 view plot 🚷 view threshold	411.00 (0.00%) Baseline: 411.00	415.11 (99.01%)

🐰 View full continuous benchmarking report in Bencher

[tatiana-s]

Nice! Generally looks good to me in terms of the approach, have left a few more specific comments and also as you mention yourself there are the failing tests. In terms of splitting PRs, I can't see the diff to see the asserts you mention, I guess the question is whether any of them could lead to user facing assertion errors that should be proper errors? If not, I don't see the harm of leaving them in and splitting into a refactor and any changes to ops, however I also think this PR is not so big that splitting is absolutely necessary so I'd also be happy to keep it all in one.

[tatiana-s]

Feels a bit odd to me that there is a specific dataclass for Pure that could just be the DataflowOp but not for the tuple where it might be nice to access things by name rather than index?

[tatiana-s]

And related a lot of the diff seems to be remembering to wrap ops in Pure, what's the main argument for this?

[acl-cqc]

Explicitness, avoids the possibility of passing in a dataflow op without thinking about whether it's pure or not ;).

I admit I could remove this, and argue that I've found them all now, but I think it's good to distinguish 'coz I'm sure we'll be adding more in the future.

A slightly different way would be to avoid the union type and define OpWithEffects = tuple[DataflowOp, Iterable[Effect]] (or even a dataclass rather than tuple), and then have fn pure(d: DataflowOp) -> OpWithEffects: return (d, ())...I'm not sure why I didn't like this originally but could go that way. Any thoughts/preferences?

[acl-cqc]

I've gone for a simple tuple with a factory fn pure (as (op, []) isn't very explicit about what the empty list is), but avoided @dataclass OpWithEffects as that'd have required writing an even-noisier OpWithEffects(op, ...) everywhere and when you see (op, [Effect.ANY]) it's reasonably obvious that the second is a collection of Effects...

[tatiana-s]

I like this better 👍

[tatiana-s]

Just for clarity, is this last node here referring the last in the sense of hierarchy as in the outer most container? I find this method a little hard to understand so may be good to have a few more comments especially if anyone tries to understand it later without much context

[acl-cqc]

I've renamed "last" to "prev" in a way that I hope is more consistent/clear, and added a comment about "get_prev_node" (perhaps too much comment?).

Everything here (except gathering to_propagate and then calling _propagate_side_effects) is at this builder object's level of the hierarchy (builders have parents/grandparents, see fields); I haven't added any more comment about that, if you think that'd help can you suggest something?

[tatiana-s]

Comment looks good to me

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.23810% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.23%. Comparing base (5244a37) to head (fb638b4).

Files with missing lines	Patch %	Lines
...-internals/src/guppylang_internals/tys/__init__.py	61.53%	5 Missing ⚠️
...src/guppylang_internals/compiler/hugr_extension.py	63.63%	4 Missing ⚠️
...uppylang_internals/std/_internal/compiler/array.py	91.30%	2 Missing ⚠️
...nals/src/guppylang_internals/std/_internal/util.py	77.77%	2 Missing ⚠️
.../src/guppylang_internals/compiler/expr_compiler.py	96.66%	1 Missing ⚠️
...pylang_internals/std/_internal/compiler/prelude.py	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2024      +/-   ##
==========================================
+ Coverage   93.21%   93.23%   +0.02%     
==========================================
  Files         152      154       +2     
  Lines       14675    14783     +108     
==========================================
+ Hits        13679    13783     +104     
- Misses        996     1000       +4     

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

[codspeed-hq]

Merging this PR will not alter performance

✅ 11 untouched benchmarks

---

_{Comparing acl/order_by_effects (fb638b4) with main (5244a37)}

[acl-cqc]

@tatiana-s I've pulled the other changes out into #2089 as that PR will do more yet and we can avoid changing effect semantics twice (this doesn't go the whole way to what we'll want for #1747 now). Also I think leaving this as pure refactor means, no reason not to put it in ;). Can you have another look please?

[tatiana-s]

Thanks, looks good to me now and seems like a good idea to get this in as a refactor to start with!

Slight nit, for purposes of a readable change log change "rm" in the title to the full "remove"?