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

PyCharm autodetection of trio.* not working #314

Closed
parity3 opened this Issue Sep 5, 2017 · 16 comments

Comments

Projects
None yet
4 participants
@parity3

parity3 commented Sep 5, 2017

It would be nice if PyCharm could resolve things like trio.run, and pretty much everything else.

In the below code snippet, PyCharm is unable to find run:

import trio

run = trio.run

I don't know how to get around this without either a pre-compile step in the trio package release process (which messes up a ton of other things like line number mappings to the master files for example), or doing a bunch of stubs in trio/__init__.py and having to maintain them. PyCharm MAY have a way to get around this (they have done some magic with Django's ORM in detecting Model.objects.* methods but they have 'built-in' support considering Django is so popular) but I have no idea where to begin.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 5, 2017

Yeah, the question here is how PyCharm is actually getting its list of module attributes for autocompletion. It'd be nice to fix this, and I can't blame PyCharm for being confused by our current extremely complex export logic, but without knowing what they need it's hard to fix :-). We're obviously not going to actually literally define all of our exports directly in trio/__init__.py, but almost no packages do that so surely they don't require it...

@parity3 to confirm -- do you have this problem for everything in trio and also everything in trio.hazmat?

@parity3

This comment has been minimized.

parity3 commented Sep 5, 2017

A number of names ARE detected; just not all of them. Here is a screenshot from a brand new project:

trio_pycharm

I hope that info can crack the case, but to me, from the outside, it seems rather arbitrary what PyCharm is picking up and what it's not.

As far as trio.hazmat, it doesn't pickup any sub-names at all.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 5, 2017

Ok, so it looks like PyCharm understands:

from ._version import __version__

and it understands

from ._timeouts import *
__all__ += _timeouts.__all__

