Fix invalid escape sequences #12028

Open
wants to merge 1 commit into
from

Projects

None yet

3 participants

@jksuom
Member
jksuom commented Jan 9, 2017

This (hopefully) fixes #11608.

@asmeurer
Member
asmeurer commented Jan 9, 2017

Hm, I claimed on the issue that tests fail without this fix but they aren't failing at #12013. I don't even see the warnings there.

@jksuom
Member
jksuom commented Jan 10, 2017

It seems that Python will not import modules that produce errors. That is what happens with those test files that contain invalid escape sequences after the warning has been changed to an error. The test running code never sees those modules that have not been imported. However, it sees the errors but reports them as coming from the last test in the previous test file (which contains no escapes).

Another matter is that there seems to be a difference between 3.6 beta and 3.6. I had to use the former to find the invalid escapes. The final 3.6 apparently gives no error indication.

@Vgr255
Vgr255 commented Jan 10, 2017

Hey there. I worked to deprecate invalid escape sequences in Python 3.6, however I must admit there were a few issues that didn't get fixed in time (the lack of a good way to error on literals being one of them). You seem to be hitting two edge cases here:

First off, errors are only produced when the literal string is created, even if it's not used. However, literals are always created at compile time (so they can inserted in func.__code__.co_consts, namely). An annoying side-effect of this is that errors are only shown when importing the module for the first time, so if you try to import a module which has been already successfully imported (say, with warnings not being promoted to errors), then you won't notice anything. That's your first edge case.

Additionally, Python uses a two-step process for locating the compiled bytecode files: it looks in the __pycache__ folder of the current working directory if it exists, and for the file foo.py, will look for foo-XYZ.pyc, where XYZ is sys.implementation.cache_tag. If such a file exists, it will then look for the magic number (which I think is a little-endian 4-bytes integer, but don't take my word for it), and compare it with the current Python interpreter's magic number. If it matches, then all is fine and the module is imported. If it doesn't match, however, then the bytecode won't match, so Python scratches it and compiles it again.

Depending on the order that you tried to import the modules, or how you imported them, it's possible that you didn't get an error when you imported an already-imported module. I also think (not 100% sure either) that the magic number changed between the beta and final releases.

It's also possible that some related changes (http://bugs.python.org/issue28128) broke it; it's the only thing I can think of that might change behaviour between the beta and final releases about this.

Hopefully that was useful enough to understand the issues you've been facing! Don't hesitate to ask again if it didn't help. Thanks @asmeurer for letting me know of this issue on Twitter :)

@asmeurer
Member

pyc files shouldn't be a problem on Travis CI (although that explains why I was having difficulty getting the errors locally). We enable warnings in __init__.py before submodules are imported. Shouldn't that be enough to trigger the warnings for the whole module?

@asmeurer
Member

I think it just straight up doesn't work calling it from __init__.py. I can get the errors to show up with python -Wd ./bin/test in final, but only with -Wd (and they show up every time, the byte compile issue doesn't seem to be a problem). We can fix this by doing:

diff --git a/sympy/utilities/runtests.py b/sympy/utilities/runtests.py
index 2222aab..7b9365f 100644
--- a/sympy/utilities/runtests.py
+++ b/sympy/utilities/runtests.py
@@ -204,7 +204,7 @@ def run_in_subprocess_with_hash_randomization(function, function_args=(),
                      repr(function_kwargs)))

     try:
-        p = subprocess.Popen([command, "-R", "-c", commandstring])
+        p = subprocess.Popen([command, "-R", '-Wd', '-c', commandstring])
         p.communicate()
     except KeyboardInterrupt:
         p.wait()

But, that just shows the warnings, at the top before the test run. It doesn't error. At best, we can do

diff --git a/sympy/utilities/runtests.py b/sympy/utilities/runtests.py
index 2222aab..9f4e0e1 100644
--- a/sympy/utilities/runtests.py
+++ b/sympy/utilities/runtests.py
@@ -204,7 +204,7 @@ def run_in_subprocess_with_hash_randomization(function, function_args=(),
                      repr(function_kwargs)))

     try:
-        p = subprocess.Popen([command, "-R", "-c", commandstring])
+        p = subprocess.Popen([command, "-R", '-W error', '-c', commandstring])
         p.communicate()
     except KeyboardInterrupt:
         p.wait()

This will cause the test runner to completely crash when a backslash escape is encountered. That's not ideal, since generally you'd want the tests to continue running on an error (even on a syntax error it generally just moves on to the next file). Another subtlety here is that it won't be tested at all if the tests are run in --no-subprocess mode.

It really sucks that our line in sympy/__init__.py doesn't apply here. That generally makes it so that we catch and fix deprecation warnings in development, even before running the tests. @Vgr255 if you know of a better way to do this let us know. Also if there is an upstream issue we can follow for this that would be great too.

For now, let's just apply the above (second) patch.

@Vgr255
Vgr255 commented Jan 11, 2017

Yes, errors and warnings are going to show before the tests start running, because you get them when you import the modules. I'm afraid I don't have a good fix handy. There was a lot of discussion on the tracker about trying to find a way to improve the errors, but I don't think there was an open issue about that. I've been busy lately and unable to even follow tracker discussion, though, so maybe there was. Feel free to open one!

Oh, and I'm clueless as to why the line in sympy/__init__.py doesn't work.

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