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

Handle empty bodies safely #8111

Closed
wants to merge 9 commits into from

Conversation

ilevkivskyi
Copy link
Member

Fixes #2350
Fixes #8005

The implementation is mostly simple, except overloads are a bit tricky and there is still little unsafety can slip through (we can fix it later with another function flag if it will look like worth it, see TODO item in a test). The actual diff is quite small but there are tons of tests, because there are many possible combinations and corner cases, and I wanted to test as much of them as possible.

I am following the logic I previously proposed in #2350. I copy it here FTR:

  • Properly check empty bodies as a default
  • Add an --allow-empty-bodies flag, we need it for tests (hundreds would fail otherwise, also in other projects like sqlalchemy-stubs and django-stubs)
  • Allow empty bodies in stub files (obviously)
  • Allow empty bodies if method is decorated with @abstractmethod
  • In protocols make functions with empty bodies implicitly abstract (unless this is a stub)
  • Add a flag for empty-bodied abstract methods to prohibit calling them via super() (unless they come from a stub)

Note that the stub exceptions in two last points can be summarized as: since we don't know the actual body for something that comes from a stub file, we choose to not give a false positive and stay silent.

Another note is that after #8102 is merged we can turn the hidden flag into an error code, but TBH I am not sure it is necessary. It looks like the flag is more handy, and it makes sense (and feels natural) to use the existing error code [return] (missing return statement) for the unsafe empty bodies.

I am going to try this soon against internal codebases to see if there are some problematic corner cases missing and make any adjustment + tests if needed. Note that mypy/visitor.py has an existing unsafety that this new logic uncovers (there is an existing TODO about this). This requires turning warn_no_return flag off for this file.

Finally, there were couple tiny bugs discovered by tests I added, so I just fixed them in this same PR.

@ilevkivskyi
Copy link
Member Author

Actually I decided not to do this. I will not have time, I will leave the branch if someone wants to work on this.

@ilevkivskyi ilevkivskyi closed this Dec 9, 2019
@TH3CHARLie
Copy link
Collaborator

I am going to try to work on this if no one else is currently doing the same.

ilevkivskyi added a commit that referenced this pull request Sep 29, 2022
Fixes #2350

This essentially re-applies #8111
modulo various (logical) changes in master since then.The only important
difference is that now I override few return-related error codes for
empty bodies, to allow opting out easily in next few versions (I still
keep the flag to simplify testing).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending a Protocol leads to unsafe behavior Handle empty function bodies safely
2 participants