Skip to content

Incorrect "implicitly abstract" function when function returns None and implementation is 'pass' #13770

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
cmeyer opened this issue Sep 29, 2022 · 9 comments · Fixed by #13776
Labels
bug mypy got something wrong

Comments

@cmeyer
Copy link

cmeyer commented Sep 29, 2022

Bug Report

To Reproduce

import typing

class A(typing.Protocol):
    def require_method(self) -> None: ...

    def optional_method(self) -> None:
        pass

class B(A):
    def require_method(self) -> None:
        print("B")
    
a = B()

Expected Behavior

This is valid code. The optional_method is optional and is implemented with a valid do-nothing body. It doesn't make sense to return None from this function.

Actual Behavior

% mypy --strict file.py
file.py:13: error: Cannot instantiate abstract class "B" with abstract attribute "optional_method"
file.py:13: note: The following method was marked implicitly abstract because it has an empty function body: "optional_method". If it is not meant to be abstract, explicitly return None.
Found 1 error in 1 file (checked 1 source file)

Your Environment

  • Mypy version used: 0.981
  • Mypy command-line flags: --strict
  • Mypy configuration options from mypy.ini (and other config files): None
  • Python version used: 3.10.6
@cmeyer cmeyer added the bug mypy got something wrong label Sep 29, 2022
@cmeyer
Copy link
Author

cmeyer commented Sep 29, 2022

This was implemented in this patch #12118

It uses a weak heuristic to guess whether a method is abstract and gives no option (that I can find) to disable the guess. How about a a command line option to disable "type checks" that makes wild guesses about the author's intention?

@erictraut
Copy link

FWIW, pyright treats ... differently from pass when determining whether a protocol method is considered "implicitly abstract", so it does not produce an error for the above code. Apparently mypy treats both ... and pass the same in this regard. Was this intentional? Would it make sense for mypy to treat pass differently here?

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 29, 2022

I think that treating pass as not being implicitly abstract could be reasonable. One reason for the current behavior may be that Python 2 didn't have ... as a literal, but I'm not sure.

@ilevkivskyi @thomkeh What do you think?

@tmke8
Copy link
Contributor

tmke8 commented Sep 29, 2022

I think that treating pass as not being implicitly abstract could be reasonable.

Yes, I agree. The reason why I did it this way was simply that mypy already had the is_trivial_body function and I re-used it.

In addition to ... and pass, the is_trivial_body function also returns true for function bodies that only contain a docstring and I think also those that only contain raise NotImplementedError:

def is_trivial_body(block: Block) -> bool:

Oh, by the way, I initially treated functions returning None and Any differently, but that functionality was removed during code review

@ilevkivskyi
Copy link
Member

I don't like an idea of having a flag for this. I also don't like deciding implicit abstractness depending on the return type (optional vs non-optional). We just need to agree on such a set of rules about what is implicitly abstract that would satisfy everyone.

Anecdotally, there are some post-Python 2 code bases that still have pass in protocols. So if we will now exclude pass, those will generate errors (unless return type is optional). We can of course try and see what will be the output from mypy_primer.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Sep 29, 2022

I'm pretty happy with the current rules. The error message is clear The following method was marked implicitly abstract because it has an empty function body: "optional_method". If it is not meant to be abstract, explicitly return None. and I don't get the big deal over doing:

    def optional_method(self) -> None:
        return None

It much more clearly communicates something is different than the difference between "..." and "pass". So I'd vote to close this issue.

@cmeyer
Copy link
Author

cmeyer commented Sep 29, 2022

To many programmers, particularly from other languages like C++, this goes against common programming idioms. Returning a value from a function typed as void/None is an error.

The ellipses are fine. Why does there need to be more heuristic than that?

Having spurious return statements in functions that are not supposed to return a value is going to lead to readability and other bugs.

I think there's an argument to be made that it is a typing error when functions declared as None return any value, even None.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Sep 29, 2022

Returning a value from a function typed as void/None is an error ... typing error when functions declared as None return any value, even None

This also works:

    def optional_method(self) -> None:
        return

I'd be in favour of changing the error message to say "explicitly return or return None", if that helps address the concern.

@cmeyer
Copy link
Author

cmeyer commented Sep 30, 2022

A return without a value is a reasonable workaround.

I still think pass should also work; but I've already updated our code and this isn't a big issue for me now.

FWIW this overall feature caught a few edge cases (actual missing abstract methods) in our code that I fixed too, although they weren't actually reachable. Thanks!

hauntsaninja added a commit to hauntsaninja/mypy that referenced this issue Sep 30, 2022
jvff added a commit to jvff/wit-bindgen that referenced this issue Nov 3, 2022
Some methods became imlicitly abstract when the body is empty (using
just `pass`). The work around seems to be to replace `pass` with
`return`.

More information can be seen on the relevant issue:
python/mypy#13770
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants