Skip to content

@final decorator is not honored on override with --check-untyped-defs #9618

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

Closed
tfranzel opened this issue Oct 20, 2020 · 5 comments · Fixed by #17276
Closed

@final decorator is not honored on override with --check-untyped-defs #9618

tfranzel opened this issue Oct 20, 2020 · 5 comments · Fixed by #17276
Labels
bug mypy got something wrong good-second-issue topic-final PEP 591

Comments

@tfranzel
Copy link

Bug Report

Overriding class methods marked with @final (pep-0591) are not reported as errors. The example script will not produce an error with mypy even though it probably should.

#9612 is potentially related, but they have a final problem in the other direction.

To Reproduce

from typing import final

class Base:
    @final
    def foo(self):
        pass

    def bar(self):
        pass

class Derived(Base):
    def foo(self):
        pass

Your Environment

✗ python --version && mypy --version && mypy foo.py 
Python 3.8.5
mypy 0.790
Success: no issues found in 1 source file
@tfranzel tfranzel added the bug mypy got something wrong label Oct 20, 2020
@a-reich
Copy link

a-reich commented Oct 27, 2020

Hi, just wanted to note that when I tried this example, mypy gives the expected error about final as soon as any annotation is added to Derived's foo method. It's possible that the design was intentionally not to check final for untyped functions? Either way, the workaround is easy.

(There is the --check-untyped-defs option but that does not work to produce the error, which I guess makes sense since the docs say it enables checking the body of a function).

@tfranzel
Copy link
Author

tfranzel commented Nov 1, 2020

hey @a-reich, i cannot reproduce your statement. adding some other decorator didn't change the outcome.

class Derived(Base):
    @cached_property
    @final
    def foo(self):   
        return 2 

also using @final again with derived did nothing:

class Derived(Base):
    @final
    def foo(self):   
        return 2 

@a-reich
Copy link

a-reich commented Nov 1, 2020

@tfranzel Did you look at the mypy-playground example I linked to? Here's the content:

from typing import final

class Base:
    @final
    def foo(self):
        pass

    def bar(self):
        pass

class Derived(Base):
    def foo(self) -> None:
        pass

It's the same as your OP but with a return annotation " -> None" added to Derived.foo. When I said this is fixed by adding an annotation I meant in the usual PEP 484 sense of type annotations, not decorators.

@tfranzel
Copy link
Author

tfranzel commented Nov 1, 2020

ahh sorry i somehow missed the link. i see, so the functionality is there. nonetheless kind of weird to only activate the check once a type hint is present.

@JelleZijlstra
Copy link
Member

I suppose this is because of mypy's general principle to not give errors for unannotated code. It feels like we should give the error if --check-untyped-defs is on, though.

@AlexWaygood AlexWaygood added the topic-final PEP 591 label Apr 1, 2022
@hauntsaninja hauntsaninja changed the title @final decorator is not honored. no error on override. @final decorator is not honored on override with --check-untyped-defs Aug 13, 2023
stroxler added a commit to stroxler/mypy that referenced this issue May 21, 2024
This fixes python#9618 in a fairly aggressive way, by turning on all override
checks for all methods. This means that with this change, MyPy will
complain in cases where just inspecting type signatures involving `Any`
is enough to establish that an override is a type error:
- This includes the case of overriding `@final` as listed in python#9618
- But it also includes arity mismatches, for example when an untyped
  subclass method adds a required attribute not in the parent class
  method.

The tests illustrate this, I added a test case to `check-functions.test`
(is this a good place for it?) for the `@final` check specifically but
also updated a few existing tests of dynamic logic.

The resulting code change is very simple (since all override checks
happen in the same place), and I think conceptually it is right to
enable all these checks because the contract is that MyPy won't
generally type check *bodies* of functions, but the function signatures
are actually part of the *class* type definition which is (in other
cases) sanity-checked even when there are no annotations.

If we think that the arity checks are too big a change, I can work
on a refactor that would let us validate *only* `@final` but not other
properties. Maybe waiting for `mypy_primer` output is the way to go?
stroxler added a commit to stroxler/mypy that referenced this issue May 21, 2024
This fixes python#9618 in a fairly aggressive way, by turning on all override
checks for all methods. This means that with this change, MyPy will
complain in cases where just inspecting type signatures involving `Any`
is enough to establish that an override is a type error:
- This includes the case of overriding `@final` as listed in python#9618
- But it also includes arity mismatches, for example when an untyped
  subclass method adds a required attribute not in the parent class
  method.

The tests illustrate this, I added a test case to `check-functions.test`
(is this a good place for it?) for the `@final` check specifically but
also updated a few existing tests of dynamic logic.

The resulting code change is very simple (since all override checks
happen in the same place), and I think conceptually it is right to
enable all these checks because the contract is that MyPy won't
generally type check *bodies* of functions, but the function signatures
are actually part of the *class* type definition which is (in other
cases) sanity-checked even when there are no annotations.

If we think that the arity checks are too big a change, I can work
on a refactor that would let us validate *only* `@final` but not other
properties. Maybe waiting for `mypy_primer` output is the way to go?
stroxler added a commit to stroxler/mypy that referenced this issue May 21, 2024
This commit fixes python#9618 by making MyPy always complain if a method
overrides a base class method marked as `@final`.

In the process, it also adds a few additional validations:
- Always verify the `@override` decorator, which ought to be pretty
  backward-compatible for most projects assuming that strict override
  checks aren't enabled by default (and it appears to me that
  `--enable-error-code explicit-override` is off by default)
- Verify that the method signature is compatible (which in practice
  means only arity and argument name checks) *if* the
  `--check-untyped-defs` flag is set; it seems unlikely that a user
  would want mypy to validate the bodies of untyped functions but
  wouldn't want to be alerted about incompatible overrides.

Note: I did also explore enabling the signature compatibility check
for all code, which in principle makes sense. But the mypy_primer
results indicated that there would be backward compability issues
because too many libraries rely on us not validating this:
python#17274
hauntsaninja pushed a commit that referenced this issue May 23, 2024
This commit fixes #9618 by making MyPy always complain if a method
overrides a base class method marked as `@final`.

In the process, it also adds a few additional validations:
- Always verify the `@override` decorator, which ought to be pretty
backward-compatible for most projects assuming that strict override
checks aren't enabled by default (and it appears to me that
`--enable-error-code explicit-override` is off by default)
- Verify that the method signature is compatible (which in practice
means only arity and argument name checks) *if* the
`--check-untyped-defs` flag is set; it seems unlikely that a user would
want mypy to validate the bodies of untyped functions but wouldn't want
to be alerted about incompatible overrides.

Note: I did also explore enabling the signature compatibility check for
all code, which in principle makes sense. But the mypy_primer results
indicated that there would be backward compability issues because too
many libraries rely on us not validating this:
#17274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong good-second-issue topic-final PEP 591
Projects
None yet
5 participants