Migrate hazmat to faked explicit importing and reexporting, to aid static analysis (issue #542)

[jmfrank63]

Continuing towards finalisation of #542
We do a try / except import of our symbols to aid static analysis tools to pick them up. Then we update the list dynamically to have python get the correct symbols.

[codecov]

Codecov Report

Merging #753 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #753      +/-   ##
==========================================
+ Coverage   99.01%   99.01%   +<.01%     
==========================================
  Files          96       96              
  Lines       11677    11682       +5     
  Branches      830      828       -2     
==========================================
+ Hits        11562    11567       +5     
  Misses         94       94              
  Partials       21       21

Impacted Files	Coverage Δ
trio/tests/test_exports.py	96.29% <ø> (-0.07%)	⬇️
trio/hazmat.py	100% <100%> (ø)	⬆️

---

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 9799564...0585e14. Read the comment docs.

[jmfrank63]

It seems the try / except worked with socket because it is a C-module. Hazmat on the other hand is pure python so failing imports are respected.

[jmfrank63]

Before I continue on hazmat, I'd thought we'll discuss the _public decorator. @njsmith Is this a concept that has to stay or could we replace it with something better understandable?
I had a discussion with one of the bonobo library maintainers and he suffered from a similar problem. Maybe we should think about a library that solves this problem.
EDIT: The failure was a simple error in the tests. However some symbols still seem to be missing on code completion. Investigation continues...

[njsmith]

It seems the try / except worked with socket because it is a C-module. Hazmat on the other hand is pure python so failing imports are respected.

I'm not sure what you mean here but it sounds like you might be confused about something? I don't know why socket being a C module would affect anything, and in any case, socket isn't a C module (though it imports a bunch of stuff from the C module _socket).

[njsmith]

@_public is part of the construction of the trio._core namespace, so trio.hazmat only uses it indirectly. Let's split that discussion off into another issue, and focus here on just the stuff in trio/hazmat.py.

I think the simplest way to handle this is a few import statements:

from ._core import (
    # all the symbols that are available universally
)

try:
    from ._core import (
        # all the Windows symbols
    )
except ImportError:
    pass

try:
    from ._core import (
        # all the epoll symbols
    )
except ImportError:
    pass

try:
    from ._core import (
        # all the kqueue symbols
    )
except ImportError:
    pass

(Actually, I guess there might not be any epoll-specific symbols, so we might be able to leave that section out?)

Anyway, the point of doing it this way is that it's pretty simple, and I think should let us stop touching globals() entirely inside this file?

[njsmith]

It might make sense to merge #756 before this, and then use #756's new tests instead of adding tests here.

[jmfrank63]

It seems the try / except worked with socket because it is a C-module. Hazmat on the other hand is pure python so failing imports are respected.

I'm not sure what you mean here but it sounds like you might be confused about something? I don't know why socket being a C module would affect anything, and in any case, socket isn't a C module (though it imports a bunch of stuff from the C module _socket).

Yes, I had an error in the tests and it took me a while to find out this was the reason. As you see it already works but some symbols are still missing.
As for the @_public construction I am going to test to define the functions statically on the top of the module with the doc string and just a raise from. They should be then overwritten by your wrapper. Additionally we could then strip the try except block from the wrapper template as the function will always be there. Did I miss any case where this does not work?
EDIT:
Static definition works but removing the try / except block does not seem to work.
@njsmith @Zac-HD
But we could use the static definition to get hold of the context in each function and then call the instance method. I didn't want to touch the current functionality without your opinion on this.

Or we could move the code into the functions and have the class using the functions.

[njsmith]

Please do not modify any file inside the trio/_core/ directory in this PR. It always goes smoother when you do one thing at a time :-).

[jmfrank63]

Please do not modify any file inside the trio/_core/ directory in this PR. It always goes smoother when you do one thing at a time :-).

Yes, right, I was just looking a little a head a little bit. Got a full code completion this way and wanted to see if this works. There is always git reset

[pquentin]

(Merged #756, which causes a conflict in trio/tests/test_exports.py as expected, sorry!)

[jmfrank63]

(Merged #756, which causes a conflict in trio/tests/test_exports.py as expected, sorry!)

No worries, Nathaniel already warned me of that. I'll merge locally, that should do it.

[jmfrank63]

@njsmith The comment to

    @abstractmethod
    def clone(self):
    r"""
    ...

in _abc.py
should be a raw string because it contains a \ (getting a deprecation warning again). I think it was PEP 257 that stated that.

Sorry for nitpicking 😄

[jmfrank63]

Productive weekend. Some symbols from kqueue, epoll and the magic @_public are still missing, they can however be defined statically without changing the code behaviour. Let me know if I should do this.

[njsmith]

This is a little different from the socket re-exports. There, we have the problem that the socket namespace varies in ways that we don't really understand and can't control, so every time we're imported we have to be ready to adapt to whatever socket happens to be doing on this particular Python. But here, we're only importing things from Trio itself, and we know for sure which different configurations are possible.

So, I think the way I'd do this is:

1. Split up the try/import/except ImportError into multiple pieces, one for each possible configuration. Like:

# Unix-specific symbols
try:
    from ._core import (
        wait_writable,
        wait_readable,
        notify_fd_close,
    )
except ImportError:
    pass

# Kqueue-specific symbols
try:
    from ._core import (
        current_kqueue,
        monitor_kevent,
        wait_kevent,
    )
except ImportError:
    pass

# Windows symbols
# ...

2. Since these import statements will actually work at runtime, we can completely delete the whole globals().update(...) block!

3. wait_socket_readable, wait_socket_writable, and notify_socket_close are always available, so they should move up into the very first from ._core import ..., along with things like cancel_shielded_checkpoint and Task.

[jmfrank63]

Hi @njsmith I'll check on this over the next day, but as far as I remember that did not work. The @_public decorator actually creates a static entry point for the methods but static code analysis does not get this.
So if we just do a

def static_entry_point:
    ''' docstring '''

that will do the job, as during import time they all get overwritten again. But static analysis does not get that the functions are getting overwritten and will just display the stubs.

[njsmith]

@_public is a separate issue we'll deal with later :-). My suggestions in the message above should work identically to what's in this PR right now, while being simpler.

[jmfrank63]

@njsmith It's currently not obvious from the track if all tasks have been finished. I had some really busy weeks. A little refresh could help :-)

[njsmith]

@jmfrank63 I think this comment is the thing to look at, still: #753 (comment)

[njsmith]

You also need to enable the hazmat test in test_exports.

[jmfrank63]

You also need to enable the hazmat test in test_exports.

Haha, yes, just found this out and saw your comment after the refresh.

[jmfrank63]

Hi @njsmith Tried to split the try except block which worked but I had to keep the dynamic globals update as without it the build failed. So there is no point in splitting only makes the code longer and less readable. I think we should leave it as it is now. Works and is readable.

[njsmith]

Tried to split the try except block which worked but I had to keep the dynamic globals update as without it the build failed

Can you show the code you tried and the test results? Even if this is true I think it's still important to understand why...

[njsmith]

It looks like this is the commit: ffdf069
And these are the test results: https://travis-ci.org/python-trio/trio/builds/467684329
And the only test failures are two annoying Travis glitches, not anything to do with the actual code?

[njsmith]

Oh, I guess the appveyor tests failed too: https://ci.appveyor.com/project/njsmith/trio/builds/20995101/job/n10octd21tdj1ris?fullLog=true

That failure is because you put wait_socket_readable, wait_socket_writable, and notify_socket_close in the "unix-only" section, but they should actually be in the "always available" section. (As noted in item 3 here.)

[jmfrank63]

That’s easy to check., I’ll try again.

…

On Fri, 14 Dec 2018 at 10:11, Nathaniel J. Smith ***@***.***> wrote: Oh, I guess the appveyor tests failed too: https://ci.appveyor.com/project/njsmith/trio/builds/20995101/job/n10octd21tdj1ris?fullLog=true That failure is because you put wait_socket_readable, wait_socket_writable, and notify_socket_close in the "unix-only" section, but they should actually be in the "always available" section. (As noted in item 3 here <#753 (comment)>.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#753 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8oHmtVbYFAZdZ21-px3MBKSF_AHL9Lks5u43lNgaJpZM4X94q6> .

[jmfrank63]

@njsmith Sorry I overlooked part two, should have looked more closely.
Do we have something like JIRA where we can assign tasks so stuff doesn't get lost?
Anyway it works and next would be trio/_core?

[njsmith]

Yep, looks good to me, thanks!

We don't have a JIRA or anything besides Github's built-in issues/PR stuff... if you want a way to keep track of what you plan to do and what you haven't done yet, then one option is to make a "checklist". This is a cute trick github supports, where you can write a regular comment with bullet points like * [ ] clean up the test, and then it renders as a check box that you can click when you're done. You can only check the boxes in a comment you posted, so to do this you'd have to go through and make your own list of what you need to do. But you might find that helpful anyway :-)

As for what to do next, I think this comment is currently the most complete checklist for what we need to do for #542: #542 (comment)

Merging this is an exciting milestone – I think jedi can now complete all top-level public symbols in Trio 🎉 ✨

Unfortunately this also means that the next steps are more complicated :-)