Fix for issue #524

[Nickatak]

Issue #524 summary:

Running any script that imports typing_extensions with the -OO flag will raise an AttributeError.

Expected behavior:

Files should be able to import typing_extensions even if they're being ran with the -OO flag.

Actual behavior:

Raises the attribute error seen below because Protocol.__doc__ is a NoneType when the -OO flag is used, and therefore has no attribute .format.

Traceback (most recent call last):
  File "testing.py", line 1, in <module>
    import typing_extensions
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 656, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 626, in _load_backward_compatible
  File "C:\Users\Nickatak\desktop\github\venv_typing\lib\site-packages\typing_extensions-3.6.2.1-py3.6.egg\typing_extens
ions.py", line 986, in <module>
AttributeError: 'NoneType' object has no attribute 'format'

Steps to recreate the error:

• Initial setup (Create a venv/clone the repo)
• Install typing (python setup.py install for /typing/setup.py)
• Install typing_extensions (python setup.py install for /typing/typing_extensions/setup.py)
• Create a Python file and try to import typing_extensions in it OR
• Start the live interpreter with -OO (python -OO) and then try to import typing_extensions.

Alternative steps to recreate the error:

• Run /typing_extensions/src_py3/test_typing_extensions.py with the OO flag (python -OO test_typing_extensions.py)

The fix:

During running the tests, we can see that the problem originates from line 985 in /typing/typing_extensions/typing_extensions.py:

Protocol.__doc__ = Protocol.__doc__.format(bases="Protocol, Generic[T]" if
                                               OLD_GENERICS else "Protocol[T]")

Placing a simple if-check to see whether Protocol.doc is not None around this piece of code avoids trying to call .format on a Nonetype.

Testing instructions:

In order to confirm whether the issue has been resolved, we can take the following steps.

• Initial setup (Create a venv/clone the repo).
• Install typing (python setup.py install for /typing/setup.py).
• Install typing_extensions (python setup.py install for /typing/typing_extensions/setup.py).
• Create a Python file and try to import typing_extensions in it OR
• Start the live interpreter with -OO (python -OO) and then try to import typing_extensions.

AND

• Run /typing_extensions/src_py3/test_typing_extensions.py with the OO flag (python -OO test_typing_extensions.py)
• Run test_typing_extensions.py normally (python test_typing_extensions.py)
• Run tox tests.

Output from testing (just the test_typing_extensions.py):

..........................................................................
----------------------------------------------------------------------
Ran 74 tests in 0.033s

OK

Request:

• Be brutal, I'm new to github. Anything you want me to fix/edit/add/etc., I will be happy to do. I'm learning :D
• If I need to find a better way to test it, let me know (I will try my best to figure it out).
• Please be a little patient when it comes to raw github commands (branching/squashing/etc.).

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

[Nickatak]

Just a heads up: I signed that (right now-prior to submitting the PR). If there's an issue with it, I am more than willing to sign it again.

[emmatyping]

Hm, it seems you staged and committed all of the files you modified. Did you perhaps run git add .? I think you probably wanted to git add typing_extensions/src_py3/typing_extensions.py. To fix that you can run git reset HEAD~ (undo the last commit). Then you can git add like I suggest above. Then git commit -c ORIG_HEAD (reuse the replaced commit message). Then git push -f origin master (this will re-write history to be just the changes to typing_extensions.py).

Also we probably want a test for this. This is somewhat open ended. One idea is to use compile with the correct optimization level, and check that the typing source compiles under -OO. Or perhaps you could pass a flag to pytest (which is the underlying test framework typing uses) to run it under -OO.

[Nickatak]

Hmm, thank you for your feedback (I totally did run git add ., guilty as charged) :D Unfortunately, I have to go to sleep now :C
I will resume things when I have free time tomorrow and attempt to follow your instructions (thank you for t he suggestions on the testing by the way, that was really disturbing me [Not to mention the git instructions]).

[ilevkivskyi]

You should also add four spaces of indent here.

[Nickatak]

Thank you! C: should I commit a second time (or just re-write the initial commit)?