(I guess probably it's just picking up on the first line there and ignoring the __all__ part.)

What it doesn't understand is:

for _symbol in _core.__all__:
    _value = getattr(_core, _symbol)
    if getattr(_value, "_hazmat", False):
        setattr(hazmat, _symbol, _value)
        hazmat.__all__.append(_symbol)
    else:
        globals()[_symbol] = _value
        __all__.append(_symbol)
del _symbol, _value

which... you know... fair enough, I guess. But this explains the whole pattern you see for trio and trio.hazmat.

And then _core's exports are also a mess of runtime generated stuff, with like helper decorators, and there's even some use of eval. (I swear it's the right thing in this particular weird case but... not so helpful here.)

It wouldn't be too bad to switch to keeping an explicit list of items that get imported into trio, trio.hazmat, and trio.testing from trio._core. The main reason we currently do it the way we do now is to make sure we don't accidentally provide some API in trio._core that we don't also provide to end-users somewhere ("keeping us honest"). But we could enforce that with a test in the test suite instead.

The big question is how we can express that list in a way that PyCharm understands, given that the things we're importing are not ultimately visible to it.

If you make an empty module with __all__ = ["asdf"] and then don't define asdf, does PyCharm offer asdf as a completion for that module?

What about a module with from ._nonexistent import asdf, where _nonexistent.py is, well, nonexistent. Does PyCharm offer asdf as a completion for that module?

@parity3

This comment has been minimized.

parity3 commented Sep 5, 2017

Defining __all__ without then defining asdf caused asdf to show in the autocomplete dropdown (but PyCharm doesn't know the type of asdf, which is expected).

The from ._nonexistent import asdf example caused asdf to show in the autocomplete dropdown as well, although the module with that nonexistent import line obviously has problems.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 5, 2017

Oh good. Okay, one more check, if you can... what about this weirdo module:

__all__ = []
_imports = ["asdf"]
__all__ += _imports
@parity3

This comment has been minimized.

parity3 commented Sep 5, 2017

asdf did NOT show up in autocomplete when using += or extend on __all__. It didn't even work if I assigned without a literal list: __all__ = _imports. It DID show up when calling __all__.append("asdf") however.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 5, 2017

Darn. Oh well, I think I can work around that.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 5, 2017

(And thank you very much for helping with my weird questions :-))

njsmith added a commit to njsmith/trio that referenced this issue Sep 5, 2017

Make our exports more visible to static analysis
In particular, this should make it so PyCharm's completion starts
working.

Fixes python-triogh-314.
@njsmith

This comment has been minimized.

@dhirschfeld

This comment has been minimized.

dhirschfeld commented Sep 5, 2017

It may be that something can be done on the PyCharm side. I've opened issues against PyCharm before and they've been somewhat responsive:

https://youtrack.jetbrains.com/issues/PY

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 5, 2017

Before subjecting the support staff to arcane questions about the details of Python static analysis, let's see if @vlasovskikh has a moment to help us out...

(The question is: the trio package has lots of exports that are generated dynamically, so PyCharm's completion falls over and dies when trying to write code using trio. In small cases, explicitly listing these in __all__ seems to be enough, but then when I tried doing that for real in #316 it apparently didn't work. Any idea what's going on here, and any suggestions on how to make this work?)

@dhirschfeld

This comment has been minimized.

dhirschfeld commented Sep 5, 2017

I'm not sure but I think explicitly listing them in __all__ should be sufficient and should be something which PyCharm supports hence the suggesting to open an issue if it isn't working.

PyCharm are always improving and fixing bugs in their inspections so it may be the case that it is actually intended to work.

See the latest bugs fixed at https://confluence.jetbrains.com/display/PYH/PyCharm+172.3968.34+Release+Notes

njsmith added a commit to njsmith/trio that referenced this issue Sep 6, 2017

Make our exports more visible to static analysis
In particular, this should make it so PyCharm's completion starts
working.

Fixes python-triogh-314.
@vlasovskikh

This comment has been minimized.

vlasovskikh commented Sep 6, 2017

The comment in cc7ff17 by @njsmith on how PyCharm infers the set of exported names is more or less accurate. We statically evaluate regular imports, including from ... import *, simple operations with __all__ like __add__, append, extend, but we don't evaluate loops or comprehensions.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 6, 2017

@vlasovskikh Thanks for checking! Do you have any idea then why @parity3 reports (confirmation and more description of behavior) that PyCharm does not detect the literal strings in this __all__ definition as being exported? Is this a bug that we should file with PyCharm?

(Repro: pip install https://github.com/njsmith/trio/archive/issue-314.zip and then import trio and check what attributes are offered as completions for trio..)

@vlasovskikh

This comment has been minimized.

vlasovskikh commented Sep 7, 2017

PyCharm highlights the strings in __all__ as unresolved because these names are defined via updating globals() a few lines below and PyCharm fails to evaluate it statically.

Mentioning names like run in __all__ doesn't help PyCharm to see the name because __all__ is redefined later via += ... .__all__ and we don't evaluate the imported __all__ definitions. Instead we just ignore __all__ and assume that all the names defined in the module are exported. But since run is not defined in this module from a static analyzer point of view (due to the use of the dynamic globals() update), we highlight trio.run as unresolved.

By the way, this is not just the limitation of PyCharm. Mypy also doesn't see run there:

mypy a.py --check-untyped-defs
a.py:3: error: Module has no attribute "run"

The easiest way to provide static information about your dynamic imports is to actually import all the stuff from .core inside a false check, including hazmat you don't want to import:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from ._core import *

or even:

from typing import TYPE_CHECKING

if False:
    from ._core import *

although some analyzers would actually evaluate False to false and ignore this import.

@njsmith

This comment has been minimized.

Member

njsmith commented Sep 9, 2017

@vlasovskikh Ahhh ok, thanks, that definitely helps. (Though it would be nice if PyCharm were clever enough to realize that += doesn't remove names from __all__ :-).)

In this case _core is even less amenable to static analysis, and I'd rather not have people getting prompted with a bunch of spurious trio.hazmat.* names when completing on trio.* and vice-versa, so probably the practical workaround is the one suggested here, of defining a submodule with a totally-static __all__ and then doing from ._reexport_core import *.

njsmith added a commit to njsmith/trio that referenced this issue Sep 9, 2017

Move toplevel _core exports into a dedicated module
Apparently PyCharm gives up on parsing __all__ entirely if it sees
__all__ += foo.__all__, so move the static trio.__all__ entries that
we want to be visible to static analyzers into their own dedicated
submodule.

See: python-trio#314 (comment)

@njsmith njsmith closed this in #316 Sep 9, 2017

@njsmith njsmith referenced this issue Aug 8, 2018

Merged

Simplify imports #594

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment