Overriding __eq__ with argument not typed as "object" is allowed

[Tomaz-Vieira]

Describe the bug Implementing __eq__ with an argument whose type is different from object produces no errors. Might be the case for other magic methods as well.

To Reproduce Run the following through pyright:

# pyright: strict

class A:
    def bool_method(self) -> bool:
        return True

    def __eq__(self, o: "A") -> bool: # bad override of object.__eq__ with no pyright error 
        return o.bool_method()

res = A() == 123 # runtime error

Expected behavior An error on the __eq__ definition, indicating that the type of o should be object. Mypy errors with the following:

error: Argument 1 of "__eq__" is incompatible with supertype "object"; supertype defines the argument type as "object

VS Code extension or command-line pyright extension version 1.1.130 pyright command line tool version 1.1.130

[erictraut]

Thanks for the feature request. Pyright already checks for method overrides for all non-dundered methods. I've extended this to include support for all dundered methods as well (with the exceptions of __init__, __new__, and __init_subclass__). The override check for dundered methods is also a little more lax than the normal check — parameter name matches are not enforced. That's necessary because the parameter names for many dundered methods in the typeshed stubs are incorrect and inconsistent. I confirmed that mypy also disables parameter name consistency in this case.

This will be enabled in the next release.

[Tomaz-Vieira]

Thanks for looking into it. I was actually thinking about this, and though def __eq__(self, o: object) is more "correct" from the sense of correct overrides... it feels less useful now that I think about it.

When type-checking the snippet A() == 123, It would be far more helpful to get compiler messages like Class A can only be compared for equality with objects of type A than just getting either a TypeError in runtime or a meaningless default boolean (this is probably less serious for __eq__ than it is for something like __lt__). But maybe this conflicts with something fundamental about python typing? Or maybe typeshed should define the type o to be the same type of self

[erictraut]

Method overrides generally need to follow the Liskov substitution principle. Since the base class (object) defines the type of parameter o as object, it's a violation of the LSP to override __eq__ with a method that chooses a subtype for o. Pyright already validated this for most method overrides, but I excluded dundered methods. This change removes that exclusion (with the few exceptions I noted above).

Yeah, I agree that it would also be useful to receive an error when you attempt to compare two values that have no type overlap. I recently added a check specifically for that. It's off by default, but it's enabled in strict mode. This feature is in the latest version of pyright, and it will be included in this week's release of pylance.

[erictraut]

This is fixed in pyright 1.1.131, which I just published. It will also be included in the next release of pylance.

[layday]

Is it intentional that __eq__ generates a name mismatch error now?

class Foo:
    # Method "__eq__" overrides class "object" in an incompatible manner
    #   Parameter 2 name mismatch: base parameter is named "o", override parameter is named "other"
    def __eq__(self, other: object) -> bool:
        ...

This seems rather redundant since dunders can't be called with keywords arguments. Perhaps the right thing to do is to double-underscore the parameter in the typeshed.

[erictraut]

dunders can't be called with keywords arguments

I wasn't aware of any rule like this. Can you point me to documentation?

[erictraut]

Actually, I already have logic in place to skip the parameter name match for dunders. I'm not able to repro the problem with the sample you posted above. Is there anything else I need to do to repro it?

[layday]

Yes, I omitted # pyright: strict.

[erictraut]

I assumed you were using strict mode. I'm not able to repro it with strict mode either. I've reviewed the code, and it's clearly skipping the param name check for overridden methods when the name of the method is a dunder.

[layday]

That is odd; here's what I have:

❯ nix run nixpkgs.python39 nixpkgs.nodejs-14_x -c npx pyright --verbose foo.py
npx: installed 1 in 1.325s
No configuration file found.
Search paths found for configured python interpreter:
  /nix/store/7pjbbmnrch7frgyp7gz19ay0z1173c7y-python3-3.9.2/lib/python3.9
  /nix/store/7pjbbmnrch7frgyp7gz19ay0z1173c7y-python3-3.9.2/lib/python3.9/lib-dynload
  /nix/store/7pjbbmnrch7frgyp7gz19ay0z1173c7y-python3-3.9.2/lib/python3.9/site-packages
When attempting to get search paths from python interpreter:
  Executing interpreter: 'python3'
  Skipping '/nix/store/7pjbbmnrch7frgyp7gz19ay0z1173c7y-python3-3.9.2/lib/python39.zip' because it is not a valid directory
  Received 3 paths from interpreter
    /nix/store/7pjbbmnrch7frgyp7gz19ay0z1173c7y-python3-3.9.2/lib/python3.9
    /nix/store/7pjbbmnrch7frgyp7gz19ay0z1173c7y-python3-3.9.2/lib/python3.9/lib-dynload
    /nix/store/7pjbbmnrch7frgyp7gz19ay0z1173c7y-python3-3.9.2/lib/python3.9/site-packages
stubPath /Users/layday/Desktop/test/typings is not a valid directory.
Assuming Python platform Darwin
Searching for source files
Found 1 source file
/Users/layday/Desktop/test/foo.py
  /Users/layday/Desktop/test/foo.py:4:9 - error: Method "__eq__" overrides class "object" in an incompatible manner
    Parameter 2 name mismatch: base parameter is named "o", override parameter is named "other" (reportIncompatibleMethodOverride)
1 error, 0 warnings, 0 infos
Completed in 0.595sec

❯ nix run nixpkgs.python39 nixpkgs.nodejs-14_x -c npx pyright --version
npx: installed 1 in 1.39s
pyright 1.1.131

❯ cat foo.py
# pyright: strict

class Foo:
    def __eq__(self, other: object) -> bool:
        ...

❯ ls -a
.      ..     foo.py

[erictraut]

OK, I'm able to repro with the command-line version but not with the language server. Very strange. Investigating now.

[erictraut]

This regression appears only in production builds, and I use debug builds for unit tests and smoke tests, so I didn't catch it. I just did a quick rebuild and republish (version 1.1.132) to fix the problem. Apologies for any inconvenience. Thanks for reporting the problem so I could fix it quickly.

[layday]

Great, thanks for the quick fix :)