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

Provide decorator for unsafe overrides #5704

Closed
JukkaL opened this issue Oct 1, 2018 · 6 comments
Closed

Provide decorator for unsafe overrides #5704

JukkaL opened this issue Oct 1, 2018 · 6 comments

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 1, 2018

Unsafe overrides are pretty common when annotating legacy codebases. The recommended way to deal with this is to add a # type: ignore comment. This has a few major issues, though:

  1. It's not intuitive. Users often don't realize that they should use it in cases like this.
  2. Users frequently misunderstand # type: ignore and believe that it ignores the type annotation, even though it only ignores the error message.
  3. Users are sometimes confused about the line on which the comment should be placed. Also, it may be unclear what to do if there is an existing comment on the line.
  4. Some users don't like to litter their code with magic comments.
  5. A # type: ignore can ignore unrelated errors that the user doesn't want to be ignored.

I propose to add a decorator that can be used to silence this error, and only this error. Example:

from mypy_extensions import unsafe_override

class Result: ...

class Thing:
    @unsafe_override
    def __eq__(self, other: object) -> Result:  # Result is an unsafe return type
        # ... omitted

This would have these benefits:

  • There is less room for misunderstanding, since the decorator is explicit about what it's used for.
  • It's easier to use, since it doesn't have to be on a specific line.
  • It doesn't look as magical/messy as a magic comment.
  • It only ignores this specific error condition, so it won't hide unrelated errors.
  • It would be easy to Google documentation for the decorator when reading code. Documentation can be explicit about why and when this should be used and provide extended discussion.

See #2783 and #1237 for relevant previous discussions.

@ilevkivskyi
Copy link
Member

Users frequently misunderstand # type: ignore and believe that it ignores the type annotation, even though it only ignores the error message.

I think this is a more general problem. I have seen this some many times in different contexts. It is not possible to rename this, but maybe we can add a large note somewhere early in the docs explaining that # type: ignore only ignores type errors, not types?

@gvanrossum
Copy link
Member

gvanrossum commented Oct 2, 2018 via email

@llchan
Copy link
Contributor

llchan commented Jan 25, 2019

One additional weakness of the # type: ignore workaround that isn't just cosmetic/UX-related:

When the type signatures are different, the overload resolution uses both sets of signatures for the function. An example might go something like this:

class Foo:
    def __eq__(self, x: int) -> Expression:
        assert isinstance(x, int)
        # ...


reveal_type(Foo() == 1)    # Expression
reveal_type(Foo() == '1')  # bool (!!!)

The expected result would be something like

Argument 1 to "__eq__" of "Foo" has incompatible type "str"; expected "int"

I imagine this is the way it is because we've disabled the type check, so there are implicit assumptions in the code that are no longer true. When we go to implement the unsafe_override decorator, it would be good to make shadowing behave "normally" (i.e. completely take over).

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jan 29, 2020

# type: ignore[override] can be used to ignore the error more safely. Perhaps it's sufficient to document this use case more prominently.

@llchan
Copy link
Contributor

llchan commented Feb 6, 2020

I'm less concerned about the ignore on the override itself (I agree the new code-specific stuff will make that safer as far as specificity), but the downstream type signature that seems to be a union of the two signatures, something that I haven't seen elsewhere. Just to reiterate my example with a bit more detail + up-to-date mypy output:

class Expression:
    pass


class Foo:
    def __eq__(self, x: int) -> Expression:  # type: ignore[override]
        assert isinstance(x, int)
        return Expression()

    def eq(self, x: int) -> Expression:
        assert isinstance(x, int)
        return Expression()


reveal_type(Foo() == 1)
reveal_type(Foo() == '1')
reveal_type(Foo().eq(1))
reveal_type(Foo().eq('1'))
main.py:15:13: note: Revealed type is 'main.Expression'
main.py:16:13: note: Revealed type is 'builtins.bool'
main.py:17:13: note: Revealed type is 'main.Expression'
main.py:18:13: note: Revealed type is 'main.Expression'
main.py:18:22: error: Argument 1 to "eq" of "Foo" has incompatible type "str"; expected "int"  [arg-type]
    reveal_type(Foo().eq('1'))  # bool (!!!)

Lines 15 and 16 should either both reveal bool (i.e. use the object.__eq__ signature), or both be Expression and emit an arg-type error like on line 18. It seems like it's treating the override as an overload, which is not what normally happens when a superclass's signature is overridden forcefully:

class A:
    def foo(self, x: int) -> int:
        return x


class B(A):
    def foo(self, x: str) -> str:  # type: ignore[override]
        return x


reveal_type(A().foo(0))  # int
reveal_type(A().foo(''))  # int + arg-type error
reveal_type(B().foo(0))  # str + arg-type error
reveal_type(B().foo(''))  # str

It seems like there must be some special magical handling for __eq__ which is making it override incompletely.

@hauntsaninja
Copy link
Collaborator

I think # type: ignore[override] is a good solution for the original problem here

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

No branches or pull requests

5 participants