xdslproject / xdsl

A Python Compiler Design Toolkit
Other
255 stars 68 forks source link

Misleading Error Messages when Constructing Operations Wrong #561

Open AntonLydike opened 1 year ago

AntonLydike commented 1 year ago

Hey all, I have encountered this issue often enough to know what's the problem when I see this error message, but for beginners it's probably devastating:

>>> from xdsl.ir import Block
>>> from xdsl.dialects import arith
>>> cst = arith.Constant(1, 32)
>>> b = Block.from_ops([cst])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/anton/uni/xdslproject/xdsl/xdsl/ir.py", line 799, in from_ops
    b.add_ops(ops)
  File "/home/anton/uni/xdslproject/xdsl/xdsl/ir.py", line 868, in add_ops
    self.add_op(op)
  File "/home/anton/uni/xdslproject/xdsl/xdsl/ir.py", line 859, in add_op
    self._attach_op(operation)
  File "/home/anton/uni/xdslproject/xdsl/xdsl/ir.py", line 845, in _attach_op
    raise ValueError(
ValueError: Can't add to a block an operation already attached to a block.

We should probably make sure that the __init__ of an Operation fails when it is constructed in such a way.

Explanation for everyone who's confused by this:

webmiche commented 1 year ago

Hmm yeah that is nasty. I guess it comes from the dataclass decorator on operation...

This particular problem should be solved, once we actually move to using the init constructor for operations, but it is certainly good to keep track here :)

webmiche commented 1 year ago

As a quick fix, one could certainly add some verification to the post_init of Operation. At least that is what I would do :)

AntonLydike commented 1 year ago

I'm confused that the dataclass operator even allows assigning a 1 to something annotated Block | None...

webmiche commented 1 year ago

Welcome to Python!

I personally genuinely think there will be a moment where I will be annoyed enough by annotations not being enforced that I will hack into the python interpreter and give it an execution model that adds assertions on the types at the start of all functions with type hints. But that day is not today.

AntonLydike commented 1 year ago

Suggestion to solve this:

Add a __post_init__ method to the Operation class that checks that the object was initialized correctly. This should be relatively straight forward, so I'm adding the good first issue label to this.