core: use with blocks for implicit building

[superlopuh]

One thing that I'm unhappy about with the current implicit builder APIs is that one has to first define the function that builds the region, and then construct the operation with that region. This is less than ideal, as they are pretty closely tied together, so the visual separation can make things harder to parse. I tried solving this first with higher-order functions but that got messy pretty quickly.

This PR introduces an alternative to this, which I now think is superior, and would like to transition to for the whole codebase if we decide to accept this PR:

Before:

@Builder.implicit_region(multiply_transpose_type.inputs)
def multiply_transpose(args: tuple[BlockArgument, ...]) -> None:
    a, b = args

...

toy.FuncOp(
    "multiply_transpose",
    multiply_transpose_type,
    multiply_transpose,
    private=True,
)

After:

with toy.FuncOp.implicit_builder(
    "multiply_transpose", multiply_transpose_type, private=True
) as (a, b):

The new approach gets rid of Python functions altogether, exposes args as part of the with block API, brings the order in which the operations are created in Python in line with their order in the IR. If we migrate all our functions this way, it'll let us have an API where the user doesn't need to import the Builder at all, only using helpers on Operations.

[codecov]

Codecov Report

Patch coverage: 92.53% and project coverage change: +0.15 🎉

Comparison is base (680c7f4) 86.92% compared to head (5380055) 87.07%.

❗ Current head 5380055 differs from pull request most recent head fc2f2cc. Consider uploading reports for the commit fc2f2cc to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
+ Coverage   86.92%   87.07%   +0.15%     
==========================================
  Files         129      127       -2     
  Lines       19799    19546     -253     
  Branches     3005     2976      -29     
==========================================
- Hits        17210    17020     -190     
+ Misses       2079     2023      -56     
+ Partials      510      503       -7     

Impacted Files	Coverage Δ
xdsl/dialects/pdl.py	85.37% <63.63%> (+0.20%)	⬆️
xdsl/builder.py	93.57% <92.85%> (-1.13%)	⬇️
docs/Toy/toy/dialects/toy.py	67.69% <100.00%> (+0.88%)	⬆️
docs/Toy/toy/tests/test_ir_gen.py	100.00% <100.00%> (ø)
tests/dialects/test_pdl.py	100.00% <100.00%> (ø)
tests/interpreters/test_pdl_interpreter.py	92.75% <100.00%> (-0.58%)	⬇️
xdsl/dialects/func.py	95.70% <100.00%> (+1.22%)	⬆️
xdsl/ir.py	86.31% <100.00%> (ø)

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[webmiche]

I like it, nice idea!

This will require all ops with regions that want access to this functionality to override implicit_builder, right? Is there a way to autogen this? Those you add here all look somewhat biolerplatey 🤔

[superlopuh]

Yeah, generating the builder automatically would be nice. One way that I know would work would require the region to be the first parameter of the init, not sure if we'd be ok imposing such a constraint. We can look for further sugar after merging this, if everyone's ok with it.

[superlopuh]

@webmiche I added back a lambda-based factory, WDYT?

[webmiche]

Nice, yes I like it 👍

I think imposing constraints on the arg order of init is not something we want to do. Let's merge this version and see if we find something better later on.

[superlopuh]

Great, I'll just wait for @math-fehr to have a look at it when he's back, there's no particular rush to merge this in

[math-fehr]

Really nice! That looks good!

[AntonLydike]

Looks pretty good, just missing some documentation I think

[AntonLydike]

😱 That's awesome!

[AntonLydike]

I'm not 100% sure this is the correct way of doing it. I'd rather see a specific sentinel value?

But then, I'm not sure how that interacts with the type system. Maybe that can be a literal of some sort?

I guess the main problem with this is that people will want to instantiate this class, so I'd recommend adding an error to the __init__ that this class is not to be constructed. But that feels quite silly to me...

[superlopuh]

I've seen it done this way before, and I feel like this is good enough. There's a proposal for a sentinel: https://peps.python.org/pep-0661/ it's a draft, we can use it when it gets accepted, I guess