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

untyped defs on derived classes #3903

Open
ostcar opened this issue Aug 31, 2017 · 22 comments
Open

untyped defs on derived classes #3903

ostcar opened this issue Aug 31, 2017 · 22 comments

Comments

@ostcar
Copy link

ostcar commented Aug 31, 2017

Currently all arguments and return values on untyped defs are set to Any.

It would be nice, if arguments and return values on derived classes would take the type annotations from the parent class. For example:

class A:
    def foo(self, a: str) -> None: pass


class B(A):
    def foo(self, a):
        reveal_type(a)

In this example the revealed type is Any but I would expect it to be str.

Of cause, if the entire project has type annotations, than this would not be an issue, but when you add type annotations on a big project and start with the base classes, it would be nice, if you could benefit from that.

@emmatyping
Copy link
Collaborator

I believe we probably don't want to support this unless --check-untyped-defs is enabled. Otherwise it would be inconsistent with how things work outside the class. If we restrict it to with that flag I believe this could be a nice feature to add, however this would complicate things and slow mypy.

@gvanrossum
Copy link
Member

gvanrossum commented Sep 2, 2017 via email

@ilevkivskyi
Copy link
Member

+1 from me for a separate flag.

@ilevkivskyi
Copy link
Member

Also for reference, a very similar discussion happened in the typing tracker, see python/typing#269.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 2, 2017

This can't be enabled by default, since a method override can have a different signature from the base class, and blindly inheriting the signature can generate false positives. A separate flag is worth considering, since similar things have been proposed several times.

@emmatyping
Copy link
Collaborator

Thinking more about it, I suppose an override should require a new type signature, as the method body is not inherited with an override, thus it is likely a signature not to be. So Im not sure I think this is such a good feature after all. If you are overriding, it is a different method body, thus inheriting the type signature seems wrong (at least to me).

However, if we do support this, I think a flag is too global, as these changes are likely to be only wanted on a single class or method even, but that is my personal experience on this.

@gvanrossum
Copy link
Member

gvanrossum commented Sep 2, 2017 via email

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 3, 2017

Yes, I think that this only makes sense if we check the body against the inherited signature. This is another reason why this needs to be opt-in -- unannotated methods are not type checked according to PEP 484.

@novoxd
Copy link

novoxd commented Jul 28, 2018

+1 this will be relay nice. Please add this for class variables too.

@OrDuan
Copy link

OrDuan commented Jul 29, 2018

I want to point out that PyCharm added this to their code analyzer.
So far this is really helpful for me.
Usually I have a base class and child class that should have the same signatures, if it is different, I override it with different types.

image

@novoxd
Copy link

novoxd commented Jul 29, 2018

Whats real bad is that previous activity in this tread was about a ear ago, and nothing go on(

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 30, 2018

Okay, here's a more detailed spec of how this could work in case somebody wishes to contribute an implementation:

  • The feature would be enabled through --inherit-signatures.
  • If this option is enabled, a method override will implicitly get a signature derived from base class signature (if one exists). It will also cause the method body to be checked. If the override has an explicit signature, this option has no effect.
  • If the option is on and the overridden method defines additional arguments with default values not present in the base class method, these get implicit Any types.
  • If the option is on and the signature of the overridden method is incompatible with the base class, generate an error. For example, if the base class method is def f(self) -> int: and the override is def f(self, x):, we'd generate an error.
  • Self types may need some special handling.
  • Subclasses of generic classes need special care.
  • This option has no effect on methods that aren't overrides.

@vicrac
Copy link

vicrac commented Dec 5, 2018

May I ask how is the progress on this proposal? I was just about to add new issue duplicationg this one when I found it, here's a code that I wanted to include to mine:


class Base:

    def foo(self, a: float) -> float:
        return a**2

class Derived(Base):

    def foo(self, a):
        return "whoaaa, string!"


a: Base = Derived() # Absolutely valid and reasonable
r: float            # Declare r as float, expected type returned by foo method from Base class
r = a.foo(4.2)      # BUUUM, we can assign a string to a float, mypy doesn't complain
print(r + 0.1)      # Runtime error "TypeError: can only concatenate str (not 'float') to str"

Need to duplicate all signatures in derived classes feels very unhandy to me, for two reasons:

  1. I need to know exactly the type of result of overriden method in base class, which is getting somehow hard when subclassing third party library classes
  2. For the same reason, I need to import explicitly this returned type, and this gets the imports messy

On the other hand, I see that sometimes we would override the method dynamically and don't want the type checker to be concerned about it. Would it better option that instead of passing argument to mypy (--inherit-signatures) we would annotate functions overriden dynamically (which in my opinion happen less often than the statically overriden ones) with some decorator annotation (let's say @dynamic_override)? It would let us have both dynamically and statically overriden ones at the same time without mypy complaining. Is it worth considering?

@ilevkivskyi
Copy link
Member

I am currently using PyCharm for editing Python code, and it has a nice feature: copy signature from superclass. It works well if you first defined a method (with all types) in the superclass and then want to override it in a subclass.

@vicrac
Copy link

vicrac commented Dec 5, 2018

Ok, this is easy to use and worth knowing (besides that actually I am using vscode 😂), but it doesn't address my primary concern that this kind of error (which by the way seems a severe violation of Liskov substitution principle to me) would pass undetected by mypy type checker till runtime fail.

@vicrac
Copy link

vicrac commented Dec 5, 2018

After considering it for a while, I am pretty sure that mypy should generate at least some warning in this case (for example "overriden method return type doesn't conform to base class method signature")

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 5, 2018

Mypy generally doesn't find errors caused by unannotated code, so unannotated method overrides aren't special. There are strictness options to enforce the presence function annotations (--disallow-untyped-defs). If mypy would start checking unannotated code by default, it could make gradual annotation of an existing codebase much harder. If possible, you should annotate as many functions as possible. Otherwise there will be false negatives.

@vicrac
Copy link

vicrac commented Dec 6, 2018

Ok, I generally see that the best solution is to annotate as much as possible (in addition, this is also better for reading code, as I don't have to search for the base class to find method signature). But still, this would be nice to have a choince - annotate or inherit signatures. So, when can I expect this feature to be introduced? Is it being implemented at the moment?

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 6, 2018

We don't know when/if this will be available.

@sandervoerman
Copy link

Is this really a desirable feature for a type checker to have, or is it rather something that would make much more sense as a code completion feature?

The PyCharm feature mentioned by @ilevkivskyi might not cover all use cases, but in the use cases where it works, it seems the superior option. So perhaps instead of a feature request for mypy, this should be a feature request for IDE developers?

Some thoughts:

  • When derived type signatures are added by means of code completion, the analysis only has to be run once, and the results become part of the code, increasing the amount of annotated code.
  • Type annotations provided by code completion may also be used as the basis for modification. The derived class may be used by code that depends on the narrower return types or broader argument types of its overridden methods.
  • If the base type is part of your codebase, then a code completion tool can give you a list of suggested updates to the derived annotations when you refactor the base type (in the same way that PyCharm gives suggestions on how to change the names of derived classes when you rename the base class).
  • If the base type is imported from the standard library or a third-party package, and you used a code completion tool to automate the annotation of derived types in your own program, then when a new version of the library changes the base type, you can be notified of this discrepancy, and check whether it might break your code at runtime.

Kami added a commit to Kami/libcloud that referenced this issue Mar 1, 2020
parent class signature for methods which are overriden in the subclass.

This means we sadly need to rely on annotations from the base class.

See python/mypy#3903 for details.
@MaxHalford
Copy link

Any news on this? I get it that it's not desirable to enforce this globally. Maybe a compromise would be to only enforce this when inheriting from abstract classes that are likely to have type hinting.

@scratchmex
Copy link

Is this just waiting for a contributor?

Pyright and pylance seems to already have it supported as discussed in microsoft/pyright#3094 with documentation here

Can some maintainer give green light so others (or maybe me) can step in? Would be great

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

Successfully merging a pull request may close this issue.