Feature request: Type annotations

[marvinvanaalst]

Feature request: type annotations

First, thanks for the great work you folks have done here!

I've transitioned most of my Python code onto using type annotations and I much enjoy having them checked statically. Is there any interest in the community here for that (or at least in the common APIs, such as IRBuilder)?

I'd volunteer to go through the chore of adding them in if folks are interested.

Cheers

[sklam]

Yes, we would like to enable typing annotations. It will likely help users catch certain kind of mistakes early. Contributions are welcomed.

[marvinvanaalst]

Ok, cool!

I've started working on this in my own fork, but it needs some more work. The ir submodule is probably fine, but for the binding submodule I honestly just bailed most of the time and set types to Any. Since I'm not really familiar with all the internals there ought to be false types in here, so someone should definitely check them. Everything passes mypy, so they are at least consistent, but I cannot guarantee that they aren't consistently wrong :smile: Also note that I have to use a way to high amount of type: ignore's in there, but at least the function signatures for now seem to work.

I'm trying to minimize actual code changes, but I'm taking the liberty to do some of them. For example, the block property in IRBuilder was defined like

@property
def block(self):
    block = self._block

so in functions like these, we would get an AttributeError, since None doesn't have .parent.

class IRBuilder:
    def function(self) -> values.Function:
        return self.block.parent  # <- block can be None

So to avoid having to write is not None checks everywhere, I changed the definition to

@property
def block(self) -> values.Block:
    block = self._block
    if block is None:
        raise AttributeError("Block is None")
    return block

which should lead to exactly the same behaviour as before, avoiding Hyrum's law.

I also changed some things in which the current behaviour is worse than running into Hyrum's law, e.g.

class VectorType(Type):
    def __eq__(self, other):
        if isinstance(other, VectorType):
            return self.element == other.element and self.count == other.count

which implicitly returns None in the case where other is not VectorType. So here I now return False instead and just hope that no one guarded themself with a is not None check here.

class VectorType(Type):
    def __eq__(self, other: object) -> bool:
        if isinstance(other, VectorType):
            return self.element == other.element and self.count == other.count
        return False

Something that needs to be discussed is some of the mixins, e.g. _StrCaching.

class _StrCaching:
    # FIXME: self.__cached_str missing
    # FIXME: self.to_string() missing

    def _clear_string_cache(self) -> None:
        try:
            del self.__cached_str  # type: ignore
        except AttributeError:
            pass

    def __str__(self) -> str:
        try:
            return self.__cached_str  # type: ignore
        except AttributeError:
            s = self.__cached_str = self._to_string()  # type: ignore
            return s  # type: ignore

Mypy doesn't understand that this assumes that __cached_str and to_string will be present in the mixed-in class.

Similarly, Value doesn't implement type or _get_reference, but a lot of functions are typed to take Value as an input. Do I understand correctly that Value itself should never be instantiated? In that case I'd suggest making it an ABC to make that explicit. So something along the lines of

class Value(ABC):
    @property
    @abstractmethod
    def type(self) -> types.Type:
        ...

    @abstractmethod
    def get_reference(self) -> str:
        ...

    def __repr__(self) -> str:
        return "<ir.{0} type='{1}' ...>".format(
            self.__class__.__name__,
            self.type,
        )

I have other stuff, but this post is already too long, so I'll leave it at the for now and check out how your responses to this are :sweat_smile:

[marvinvanaalst]

Quick nudge: has anyone had time to take a look at the proposed code? :)