The peephole optimizer can optimize indexed access to an unicode
constant which does not give the same result in UCS-2 and UCS-4 builds.
As a result, the pyc file is not portable across those builds.

This is something I witnessed when recompiling in UCS-2 rather than
UCS-4 mode, and having a strange failure in test_multibytecodec. Erasing
test_multibytecodec.pyc suppressed the failure.

Here is a small demonstration of the problem:

>>> def f():
...  return '\U00012345'[0]
... 
>>> import dis
>>> dis.dis(f)
  2           0 LOAD_CONST               3 ('\ud808') 
              3 RETURN_VALUE         


For reference, here is the error I had in test_multibytecodec:

======================================================================
FAIL: test_gb18030 (test.test_multibytecodec.Test_StreamWriter)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/py3k/__svn__/Lib/test/test_multibytecodec.py",
line 185, in test_gb18030
    self.assertEqual(s.getvalue(), b'123\x907\x959')
AssertionError: b'123\x907\x959\x907\x959' != b'123\x907\x959'

----------------------------------------------------------------------

Is this related to issue3297 ?

I don't think so. Issue 3297 seems related to the way unicode objects
are marshalled/unmarshalled, even if the build settings don't change.

I have the same failure on trunk (narrow build).

The attached patch skips the peepholer optimizations for BINARY_SUBSCR if the resulting char is a surrogate on narrow builds or a non-bmp char in wide builds.
Note that this affects the optimization of lone surrogates on narrow builds too, but I think it's not worth to adding more complexity on the peepholer and check if they are part of a surrogate pair.
The patch still lacks comments and could have better tests.

Ezio Melotti wrote:
> 
> Ezio Melotti <ezio.melotti@gmail.com> added the comment:
> 
> The attached patch skips the peepholer optimizations for BINARY_SUBSCR if the resulting char is a surrogate on narrow builds or a non-bmp char in wide builds.
> Note that this affects the optimization of lone surrogates on narrow builds too, but I think it's not worth to adding more complexity on the peepholer and check if they are part of a surrogate pair.
> The patch still lacks comments and could have better tests.

             newconst = PyObject_GetItem(v, w);