[Nickatak]

I did an overhaul of the PR markdown. I realized that while I kind-of understood the issue, I couldn't even reproduce it consistently [didn't understand it other than syntactically], let alone test it. After trying many many different things (and spending a lot of time trying to test the wrong module), I finally was able to reproduce the error consistently AND provide a solid way of confirming the issue is resolved. As for the review: please let me know if you'd like me to make another commit to fix that or if you would rather I reset the head->applied the review-fix -> re-committed. Thank you very much for your patience! C:

[elliott-beach]

As for the review: please let me know if you'd like me to make another commit to fix that or if you would rather I reset the head->applied the review-fix -> re-committed. Thank you very much for your patience! C:

According to python/mypy#4161, I guess the policy is to not rewrite commits.

[ilevkivskyi]

@Nickatak

As for the review: please let me know if you'd like me to make another commit to fix that or if you would rather I reset the head->applied the review-fix -> re-committed. Thank you very much for your patience! C:

You can just commit on your branch and push. As I can see from the current state of PR you did everything right.

The only thing missing now is a test. See @ethans post above

Also we probably want a test for this. This is somewhat open ended. One idea is to use compile with the correct optimization level, and check that the typing source compiles under -OO. Or perhaps you could pass a flag to pytest (which is the underlying test framework typing uses) to run it under -OO.

[Nickatak]

Indeed. Initially, I misinterpreted that statement as "We need a way to make sure this works manually." While the former is important, I realize now that what he meant was we need a written test-case (and I think using compile is probably the easiest way to go here). I have a few questions and will try to list them out as clearly/separately as possible:

• Wouldn't the test go inside test_typing_extensions.py (not test_typing.py), since the issue itself originated from within typing_extensions.py? Or do both need tests? (I think it would be strange to add a test for typing, when typing_extensions isn't imported anywhere in typing, or maybe I'm misunderstanding something?)
• In any case of the above question ^: Is there a particular section that you would recommend placing the test in (I was thinking under ProtocolTests for either, named something like test_compiles_with_optimization)?
• Looking at all the previous tests written for both files (test_typing_extensions.py and test_typing.py), don't both use the unittest module (instead of pytest) for py3?
• Clearly, I have attempted to fix something way outside the range of doing so swiftly/independently with my current ability (and learned/am learning a lot along the way). Is it preferable in the future that I post these types of questions inside the Issue itself instead of opening a PR?

[emmatyping]

Wouldn't the test go inside test_typing_extensions.py (not test_typing.py), since the issue itself originated from within typing_extensions.py? Or do both need tests? (I think it would be strange to add a test for typing, when typing_extensions isn't imported anywhere in typing, or maybe I'm misunderstanding something?)

There should be a test for both typing.py and typing_extensions.py (one in each test_* file). The reasoning is that while the issue here was in typing_extensions.py, a) many things move from typing_extension.py into typing.py and b) we don't want regressions on this behavior in either file.

In any case of the above question ^: Is there a particular section that you would recommend placing the test in (I was thinking under ProtocolTests for either, named something like test_compiles_with_optimization)?

This seems fine to me.

Looking at all the previous tests written for both files (test_typing_extensions.py and test_typing.py), don't both use the unittest module (instead of pytest) for py3?

pytest is used as the test runner, but automatically picks up unittest test cases.

Clearly, I have attempted to fix something way outside the range of doing so swiftly/independently with my current ability (and learned/am learning a lot along the way). Is it preferable in the future that I post these types of questions inside the Issue itself instead of opening a PR?

Ideally you should research these topics independently, the internet is a great resource! However, you can also ask on the typing Gitter chat channel for issues more closely pertaining to project specific things. I should usually be around to answer questions, and if I am not, someone else usually is.

[Nickatak]

So, I'm having a little bit of trouble isolating the issue with compile. Since compile itself needs some kind of string for a source, I only know of one option: read in the entire file (I tried using inspect, but since the source for that is tabbed, it just raises SyntaxErrors). But, doing so doesn't really specify that the issue is with the Protocol class (as when I exec() to test, the caught AttributeError could be anywhere within the file). While the below does produce the failed test when using the old file, and does not produce a failed test when using the newly committed changes (using optimize=2), I feel like this is a) hacky as heck, and b) very brittle. Interestingly, even with the old file, it is able to compile it with optimize=2, the issue displays itself when you exec() it.

Questions:

• I'm not so sure I would read in the file so statically like that. Is this even acceptable?
• The overall scope of the test below is so wide, I don't think that's acceptable either, is it?
• Is it acceptable to import a module just for one test? (EG: py_compile could read the file in for me, although it still leads me around to exec'ing anyway)
• Maybe this would be a better test under AllTests?
• ^ If that's the case should I use inspect or maybe py_compile instead of manually reading in the entire file?
• I couldn't really find a clean way to "assertDoesNotRaise". Do you know of a better/cleaner way? I also was thinking about just leaving it as exec(test_module, {}), as this would give a traceback (but doing so only makes sense if it's an AllTests test really).
• Maybe I'm approaching this the wrong way, but the idea was to: load the file, compile it with the -OO flag (optimize=2), and try to run it (exec). Am I completely misguided in my thinking?

Just a heads up, this looks terrible, sorry about that:

        def test_compiles_with_opt(self):
            with open('typing_extensions.py', 'r') as fo:
                test_code = fo.read()

            test_module = compile(test_code, 'typing_extensions', 'exec', optimize=2)
            try:
                exec(test_module, {})
            except AttributeError:
                self.fail('Module did not compile correctly.')

Edit: I'm going to commit, so we can review it, but I personally don't like this as a test at all (I'm only going to commit the typing_extensions test first. If we can find a better solution/agree on the commit, then I'll create another test for typing as well. Also, I'm bad, and forgot to run git diffs before I pushed (sorry about the multi-commit).

Edit2: Ah, I see some errors have occurred with my test (it really didn't like that). Hmm...(rewriting test).

Edit3: I think using inspect is much cleaner here. If not, I can always read it directly from typing_extensions/src_py3/typing_extensions.py, but that approach makes this so brittle (It "works" by the CI bot's standards [Also tested with pytest]). I think this suits better as a general AllTest though, than a Protocol-specific test.

[ilevkivskyi]

Just checking that file compiles with optimizations seems OK to me. Also I would add the same tests on Python 2 (although it doesn't fail now, it would be useful to prevent this in future). There are two folders: python2 and typing_extensions/src_py2.

[Nickatak]

I'm having some issues porting the tests over to py2 directly (the compile() for py2 doesn't support an optimize flag like it does in py3). I was also looking at the pytest option to see if I could pass an optimization flag for py2, and I couldn't find anything about it. I think I may be able to come up with a (really hackish) way to check the py2 sources using os.system/subprocess.call. I'm doing some testing with them right now, to see if I can get something to work (I am able to reproduce the error [tried to do something to a nonetype docstring due to OO] if something like the original issue happens in the other sources, and it does indeed break correctly when this is used: python -OO -m pytest typing_extensions/src_py2).

WIP-1: I'm pretty sure I got something to work. It is SO brittle though (I went with my initial idea and ended up using subprocess.check_output). Oh no, I broke it. :C

WIP-2: YAY-Still broken. Now the tox tests don't work at all, but all the py.test and pytests do: OH, it's because I used os.getcwd() [which would be different using tox, because tox's originating cwd must be elsewhere than the top level directory]? No, I don't think so, if I remove the two typing tests, the typing_extensions ones work fine, which means there's an issue with what I'm doing inside test_typing for both py2/py3. Hmm...I'm going to sleep for now, I'll be on tomorrow working on this all day :D

WIP-3: Hmm, the test_typing with both py2 and py3 have some serious problems when testing them with tox. I'm not exactly sure why this is. There seems to be something calling super() which is making a new thing, which is calling super() which is...so on and so forth.

WIP-4: Oh man I'm bad. Something seems to be failing with unittest.discover(). Alright, that's real interesting. I guess re-loading the module like that is causing isinstance some serious problems. I think I may have an idea to get this to work, but it'll be very brittle (again). :C Oh, tox isn't run on typing_extensions. Hmm...Still. I think I have found a way to make it work (testing/duplicating errors tonight, will commit tomorrow).

Tests:

• Py2 test_typing_extensions (pytest, and py.test [tox doesn't run on these]).
• py3 test_typing_extensions (pytest, and py.test [tox doesn't run on these]).
• Py2 test_typing (pytest/py.test working; tox==In testing).
• Py3 test_typing (pytest/py.test working; tox==In testing).

[Nickatak]

I found the issue with my old tests and wrote new tests for it! :D

They're kind of (extremely) brittle, but they do test the issue successfully. I'll try to explain some of the decisions I made regarding the test.

My reason for choosing to use subprocess over compile:

compile() itself doesn't throw errors (unless it's syntax/import/etc.). The only way to detect whether there are runtime errors using this method is to attempt to exec() it after compiling. The problem (or atleast what I understand) with this is that when you exec(), it forces the module to be 'reloaded' in memory and isinstance() will break, causing an infinite loop when other tests are loaded with unittest.discover() with tox. Instead, I've chosen to spawn an entirely different (separate) process passing the -OO flag as a command-line argument. While this makes the test EXTREMELY brittle, it does work.

tox doesn't run any tests found in typing_extensions (but just incase, I tox'd them with the new tests anyway). I decided to use a very similar testing method that I tried to use for typing_extensions_py2 because it doesn't reload the module (very important). I have made a handy checklist for testing my tests (seriously, sorry) and will post it below.

Tests of tests:

• typing_extensions_py3:
  • With the old error present (without the if Protocol.__doc__ is not None: statement).
    • py.test typing_extensions/src_py3 fails.
    • pytest typing_extensions/src_py3 fails.
    • tox passes (tox tests do not load typing_extensions).
  • Without the old error present (with the if Protocol.__doc__ is not None: statement).
    • py.test typing_extensions/src_py3 passes.
    • pytest typing_extensions/src_py3 passes.
    • tox passes (tox tests do not load typing_extensions).
• typing_extensions_py2:
  • With a duplicated error present (Insert __doc__ += 's' on line 345 in typing_extensions.py to force error).
    • py.test typing_extensions/src_py2 fails.
    • pytest typing_extensions/src_py2 fails.
    • tox passes (tox tests do not load typing_extensions).
  • With the original file.
    • py.test typing_extensions/src_py2 passes.
    • pytest typing_extensions/src_py2 passes.
    • tox passes (tox tests do not load typing_extensions).
• typing_py3 (/src/):
  • With a duplicated error present (Insert __doc__ += 's' on line 1697 in typing.py to force error).
    • py.test src fails.
    • pytest src fails.
    • tox fails.
  • With the original file.
    • py.test src passes.
    • pytest src passes.
    • tox passes.
• typing_py2 (/python2/):
  • With a duplicated error present (Insert __doc__ += 's' on line 1602 in typing.py to force error).
    • py.test python2 fails.
    • pytest python2 fails.
    • tox fails.
  • With the original file.
    • py.test python2 passes.
    • pytest python2 passes.
    • tox passes.

[ilevkivskyi]

Two more minor comments.

[ilevkivskyi]

inspect is not used anymore.

[Nickatak]

Removed inspect (latest commit).

[ilevkivskyi]

You are not using e, below, so ... as e is not needed.

[Nickatak]

Hm, do you think the specific catch is still necessary? I ended up removing it on my latest commit as a whole and can put it back if you think it is. :D

[ilevkivskyi]

I think we can keep the current version of the tests (unless @ethanhs has another ideas). I can merge this after you fix the remaining comments.

[emmatyping]

This testing method is acceptable to me.

[ilevkivskyi]

Bare except is bad style. It can catch something unrelated, I was talking about only removing as e part, since it was not used.

[Nickatak]

Indeed. I will add it back immediately.

[ilevkivskyi]

Thanks! LGTM.