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

Add a mypy plugin to support @total_ordering. #9525

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Apr 13, 2020

Without this, when using a comparison method synthesized by the
@total_ordering class decorator, MyPy will incorrectly observe
something like:

src/python/pants/option/option_value_container.py:108:12: error: Unsupported
left operand type for >= ("Rank")  [operator]
            if new_rank >= existing_rank:
               ^
Found 1 error in 1 file (checked 40 source files)

Here, just __lt__ is explicitly defined in the "Rank" class and
@total_ordering generates __ge__ which MyPy is ignorant of.

Also, in order to get the in-repo plugin working, fix the mypy task to
upgrade its interpreter when --include-requirements is turned on and
remove type checking tags from mypy example targets which are only ever
used in integration tests where tags are not used as filters anyhow.

[ci skip-jvm-tests]

Without this, when using a comparison method synthesized by the
`@total_ordering` class decorator, MyPy will incorrectly observe
something like:
```
src/python/pants/option/option_value_container.py:108:12: error: Unsupported
left operand type for >= ("Rank")  [operator]
            if new_rank >= existing_rank:
               ^
Found 1 error in 1 file (checked 40 source files)
```

Here, just `__lt__` is explicitly defined in the "Rank" class and
`@total_ordering` generates `__ge__` which MyPy is ignorant of.

Also, in order to get the in-repo plugin working, fix the mypy task to
upgrade its interpreter when `--include-requirements` is turned on and
remove type checking tags from mypy example targets which are only ever
used in integration tests where tags are not used as filters anyhow.
[ci skip-jvm-tests]
@jsirois
Copy link
Contributor Author

jsirois commented Apr 13, 2020

A follow-up that converts RankedValue rank integer constants to an Enum will use this to pass type checks without compromise... besides this compromise.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I've never seen a MyPy plugin before. Neat!

Consider trying to upstream this - I think it'd be helpful to others and MyPy seems receptive to contributions like this.

@@ -32,6 +32,7 @@ def cmdline(cls, *, include_requirements):
cmd = [
"--backend-packages=pants.contrib.mypy",
f'--mypy-config={cls.example_dir / "mypy.ini"}',
"--mypy-version=mypy==0.720",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test tests the Django MyPy plugin and that plugin doesn't work with our new, newer, 3rdparty dep so I pinned to the low point of the range it does support. IIRC it was >=0.720,<0.730.

@stuhood
Copy link
Member

stuhood commented Apr 13, 2020

How will this interact with the upstreaming of a potential v2 implementation of mypy?

@jsirois
Copy link
Contributor Author

jsirois commented Apr 13, 2020

@Eric-Arellano it looks like this is already underway:
issue: python/mypy#4610
attempt by issue author: python/mypy#7848
earlier attempt and now the frontrunner: python/mypy#7831

@jsirois
Copy link
Contributor Author

jsirois commented Apr 13, 2020

How will this interact with the upstreaming of a potential v2 implementation of mypy?

Thats alot of speculation that'd I'd normally defer on until something real showed up, but I assume you're being coy about Toolchain's internal v2 impl which actually exists and is slated for upstreaming.

Good question.

There is a small fix in this PR for MypyTask handling of the interpreter to run mypy with when --include-requirements is set. There is a Toolchain internal issue filed to solve that same problem and so that fix will be needed.

After that fix, the only other requirement - which is a requirement in v1 here too - is that the plugin registration function is available on the sys.path for MyPy to find. Since v1 adds all python source roots to the PYTHONPATH (not sure why it does this!), all v1 invocations see the plugin registration function since its in an in-repo loose source form. In v2 we'll need to add a dependency on our MyPy plugin. This will require one of:

  1. Bake it into the Pants rule by injecting the plugin from a resource to the mypy PEX we build and run.
  2. Publishing the MyPy plugin and adding a requirement on it in our [mypy] extra_requirements configuration for the repo.
  3. Like 2, but support requirements that look like //src/python/pants/mypy/plugins or //src/python/pants/mypy/plugins.whl.

Option 3 is what we want at the end of the day (The //src/python/pants/mypy/plugins.whl borrows the Bazel idea of implicit targets where, effectively, the extension is the goal name to execute on the target to obtain a file - this would be implementable in v2 with await Get providing the needed recursion).

@jsirois jsirois merged commit 2d5ce99 into pantsbuild:master Apr 13, 2020
@jsirois jsirois deleted the mypy/total_ordering/plugin branch April 13, 2020 18:45
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.

3 participants