Issue2598

Hi,

I am a member of the NbPython team (http://nbpython.org/). We develop the python plugin for Netbeans IDE and bundle Jython in our plugin.

We rely on jython to parse import statements. We found out that the result of org.python.ast.alias.getInternalName (https://github.com/jythontools/jython/blob/master/src/org/python/antlr/ast/alias.java#L24) is wrong in some cases. If we parse statement like `from . import toto`, the method returns an empty string instead of `.`. This causes our code formating capabilities to fail. Tested with jython 2.7.0 and 2.7.1rc2.

So there is a slight difference, but I'm not able to see what Julien reports. In CPython 2.7.13, as well as CPython 3.6.1, we get:

>>> import ast; x = ast.parse("from . import toto")
>>> type(x.body[0].module)
<type 'NoneType'>
>>> x.body[0].level
1

whereas in Jython tip/2.7.1:

>>> import ast; x = ast.parse("from . import toto")
>>> type(x.body[0].module)
<type 'str'>
>>> x.body[0].module
''
>>> x.body[0].level
1

Note this is not '.' for the module/package name in either case.

I'm not certain why alias is mentioned, since there's no aliasing here via the use of the `as` keyword.

Digging a little deeper, the reason the module attribute is set to '' instead of null (in Java) is in `dottedNameListToString`. So let's try this change:

diff -r d5af3b203d59 src/org/python/antlr/PythonTree.java
--- a/src/org/python/antlr/PythonTree.java	Mon Jun 12 22:31:52 2017 -0600
+++ b/src/org/python/antlr/PythonTree.java	Wed Jun 14 10:36:25 2017 -0600
@@ -177,7 +177,7 @@
      */
     public static String dottedNameListToString(List<Name> names) {
         if (names == null) {
-            return "";
+            return null;
         }
         StringBuilder sb = new StringBuilder();
         boolean leadingDot = true;

While this diff does fix the earlier AST parsing problem so it complies with CPython's semantics, unfortunately this small change currently does not work for Jython itself, because imp.java directly builds up module names through direct appends with StringBuilder. Let's look at that specific API:

https://docs.oracle.com/javase/7/docs/api/java/lang/StringBuilder.html#append(java.lang.String)

>>> "If str is null, then the four characters "null" are appended."

Well this is a rather unfortunate coercion!* And it can be quicky seen in running the regrtest, so we get immediate exceptions in unittest itself, namely `ImportError: No module named unittest.null`.

Given how convoluted the import code is, I don't see us making this change until 2.7.2 unfortunately. But a small patch may prove me wrong; it should be safe for Jython given how much imports of various kinds are exercised through the standard library and testing specifically.

*At least the equivalent of `StringBuilder` in Python, `StringIO`, will write an empty string for `None`; and better yet, `"abc" + None` results in a `TypeError`.

Thanks for looking into this. I managed to find a workaround in NetBeans so our formating works as expected. I am not even sure that a CPython compliant output will solve our problem.

> I'm not certain why alias is mentioned, since there's no aliasing here via the use of the `as` keyword.

I may have made a mistake in my analysis.

I don't think I have the skills to tackle this bug in Jython thought. But if you come up with something and need testers, let me know.

Julien, sounds good about your workaround. In general, when we find a discrepancy between CPython and Jython, we converge on CPython, even if it's arbitrary and undocumented/untested as is the case here ("" vs None/null). The reason we do so is that the larger ecosystem of Python code that's out there often relies on these distinctions. The Java API then usually just follows for simplicity's sake.

Because this will have an impact on users like you, we definitely will want to involve you in testing so that you can update accordingly. Yes, there's a bit of cost here - accepting either "" or null - but it's unavoidable IMHO.

Although (perhaps) an easy fix as OP has work-around I propose we do not prioritise for 2.7.2. (Vote the other way by fixing it!)