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

Make our exports more visible to static analysis #316

Merged
merged 6 commits into from
Sep 9, 2017

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Sep 5, 2017

In particular, this should make it so PyCharm's completion starts
working.

Fixes gh-314.

@njsmith
Copy link
Member Author

njsmith commented Sep 5, 2017

@parity3 Can you check if this fixes your problem?

@codecov
Copy link

codecov bot commented Sep 5, 2017

Codecov Report

Merging #316 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
- Coverage   99.24%   99.24%   -0.01%     
==========================================
  Files          84       86       +2     
  Lines       10340    10323      -17     
  Branches      725      726       +1     
==========================================
- Hits        10262    10245      -17     
  Misses         61       61              
  Partials       17       17
Impacted Files Coverage Δ
trio/_core/__init__.py 100% <ø> (ø) ⬆️
trio/_core/_ki.py 100% <ø> (ø) ⬆️
trio/_core/_result.py 100% <ø> (ø) ⬆️
trio/_core/_traps.py 100% <ø> (ø) ⬆️
trio/_core/_unbounded_queue.py 100% <ø> (ø) ⬆️
trio/_core/_parking_lot.py 100% <ø> (ø) ⬆️
trio/_deprecate.py 100% <100%> (ø) ⬆️
trio/_core/_io_kqueue.py 78.2% <100%> (-1.32%) ⬇️
trio/hazmat.py 100% <100%> (ø) ⬆️
trio/_core/_io_epoll.py 100% <100%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 733be75...ffdb562. Read the comment docs.

@python-trio python-trio deleted a comment from codecov bot Sep 5, 2017
@njsmith
Copy link
Member Author

njsmith commented Sep 5, 2017

For testing, it should be possible to do: pip install https://github.com/njsmith/trio/archive/issue-314.zip (do this in a venv of course!)

@parity3
Copy link

parity3 commented Sep 5, 2017

I checked out your commit. PyCharm still has an issue with resolving trio.run.
I modified trio/__init__.py and added this right before the __all__ definition, and it works:

TrioInternalError = _core.TrioInternalError
RunFinishedError = _core.RunFinishedError
WouldBlock = _core.WouldBlock
Cancelled = _core.Cancelled
ResourceBusyError = _core.ResourceBusyError
MultiError = _core.MultiError
format_exception = _core.format_exception
run = _core.run
open_nursery = _core.open_nursery
open_cancel_scope = _core.open_cancel_scope
current_effective_deadline = _core.current_effective_deadline
STATUS_IGNORED = _core.STATUS_IGNORED
current_time = getattr(_core, 'current_time')
current_instruments = getattr(_core, 'current_instruments')
current_clock = getattr(_core, 'current_clock')
remove_instrument = getattr(_core, 'remove_instrument')
add_instrument = getattr(_core, 'add_instrument')
current_statistics = getattr(_core, 'current_statistics')
TaskLocal = _core.TaskLocal

__all__ = [
    "TrioInternalError", "RunFinishedError", "WouldBlock", "Cancelled",
    "ResourceBusyError", "MultiError", "format_exception", "run",
    "open_nursery", "open_cancel_scope", "current_effective_deadline",
    "STATUS_IGNORED", "current_time", "current_instruments", "current_clock",
    "remove_instrument", "add_instrument", "current_statistics", "TaskLocal"
]

Notice the getattr calls (ie it's ugly). PyCharm won't add things to the trio root module that it thinks do not exist unfortunately.

Hope this helps.

@njsmith
Copy link
Member Author

njsmith commented Sep 5, 2017

Ugh. I really don't want to do that. Any idea why this seems to contradict what you observed here?

@parity3
Copy link

parity3 commented Sep 5, 2017

It does seem to contradict the simple experiments I did above; the __all__ name strings were all a fiercely-colored red and did not resolve. No idea why one worked and not the other; perhaps something to do with the other activity going on in __init__.py.

Maybe just auto-generate a .pyi file and check it into typeshed? I hear pycharm and others use that for stub generation for similar hard-to-introspect packages to trio.

@njsmith
Copy link
Member Author

njsmith commented Sep 5, 2017

Typeshed is for declaring the types of variables. I don't actually know if you can use it to declare the attributes of a module?

@parity3
Copy link

parity3 commented Sep 5, 2017

Oh well, I don't have any more knowledge here (regarding typeshed). I have yet another question that may provide insight.. how is numpy using __all__.extend without issue? Or is PyCharm just not resolving those as well?

@njsmith
Copy link
Member Author

njsmith commented Sep 5, 2017

@parity3 Those numpy imports are similar to the parts of trio that pycharm does understand (ref). I'm guessing that it's actually the from .core import * that it detects, rather than the __all__.extend(core.__all__). Or maybe it's not resolving them, I dunno; you're the one with PyCharm installed ;-)

@parity3
Copy link

parity3 commented Sep 5, 2017

Maybe numpy's less notable names in _mat.__all__ would be unresolved in PyCharm, who knows. I'm out of time researching this right now unfortunately. I really hope someone can swoop in and save the day here. At the moment I see some bad options like:

  • have init.py reference all public names with knowledge of where they lie in their sub-packages (not DRY)
  • move out all the imports that should not be exposed, directly into hazmat and just use simple import *'s to dictate what's public (remove the _public / _hazmat fn attributes that currently dictate exposure)
  • move the hazmat methods into a sub-sub-module underneath trio._core.hazmat.... (ugh)

All of those are frightening workarounds; why does this have to be so challenging!?

@parity3
Copy link

parity3 commented Sep 6, 2017

Here is a SLIGHTLY better workaround for trio/__init__.py PyCharm friendliness:

from . import _core
from ._core import *
to_keep = [
    "TrioInternalError", "RunFinishedError", "WouldBlock", "Cancelled",
    "ResourceBusyError", "MultiError", "format_exception", "run",
    "open_nursery", "open_cancel_scope", "current_effective_deadline",
    "STATUS_IGNORED", "current_time", "current_instruments", "current_clock",
    "remove_instrument", "add_instrument", "current_statistics", "TaskLocal"
]
any(globals().__delitem__(_) for _ in _core.__dict__ if _ not in to_keep)
del _core
del to_keep

... And I think we can remove all references/modifications to __all__ in the file as well?

Also, note that hazmat.py, defining its __all__ had no red in PyCharm. So it may be specific to an __init__.py package module, and less strict for non-package modules.

@njsmith
Copy link
Member Author

njsmith commented Sep 6, 2017

@parity3 (please don't keep poking at this if you have other things you need to be doing, it is not urgent :-))

If you do that, then does pycharm actually show the correct suggestions for trio.*, or does it also show all the hazmat stuff that gets deleted (but pycharm doesn't know that)?

So it may be specific to an __init__.py package module, and less strict for non-package modules.

If that's true then I guess a potential workaround would be to move the trickiness into trio/_reexport.py and then have __init__.py do from ._rexport import *...

@parity3
Copy link

parity3 commented Sep 6, 2017

You're right, PyCharm gives no error when resolving trio.checkpoint_if_cancelled when it should (runtime error happens).

Ya I really need to stop now ;)

This isn't really needed currently (we'd know, the symptom is a
RecursionError at import time), but it's a genuine bug fix for a
problem I ran into while implementing python-triogh-316. And since that PR seems
to be stalled at the moment, I want to get this in so it doesn't get
forgotten.
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
Copy link
Member Author

njsmith commented Sep 9, 2017

@parity3 Just pushed a new commit based on the discussion in #314 -- I think it should work for you, but can you confirm?

(Or same question for anyone else with PyCharm)

@parity3
Copy link

parity3 commented Sep 9, 2017

I can confirm that trio.run and many more names are able to be resolved by PyCharm.

However, trio.hazmat.checkpoint_if_cancelled still did not resolve. I went into hazmat.py and sure enough, the __all__ definition had a bunch of red even though they are simple constants. I narrowed it down to the presence of __all__.remove().

I outsmarted PyCharm by replacing the code in hazmat.py starting at line 41 (for _sym ...):

# Some hazmat symbols are platform specific
import itertools
contained_in_core = set(filter(_core.__all__.__contains__, __all__))
globals().update(zip(contained_in_core, map(getattr, itertools.repeat(_core,len(contained_in_code)), contained_in_core)))
any(map(__all__.remove, set(__all__).difference(contained_in_core)))

This achieves the same thing but PyCharm somehow does not parse through the functional programming tricks. It also worked with an eval() approach but that's even more clunky.

@njsmith
Copy link
Member Author

njsmith commented Sep 9, 2017

@parity3: ugh, of course. Hmm.

@parity3
Copy link

parity3 commented Sep 9, 2017

Actually, even simpler.. above the for-loop, just assign a variable like so: remove = __all__.remove
and switch __all__.remove(_sym) with remove(_sym) and everything works fine as-is.

@parity3
Copy link

parity3 commented Sep 9, 2017

For anyone else who wants to get involved here, the test code I'm using to verify proper resolution in PyCharm is:

import trio, sys

# make sure ".run" isn't a bad color
run = trio.run

# make sure ".checkpoint_if_cancelled" isn't a bad color
checkpoint_if_cancelled = trio.hazmat.checkpoint_if_cancelled
ON_POSIX = 'posix' in sys.builtin_module_names
if ON_POSIX: # haven't tested this but should work
    assert 'wait_kevent' in trio.hazmat.__all__ and 'current_iocp' not in trio.hazmat.__all__
else: # yes, tested this
    assert 'wait_kevent' not in trio.hazmat.__all__ and 'current_iocp' in trio.hazmat.__all__

@njsmith
Copy link
Member Author

njsmith commented Sep 9, 2017

remove = __all__.remove

ughhhh this solution is so terrible and so beautiful in different ways.

I guess if that's what we have to do then that's what we have to do. I'm really curious if this fiddling helps with other IDEs. And wish we had a better way to test this. It's seems very likely that PyCharm will someday get cleverer about these, but will it be because it becomes clever enough not to need these tricks, or clever enough to see through our tricks and break everything again? It is a mystery 👻

I wish we could write a test for this, but I suppose that's asking too much. Oh well.

@parity3
Copy link

parity3 commented Sep 9, 2017

Yes, confirmed the stupid hack is working now. As for the future, unknown as always.

@njsmith
Copy link
Member Author

njsmith commented Sep 9, 2017

Okay, let's cross our fingers and see how it goes...

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.

2 participants