Using # type: ignore with mypy and alternatives

[davidorme]

mypy has some limits on what it can infer about types from the code and so occasionally reports typing errors that are 'incorrect'. One way of removing those is to add # type: ignore on the affected line and that stops it. We should never do that just to get mypy to shut up but always with a comment explaining why mypy is incorrect. We can also extend # type: ignore to make mypy only ignore specific typing errors on a line by using mypy codes: # type: ignore [union-attr]

There is an example in the code addressed this comment #122 (comment):

• a function argument can be None
• if it is None it is set to a value
• something is then done with the argument but mypy thinks it can still be None and objects.

However, in that comment @alexdewar pointed out another way of muting mypy using either typing.cast and it turns out you can also use assert to update mypy's knowledge of what types apply to a variable.

At the moment, we have a few cases, all of which are solved using # type: ignore but we should do this consistently. Thoughts?

[jacobcook1995]

The only place I've used # type: ignore is on package imports for packages that don't have type hints (even on typeshed), where it seems pretty unavoidable. My concern with using # type: ignore is whether there are downstream consequences to using it. Obviously in some cases ignoring types is completely unavoidable (e.g. importing packages with no type hints) but worry about not propagating type information through the model. Like will it lead to more annoying mypy errors downstream, or large enough portions of the model not having types that it's questionable why we are using mypy at all?

[davidorme]

I don't think it leads to downstream errors - it only turns off the check for that specific line. In general, we should probably first look at rewriting the code and typing to avoid the issue - it is an indication there might be better practice and if we're having to # type: ignore the same issue repeatedly through a method or function then that is not the best way of doing something.

There are cases where it is difficult to avoid - I think the case in the link is one. I guess one of the features of using assert or cast is that it 'updates' mypys knowledge and so applies for the rest of the method/function, not just the line, and that seems like it could mask the fact that there is a widespread issue with the structure? Where the comment is explicitly local.

[alexdewar]

Yeah, it shouldn't cause downstream errors. If you've annotated a function's return type then I think mypy will just assume that that's what's being returned, so any callers shouldn't have any problems.

[alexdewar]

I think there are a couple of considerations here. Casting is basically a no-op, so if you really know for sure, e.g. that a variable x cannot be None you should just cast it. If it could be None then mypy is doing its job by complaining, in which case you should check for this: an if statement or an assert will do the trick. I wouldn't use assert to check if a variable is None if you know it can't be. While it might make mypy happy it will be a bit confusing for people reading your code and, as I said, casting is a bit cheaper anyway.

[davidorme]

If I've got this right, then the cast option has no impact at all on the running code and in fact can be stripped out - such as if python -OO is used (or at least assert definitely is). That is not the case for using if x is not None: of course.

So those lines of code are only there to inform mypy - and we can do the same thing with # type: ignore [explicit-thing-we-are-ignoring]. Those comments have no impact at all on the code, they just inform the mypy checking when it is running. I don't understand what the advantages are of using the code based solution over the comments?

[alexdewar]

Yes, you're right -- casting should be more or less free (like adding # type: ignore). Sorry, I think we might have got our wires crossed here.

You just want to be sure that your casting is correct, e.g. don't cast away an Optional[] if the result might actually be None. In that case you obviously should check for this explicitly.