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

Error when adding parameters with defaults to override method #682

Closed
alexkoay opened this issue May 22, 2020 · 5 comments
Closed

Error when adding parameters with defaults to override method #682

alexkoay opened this issue May 22, 2020 · 5 comments
Labels
as designed Not a bug, working as intended

Comments

@alexkoay
Copy link

Pyright reports an error due to the increase in parameter count, even though the override method has default values set.

To Reproduce

class Foo:
    def method(self) -> None:
		pass

class Bar(Foo):
    def method(self, opt: bool = False) -> None:
		pass

Bar.method will be flagged with reportIncompatibleMethodOverride error.

VS Code extension or command-line
Using VS Code extension 1.1.37 on Windows 10.

@erictraut
Copy link
Collaborator

This behavior is intended. It is bad design in object-oriented programming to override a parent method with a different signature in a subclass. The signature of the overridden method should match exactly, including optional parameters. That's what this error is designed to detect.

You can choose to disable this error globally or within a single file if you have good reason to do so.

@erictraut erictraut added the as designed Not a bug, working as intended label May 22, 2020
@alexkoay
Copy link
Author

For anybody who might be have encountered the same problem, an alternative that separates the overloading and overriding works as well.

from typing import overload
class Foo:
	def method(self) -> None: pass

class Bar(Foo):
	@overload
	def method(self) -> None: ...

	@overload
	def method(self, opt: bool = False) -> None: ...

	def method(self, opt: bool = False) -> None: pass

@erictraut
Copy link
Collaborator

@alexkoay, while this might make the error disappear, I would not recommend this approach. You're violating an important rule of object-oriented programming here. Methods that are overridden by a subclass should not have a different signature. This diagnostic rule within Pyright is meant to catch and prevent such a violation.

@alexkoay
Copy link
Author

alexkoay commented May 25, 2020

@erictraut, it isn't a violation if you would look at the snippet I shared.

I agree that an overridden method should maintain the same signature. As long as you have an instance of the class or a subclass, calling the method with parent class' signature should always be valid. That is that the first @overload does in the snippet.

On the other hand, subclasses can sometimes require additional (albeit optional) parameters to enhance the handling of said method (method overloading). If we know that a variable is definitely of the subclass, we should be able to do more specific things with it as well. This is the part that comes in with the second @overload, and is separate from the overriding that is taking place.

Referring to the classes above, I hope this snippet can make it clearer.

foo: Foo = Foo()
bar: Bar = Bar()
foobar: Foo = cast(Foo, bar)

# 1. will run, takes no parameters
foo.method()

# 2. runtime error, linted as error
# Foo.method takes no parameters
foo.method(opt=True)

# 3. will run, opt is False by default
# Bar.method should raise incompatibleOverridenMethod if this does not work
bar.method()

# 4. will run, opt is set to True
# Bar.method has an optional parameter
bar.method(opt=True)

# 5. will run, opt is False by default (similar to case 1)
foobar.method()

# 6. will run, but linted as error (similar to case 2)
# type signature is Foo and Foo.method does not take any parameters
foobar.method(opt=True)

@erictraut
Copy link
Collaborator

I understand what you're saying, but I still think this is an ill-advised practice and would strongly encourage a different approach if I were code reviewing this.

If you want to introduce a method in subclass that accepts more parameters, it should have a different name, perhaps calling through to the original method in some cases. It's not an override for the method in the base class because it can be called in different ways from that method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended
Projects
None yet
Development

No branches or pull requests

2 participants