Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: a type comment to ignore expected errors #8655

Open
bluetech opened this issue Apr 10, 2020 · 11 comments
Open

RFC: a type comment to ignore expected errors #8655

bluetech opened this issue Apr 10, 2020 · 11 comments
Labels

Comments

@bluetech
Copy link
Contributor

bluetech commented Apr 10, 2020

Problem

Python is optionally typed, and a lot of type-annotated libraries also add runtime type checks to prevent misuse by users who do not type-check their own code. For example:

def launch_missile(name: str) -> None:
    if not isinstance(name, str):
        raise TypeError("name is not an str")
    ....

If the library then wants to test this check, mypy complains:

def test_launch_missile_only_accepts_str() -> None:
    with pytest.raises(TypeError):
        launch_missile(10)  # error: Argument 1 to "launch_missile" has incompatible type "int"; expected "str" [arg-type]

The error is legitimate and expected, but cannot be left alone because it causes mypy to fail. The usual way to fix this is to tell mypy to ignore it:

launch_missile(10)  # type: ignore[arg-type]

This solution for the most part works great, but is not ideal:

One problem with it is that if something in the type of the line changes unintentionally such that the type error no longer triggers, the type: ignore comment doesn't do anything, silently. Hence it is not a good vehicle for doing "type-level assertions".

mypy provides also solution for this unused-ignore problem -- the warn_unused_ignores option. But this in itself is also not ideal: it can be specified either globally or per-file, but that is too coarse. Sometimes there are type: ignores which only conditionally trigger, depending on e.g. python version, dependency version or platform, so warn_unused_ignores cannot be set.

Another problem with using type: ignore for "type-level assertions" is that the intent is not self-evident - it is not immediately clear whether it is meant as an assertion or as a workaround/TODO/I-know-this-is-bad-but-I-am-doing-it-anyway.

When working on adding (inline) type annotations to the pytest project, and also in my personal projects, I have wanted this often.

Solution

Add a new type: ignore-expected directive (also accepts error codes type: ignore-expected[arg-type,operator]).

Semantically, this directive is exactly the same as type: ignore, except that it triggers an error about being unused even if warn_unused_ignores is not set, and has a differentiated error message unused 'type: ignore-expected' comment.

For type-errors that are "asserted"/expected, type: ignore-expected should be used. For other cases, type: ignore should continue to be used.

Practical matters

A POC implementation is available at https://github.com/bluetech/mypy/tree/type-ignore-expected. The implementation is not polished, and is lacking comprehensive tests and documentation, but is meant to show feasibility.

Above I proposed to add a new directive. The existing directive type: ignore is actually parsed and is a part of the Python AST (since Python 3.8, or in typed-ast for earlier versions). It is represented as a TypeIgnore AST node (link). If this idea is accepted, then hopefully the TypeIgnore node can be extended with a expected: bool attribute. In the mean time, due to the loose why the comment is apparently parsed by the parser, it is possible to handle any way in a hacky way, as the PR does. Such an approach can also be used to provide seamless backward compatibility.

Prior work

When researching this I have found out that TypeScript has just added support for exactly this (not in a released version yet at the time of writing), see https://devblogs.microsoft.com/typescript/announcing-typescript-3-9-beta/#ts-expect-error-comments. The equivalent to their directive name would be type: expect-error[...], but is otherwise the same as far as I can tell.

@JelleZijlstra
Copy link
Member

Another option is to write the test as launch_missile(cast(str, 10)).

@IanVS
Copy link

IanVS commented Feb 12, 2021

Another option is to write the test as launch_missile(cast(str, 10)).

But then wouldn't that prevent the test from executing the way it was intended? The exception would no longer be raised and the test would fail, right? I'm just learning Python, so maybe I'm wrong.

@Dreamsorcerer
Copy link
Contributor

But then wouldn't that prevent the test from executing the way it was intended? The exception would no longer be raised and the test would fail, right?

No, the cast() function is for static typing. At runtime it returns the second argument unchanged:

>>> cast(str, 10)
10

The cast function tells mypy that we know this variable is actually a str, although in this case we are simply lying to it.

If you wanted to actually convert the type, you would use str():

>>> str(10)
'10'

@mwgamble
Copy link

mwgamble commented Aug 9, 2021

That isn't an ideal solution. It doesn't solve the underlying problem, which is that the type-checker should enforce that there is a type error on the given line, and only raise an error to the user if there is no longer a type error on that line. Using cast() is just a blanket "make my problems go away" tool.

@JelleZijlstra
Copy link
Member

@mwgamble there is a --warn-unused-cast flag that will flag an error if the cast is no longer necessary.

@mwgamble
Copy link

mwgamble commented Aug 9, 2021

But that's all a roundabout messy way of solving the problem, rather than supporting something similar to what Typescript has supported for quite a while now.

@Dreamsorcerer
Copy link
Contributor

@mwgamble there is a --warn-unused-cast flag that will flag an error if the cast is no longer necessary.

Ohhh, how long has that been around for? I can't find it listed in the docs.

@JelleZijlstra
Copy link
Member

Sorry, it's actually --warn-redundant-casts (https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-warn-redundant-casts).

@Dreamsorcerer
Copy link
Contributor

Oh, yes, I have used that option. :P

@rattrayalex
Copy link

For others interested in this feature, pyright has a configuration option: reportUnnecessaryTypeIgnoreComment

@JelleZijlstra
Copy link
Member

@rattrayalex mypy has an equivalent flag (https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-warn-unused-ignores), but as the OP explains, it does not fully cover the use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants