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

mypy plugin broken with mypy==0.730, works with 0.711 #21

Closed
ulidtko opened this issue Oct 8, 2019 · 13 comments
Closed

mypy plugin broken with mypy==0.730, works with 0.711 #21

ulidtko opened this issue Oct 8, 2019 · 13 comments
Labels
bug Something isn't working

Comments

@ulidtko
Copy link

ulidtko commented Oct 8, 2019

Hi! Nice work, thanks for publishing, absolutely appreciated.

Let's take a tiny example:

from adt import adt, Case

@adt
class Cmd:
    READKEY:   Case[str, str]
    WRITEKEY:  Case[str, str, str]
    DELETEKEY: Case[str, str]

This works just fine on the REPL, but the mypy plugin thing isn't giving it to me:

command.py:6: error: "Case" expects no type arguments, but 2 given
command.py:7: error: "Case" expects no type arguments, but 3 given
command.py:8: error: "Case" expects no type arguments, but 2 given
Found 3 errors in 1 file (checked 1 source file)

Of course I followed the README, and had put the lines in the project's setup.cfg:

[mypy]
plugins = adt.mypy_plugin

I'm abolutely sure that the plugin gets loaded: mypy spews errors as expected whenever I garble the config. But the complaints are still there. Python 3.6; mypy 0.730 — the freshest at pypi right now.

Do you see anything I'm missing? Or if you could perhaps retest with recent mypy, perhaps they broke the plugin...

@jspahrsummers
Copy link
Owner

Thanks for trying it out! Sorry for my slow replies (past and future).

Just for due diligence' sake, do you see these issues if you manually install mypy==0.711, like in the requirements.txt? Of course, this would not be a permanent fix, it's just helpful to isolate the problem.

@ulidtko
Copy link
Author

ulidtko commented Oct 11, 2019

Confirmed in a venv, works flawlessly with mypy==0.711, but broken with 0.730.

@ulidtko ulidtko changed the title mypy 0.730 error: "Case" expects no type argument, but 2 given mypy plugin broken with mypy==0.730, works with 0.711 Oct 11, 2019
@kastiglione
Copy link
Collaborator

The 0.720 release notes say:

Plugin API Changes
The new semantic analyzer may require changes to mypy plugins. We have a GitHub issue with more information for maintainers of plugins.

They moved to their new semantic analyzer as the default in this version.

@kastiglione
Copy link
Collaborator

Ah details: python/mypy#6617

@jspahrsummers jspahrsummers added the bug Something isn't working label Oct 11, 2019
@zacchiro
Copy link

zacchiro commented Dec 23, 2019

probably the same issue, but updated console log for mypy 0.750:

$ mypy --version
mypy 0.750

$ mypy part1.py 
part1.py:24: error: "Case" expects no type arguments, but 1 given
part1.py:25: error: "Case" expects no type arguments, but 1 given
part1.py:20: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.rtfd.io/en/latest/common_issues.html#using-a-development-mypy-build
If this issue continues with mypy master, please report a bug at https://github.com/python/mypy/issues
version: 0.750
part1.py:20: : note: please use --show-traceback to print a traceback when reporting a bug

$ mypy --show-traceback part1.py 
part1.py:24: error: "Case" expects no type arguments, but 1 given
part1.py:25: error: "Case" expects no type arguments, but 1 given
part1.py:20: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.rtfd.io/en/latest/common_issues.html#using-a-development-mypy-build
Please report a bug at https://github.com/python/mypy/issues
version: 0.750
Traceback (most recent call last):
  File "mypy/semanal.py", line 4656, in accept
  File "mypy/nodes.py", line 939, in accept
  File "mypy/semanal.py", line 1029, in visit_class_def
  File "mypy/semanal.py", line 1106, in analyze_class
  File "mypy/semanal.py", line 1115, in analyze_class_body_common
  File "mypy/semanal.py", line 1161, in apply_class_plugin_hooks
  File "/home/zack/.local/lib/python3.7/site-packages/adt/mypy_plugin.py", line 115, in _transform_class
    cases = _get_and_delete_cases(context)
  File "/home/zack/.local/lib/python3.7/site-packages/adt/mypy_plugin.py", line 149, in _get_and_delete_cases
    _CaseDef(context=context, name=var.name(),
TypeError: 'str' object is not callable
part1.py:20: : note: use --pdb to drop into pdb

adt is great, so it's kinda of a pity it cannot be properly type checked right now with mypy :-( Is there any workaround for this?

thanks a lot for adt !

@jspahrsummers
Copy link
Owner

There's a workaround in pinning to mypy==0.711, but otherwise the plugin just needs to be refactored for the latest version of mypy. I'm afraid I haven't had bandwidth to work on this, but I'd be happy to review anyone else's attempt in a pull request.

@wchresta
Copy link
Collaborator

wchresta commented Dec 28, 2019

So I have been looking into this for a few days, now. It seems like mypy changed the way and timing of the execution of the hooks. There are three issues (so far) we need to solve:

  • error: "Case" expects no type arguments, but 1 given
    The type analyzer stumbles over the Case usage having more type parameters than the definition. This is because the type is analyzed before the get_class_decorator_hook hook is executed. I was able to solve this by special-casing our Case type by providing a get_type_analyze_hook-hook, which returns a CaseConstructor type instead of Case[..].
  • error: 'L' is a type variable and only valid in type context
    When using Case[T] with a type variable, the semantic analyzer is executed before the get_class_decorator_hook hook. Since the hook is not executed before, it is not removing the type variable and the semantic analyzer complains about the type variable existing in run-time code. I was able to solve this by introducing a get_customize_class_mro_hook-hook which performs the _get_and_delete_cases-operation on the code and stoes the _ClassDef in the plugin state.
  • error: Cannot infer type argument 1 of "match" of "Expression"
    The plugin is unable to correctly resolve the match type when using a type variable in the Case. I'm currently looking into how to solve this.

An alternative would be to request mypy to provide a hook that lets us change class definition in the AST before they run the semantic analyzer. Since I'm still not completely sure how the plugin and mypy works, I have not done this, yet. If I understand more and there is indeed no simpler solution, I will make the feature request. I do expect implementation of the hook to take a while though, so it would be nice if we had a workaround to have adt work with newer mypy versions.

Edit: I realized that some of my test cases are wrong (due to the readme containing a typo) and some are also failing in the current version for mypy 0.711. I'm going to check which errors are actually due to the mypy version and give an update here.

wchresta added a commit to wchresta/adt that referenced this issue Dec 28, 2019
* Add test cases for issues jspahrsummers#21, jspahrsummers#25, jspahrsummers#26
* Test case for jspahrsummers#21 is active because it passes with mypy==0.711
* Test cases for jspahrsummers#25 and jspahrsummers#26 are deactivated because they fail
wchresta added a commit to wchresta/adt that referenced this issue Dec 28, 2019
* Add test cases for issues jspahrsummers#21, jspahrsummers#25, jspahrsummers#26
* Test case for jspahrsummers#21 is active because it passes with mypy==0.711
* Test cases for jspahrsummers#25 and jspahrsummers#26 are deactivated because they fail
wchresta added a commit to wchresta/adt that referenced this issue Dec 28, 2019
* Add test cases for issues jspahrsummers#21, jspahrsummers#25, jspahrsummers#26
* Test case for jspahrsummers#21 is active because it passes with mypy==0.711
* Test cases for jspahrsummers#25 and jspahrsummers#26 are deactivated because they fail
@jspahrsummers
Copy link
Owner

jspahrsummers commented Dec 28, 2019

@wchresta I took a cursory glance at these, based on your (very helpful) write-up, and in conjunction with the mypy plugin notes (found via the link @kastiglione shared).

The type analyzer stumbles over the Case usage having more type parameters than the definition. This is because the type is analyzed before the get_class_decorator_hook hook is executed.

I wasn't able to confirm this one--when adding some logging into adt's get_class_decorator_hook and _transform_class, it appears those functions are both invoked before the type errors are printed by mypy. Interestingly, _transform_class never ends up completing on file issue21.py.

My hypothesis is that type analysis is unintentionally getting triggered by adt while the class definition is being modified. I haven't dug deeper than that, though.

Sorry for the poorly-documented code, and wherever conventions in the project might be weird! Ironically, I wasn't very experienced in Python while building this. 😅

@wchresta
Copy link
Collaborator

@jspahrsummers yeah, at first it looked to me like that, too. I then took a debugger to observe mypy's internals. It seems like the output of the error messages might be buffered; the fail method seems to be called before our hook gets executed.

The reason for this is here:
https://github.com/python/mypy/blob/25c993be1007a09baac5d95c1d2bfce779055ad3/mypy/semanal.py#L1114-L1115

The type analyzer with raises the failure is ran when .accept() is called. But the decorator hook is only called after.

Anyway, it seemed like this is the only real problem with the new semantic analyzer. The other problems were either wrong syntax on my part or have already existed.

@wchresta
Copy link
Collaborator

wchresta commented Jan 7, 2020

Should be fixed with #28

@radix
Copy link

radix commented Aug 27, 2020

I'm just dipping my toe into type-checking with mypy and adt, but I seem to still be getting this error with mypy 0.782. Can anyone else confirm that this bug is fixed?

stacks/models.py:2627: error: "Case" expects no type arguments, but 1 given
stacks/models.py:2631: error: "Case" expects no type arguments, but 1 given

@radix
Copy link

radix commented Aug 27, 2020

oh, never mind, it's because I didn't have the plugin enabled!

@ulidtko
Copy link
Author

ulidtko commented Aug 27, 2020

@radix oh... can confirm!

Upd: doesn't matter
ulidtko@pasocon ~ > cat test.py
from adt import adt, Case

@adt
class Cmd:
    READKEY:   Case[str, str]
    WRITEKEY:  Case[str, str, str]
    DELETEKEY: Case[str, str]

ulidtko@pasocon ~> mypy test.py
test.py:5: error: "Case" expects no type arguments, but 2 given
test.py:6: error: "Case" expects no type arguments, but 3 given
test.py:7: error: "Case" expects no type arguments, but 2 given
Found 3 errors in 1 file (checked 1 source file)

ulidtko@pasocon ~ [1]> mypy --version
mypy 0.782

Haha, I've fallen into the same trap again :)

Sure, you need to add this into setup.cfg (which doesn't exist of course when testing on standalone file):

[mypy]
plugins = adt.mypy_plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants