Add "most common base" rewriter

[chrhansk]

This is an attempted fix for #298: It replaces a union of types by the most common (i.e., most specific) base of all types in the union.

[facebook-github-bot]

Hi @chrhansk!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[carljm]

Thanks for the PR! The new rewriter in typing.py looks reasonable, modulo a couple comments, and the test for it in test_typing.py looks good. All the rest of the changes in this PR seems unrelated to the union-rewriter feature, and should be removed from the PR.

The PR should also add documentation for the new rewriter.

[chrhansk]

Thanks for the feedback. I added a test case covering multiple inheritance and fixed up the call to the zip function. I also renamed the rewrite to make the intentions more clear.

The difference with respect to mro in fact revolves around multiple inheritance: The mro states that in case a class is derived from two or more bases, the resolution uses the first of these bases. The rewriter does however enable a partial rewrite even in case of multiple inheritance (see the test_multiple_bases_nontrivial test case), which would not be possible by simply using mro(). It is enable by only returning a partial list of bases, which is cut off as soon as any ambiguous base is reached.

[carljm]

Ah, yeah, makes sense that we must not reduce to a common base via multiple inheritance.

A few more comments on the code, and also please run pipenv run black monkeytype to fix up the formatting using the autoformatter.

Thank you!

[carljm]

Code looks good, thanks for the contribution! Do you feel up to adding some documentation as well? It doesn't need to be extensive, just a mention of the new rewriter and a description of what it does. If not, that's fine, I can add the docs myself, but it may be a couple weeks before I have a chance to do that, and I probably won't merge the PR until docs are ready.

[chrhansk]

I added some docstrings to the rewriter itself as well as an entry in the changelog. Is that sufficient?

[carljm]

Sorry, I wasn't clear enough! The changelog entry and docstrings look great, but what I actually meant was the public-facing documentation (published at https://monkeytype.readthedocs.io/en/stable/) which is in the docs/ subdirectory. Specifically an entry for the new rewriter should be added to the "Type Rewriters" section at the bottom of https://raw.githubusercontent.com/Instagram/MonkeyType/main/doc/generation.rst

[chrhansk]

OK, done.

[carljm]

Thanks, looks great!