Changed typing to improve type checking in PyCharm

[michel-steuwer]

This change no longer highlights code such as:

def translate_def(ctx: Ctx, op: Operation) -> [Operation]:
    if isinstance(op, choco_ast.FuncDef):
        return translate_fun_def(ctx, op)

def translate_fun_def(ctx: Ctx, fun_def: choco_ast.FuncDef) -> Operation:
...

as a typing error.

With this change PyCharm (and I assume mypy) correctly assumes that the type of op in the branch is choco_ast.FuncDef and not Operation),

[math-fehr]

Here, shouldn't this be typing.Type[OperationType]?
I'm actually not sure why this change would work.

[michel-steuwer]

I think that both syntaxes work just fine. It might be that the newer syntax has been introduced in some recent Python versions.

[math-fehr]

So I'm not understanding something.
Here, from what I understand, op will be an object of type Operation (or any superclass).
However, we want op to be a type that is a subclass of Operation.
Am I understand this right?

[michel-steuwer]

Sory, you are absolutely right.

[michel-steuwer]

This should be typing.Type[OperationType]

[michel-steuwer]

Hm. Now I am confused myself.

[michel-steuwer]

What I wanted to say here is that the argument is a concrete type that is a subtype of Operation

[math-fehr]

So normally, typing.Type[Operation] should work, but typing.Type[OperationType] as well.
As long as you use the type only once, they should be equivalent

[michel-steuwer]

This was indeed a mistake. I fixed this.

[math-fehr]

Besides this one comment, this looks good!

[michel-steuwer]

Btw. Using this change, I am not sure if it fixes all the problems I had ... I am confused again how PyCharm behaves :-(

[math-fehr]

Yeah, I have some other fixes in a different branch that I should PR soon.
Some other changes include the dataclasses import, or addind signatures in some places.
What I would really like to achieve is making PyCharm properly understand the decorators I used, but that's a bit hard

[michel-steuwer]

I realized that we trade here one warning against the other: The code I gave above doesn't warn anymore, but the call to register_op now does. I thought I had taught the type checker that OperationType must be a subtype of Operation but that does not seem to be the case ...

I also can't get the yapf working:

yapf: toml package is needed for using pyproject.toml as a configuration file

[math-fehr]

Where does it warns you exactly in both cases? It's also possible that we made a mistake somewhere.
Also, for yapf, I just use yapf -ir PATH

[michel-steuwer]

yapf works fine in the other project, but not this one ... I am soo not used to working with python.

It warns me when I register the ops for each dialect but more broadly it doesn't understand that a class that has been annotated with the decorator is a subclass of Operation. Before this PR it didn't understand that the annotated class is different to Operation (as the decorator was returning that type).

Writing:

@irdl_op_definition
class Foo(Operation):
  name = "foo"

fixes this, but introduces other warnings ...

[math-fehr]

Yeah, I saw this Operation issue, and I have a patch that should fix it (on the builders branch).
I'm curious, which errors start popping when you add the Operation?

[michel-steuwer]

It just reminds me that I have to implement all abstract methods (which are implemented by the decorator)

[math-fehr]

Okay, so for this, I have a fix, which is simply to remove the need for the decorator.
I'll push this on this branch

[math-fehr]

I fixed a bit the code (PyCharm was complaining on an import), and also added support for Attributes.
Now PyCharm should complain a bit less, what do you think?

[michel-steuwer]

Yes, that looks good.

I assume that the decorator will still work even when we do not make an operation a subclass of Operation?
I.e.:

@irdl_op_definition
class FuncOp:
    name = "buildin.func"

and

@irdl_op_definition
class FuncOp(Operation):
    name = "buildin.func"

will behave the same at runtime?

I don't think this is a big issue. Making the subtyping relationship explicit in the user-written code is a good thing, as it makes the relationship clear (which was not clear to me using the API)!

[math-fehr]

Yes, right now both should work.
I plan to make a future PR that enforce making explicit this inheritance. I definitely agree it would make things clearer