+            if (PyUnicode_Check(v)) {
+                Py_UNICODE ch = PyUnicode_AS_UNICODE(newconst)[0];

Without checking, you shouldn't assume that newconst is a PyUnicodeObject.

Other than that the patch looks fine.

Are there any cases where v[w] -- where v is a unicode object -- returns a non-unicode object?

Ezio Melotti wrote:
> 
> Ezio Melotti <ezio.melotti@gmail.com> added the comment:
> 
> Are there any cases where v[w] -- where v is a unicode object -- returns a non-unicode object?

There could be: either from subclasses or from buggy code. In any case,
macros should only be used if you're certain that the object cannot
be anything else.

Also note that newconst can well be NULL.

Here's a new patch that checks that newconst is not NULL and that it's a unicode object.
I added a test for the case where it's NULL.  I don't think it's possible to test the case when newconst is not unicode though, because unicode subclasses are not literals and don't get optimized in the first place.

> ... which does not give the same result in UCS-2 and UCS-4 builds.
> As a result, the pyc file is not portable across those builds.

Since Python 3.2, the pyc filename contains a tag (u) to indicate wide build (sys.maxunicode==0x10FFFF), instead of narrow (sys.maxunicode==0xFFFF).

I think we can keep the optimizer for Python >= 3.2. I suppose that Python 3.1 has the bug.

Ezio Melotti wrote:
> 
> Ezio Melotti <ezio.melotti@gmail.com> added the comment:
> 
> Here's a new patch that checks that newconst is not NULL and that it's a unicode object.
> I added a test for the case where it's NULL.  I don't think it's possible to test the case when newconst is not unicode though, because unicode subclasses are not literals and don't get optimized in the first place.

Thank you.

PEP 3147 says[0]:
"""
For backward compatibility, Python will still support pyc-only distributions, however it will only do so when the pyc file lives in the directory where the py file would have been, i.e. not in the __pycache__ directory. pyc file outside of __pycache__ will only be imported if the py source file is missing.
"""

Does that mean that there could be cases where untagged pyc files are used in 3.2+?
In that case the patch should be ported to 3.2 and 3.3 too.

[0]: http://www.python.org/dev/peps/pep-3147/#rationale

Ezio Melotti wrote:
> 
> Ezio Melotti <ezio.melotti@gmail.com> added the comment:
> 
> PEP 3147 says[0]:
> """
> For backward compatibility, Python will still support pyc-only distributions, however it will only do so when the pyc file lives in the directory where the py file would have been, i.e. not in the __pycache__ directory. pyc file outside of __pycache__ will only be imported if the py source file is missing.
> """
> 
> Does that mean that there could be cases where untagged pyc files are used in 3.2+?

Yes... even though we did discuss using the same tagging support
in that scenario as well, at least for 3.3.

> In that case the patch should be ported to 3.2 and 3.3 too.
> 
> [0]: http://www.python.org/dev/peps/pep-3147/#rationale

Do you think this should go in 3.1 too?

Ezio Melotti wrote:
> 
> Ezio Melotti <ezio.melotti@gmail.com> added the comment:
> 
> Do you think this should go in 3.1 too?

If the problem triggers there as well: Yes.

Is the problem also visible on Python 2.7 ?

Yes. The original report was for 2.6.
I will apply the patch on all the 4 branches then.

New changeset 3cffa2009a92 by Ezio Melotti in branch '2.7':
Issue #5057: fix a bug in the peepholer that led to non-portable pyc files between narrow and wide builds while optimizing BINARY_SUBSCR on non-BMP chars (e.g. u"\U00012345"[0]).
http://hg.python.org/cpython/rev/3cffa2009a92

New changeset 4679d0fef389 by Ezio Melotti in branch '3.1':
Issue #5057: fix a bug in the peepholer that led to non-portable pyc files between narrow and wide builds while optimizing BINARY_SUBSCR on non-BMP chars (e.g. "\U00012345"[0]).
http://hg.python.org/cpython/rev/4679d0fef389

New changeset 503578ddf286 by Ezio Melotti in branch '3.2':
#5057: Merge with 3.1.
http://hg.python.org/cpython/rev/503578ddf286

New changeset 9801e1f78264 by Ezio Melotti in branch 'default':
#5057: Merge with 3.2.
http://hg.python.org/cpython/rev/9801e1f78264

Sorry to re-open this issue.  The following example shows that it was not fully resolved:

    def f():
        return u'\U00023456abcdef'[3]
    import dis; dis.dis(f)
    print f()

On a wide build it should print 'c' and on a narrow build it should print 'b'.  But if the .pyc file was created on the other platform, it behaves like the other platform would.

This whole issue doesn't affect 3.3.

For 2.7/3.2 there are three possible options:
 1) remove constant folding altogether on unicode (this is the solution adopted by PyPy);
 2) scan the string up to the index looking for non-BMP chars and disable the constant folding if they are found (probably not very efficient);
 3) leave the "buggy" code there (might lead to obscure failures in remote cases);

Any opinions?

Attached a patch that implements option 1).

Option 2) would have my preference.

Did anyone ever show that this particular detail, which looks like a completely obscure case to me, has any measurable effect on any code whatsoever?  Just coming up with numbers, but I'm sure it gives you 5% on the most specially tuned micro-benchmark, and nothing at all in all other cases.

Just saying my vote goes for option 1, but I won't argue if people feel that it's a good investment of their time :-)

I prefer option (1), remove the buggy optimization. Python 3.3 does solve correctly this issue.

issue5057-3.diff LGTM.

I added debug output in peepholer, ran tests and found that this optimization happened for unicode strings only in test_multibytecodec (where it used deliberately) and test_peepholer.  Seems as this is very rare case.

New changeset 9481e062fe26 by Ezio Melotti in branch '2.7':
#5057: the peepholer no longer optimizes subscription on unicode literals (e.g. u"foo"[0]) in order to produce compatible pyc files between narrow and wide builds.
http://hg.python.org/cpython/rev/9481e062fe26

New changeset 56bc323288d1 by Ezio Melotti in branch '3.2':
#5057: the peepholer no longer optimizes subscription on unicode literals (e.g. u"foo"[0]) in order to produce compatible pyc files between narrow and wide builds.
http://hg.python.org/cpython/rev/56bc323288d1

New changeset 3b4f2f9272b4 by Ezio Melotti in branch '3.3':
#5057: null merge with 3.2 (only add tests).
http://hg.python.org/cpython/rev/3b4f2f9272b4

New changeset 0790c16bb275 by Ezio Melotti in branch 'default':
#5057: null merge with 3.3 (only add tests).
http://hg.python.org/cpython/rev/0790c16bb275