homepage

The following leads to a SyntaxError in 3.0:
 compile(b'# coding: latin-1\nu = "\xC7"\n', '<dummy>', 'exec')
That is not the case in Python 2.6.

Looks like Parser/tokenizer.c:check_coding_spec() considered Latin-1 a
raw encoding just like UTF-8. Patch is in the works.

Here is a potential fix. It broke test_imp because it assumed that
Latin-1 source files would be encoded at Latin-1 instead of UTF-8 when
returned by imp.new_module(). Doesn't seem like a critical change as the
file is still properly decoded.

Attached is a test for test_pep3120 (since that is what most likely
introduced the breakage). It's a separate patch since the source file is
marked as binary and thus can't be diffed by ``svn diff``.

Can someone double-check this patch for me? I don't have much experience
with the parser so I want to make sure I am not doing anything wrong.

There is a potential dependency on issue3594 as it would change how
imp.find_module() acts and thus make test_imp no longer fail in the way
it has.

That line dates back to the PEP 263 implementation. Martin?

Since this is marked "release blocker", I'll provide a shallow comment:
I don't think it should be a release blocker. It's a bug in the compile
function, and there are various work-arounds (such as saving the bytes
to a temporary file and executing that one, or decoding the byte string
to a Unicode string, and then compiling the Unicode string). It is
sufficient to fix it in 3.0.1.
I don't think the patch is right: as the test had to be changed, it
means that somewhere, the detection of the encoding declaration now
fails. This is clearly a new bug, but I don't have the time to analyse
the cause further.
In principle, there is nothing wrong with the tokenizer treating latin-1
as "raw" - that only means we don't go through a codec.

Actually, the tests don't have to change; if issue 3594 gets applied
then that change cascades into this issue and negates the need to change
the tests themselves.
As for treating Latin-1 as a raw encoding, how can that be theoretically
okay if the parser assumes UTF-8 and Latin-1 is not a superset of Latin-1?

> As for treating Latin-1 as a raw encoding, how can that be theoretically
> okay if the parser assumes UTF-8 and Latin-1 is not a superset of Latin-1?
The parser doesn't assume UTF-8, but "ascii+", i.e. it passes all
non-ASCII bytes on to the AST, which then needs to deal with them;
it then could (but apparently doesn't) take into account whether the
internal representation was UTF-8 or Latin-1: see ast.c:decode_unicode
for some remains of that.
The other case (besides string literals) where bytes > 127 matter is
tokenizer.c:verify_identifier; this indeed assumes UTF-8 only (but
could be easily extended to support Latin-1 as well).
The third case where non-ASCII bytes are allowed is comments; there
they are entirely ignored (i.e. it is not even verified that the
comment is well-formed UTF-8).
Removal of the special case should simplify the code; I would agree
that any speedup gained by not going through a codec is irrelevant.
I'm still puzzled why test_imp if the special case is removed.

The test_imp stuff has to do with PyTokenizer_FindEncoding().
imp.find_module() only opens the file, passes the file descriptor to
PyTokenizer_FindEncoding() and then returns a file object with the found
encoding.
Problem is that (as issue 3594 points out), PyTokenizer_FindEncoding()
always fails. That means it assumes only the raw encodings are okay.
With Latin-1 being one of them, it returns the file opened as Latin-1 as
is correct. Removing that case here means PyTokenizer_FindEncoding()
fails, and thus assumes only UTF-8 as a legitimate encoding and opens
the files with the UTF-8 encoding. It took a while to find these two
bugs obviously. =)

I have attached a new version of the patch with the changes to test_imp
removed as issue 3594 fixed the need for the change. I have also
directly uploaded test_pep3120.py since it is flagged as binary and thus
cannot be diffed by svn.

Using py3k trunk + fix_latin.diff:
 - compile(b'# coding: latin-1\nu = "\xC7"\n', '<dummy>', 'exec') 
doesn't fail
 - test_pep3120.py is ok
 - but execute a ISO-8859-1 script fails: see attached iso.py
Original Python3:
$ python iso.py
'Bonjour ma ch\xe8re amie'
Patched Python3:
$ python iso.py
'Bonjour ma ch\xc3\xa8re amie'

See also Lib/test/test_shlex.py: trunk is ok, but with fix_latin.diff 
the test fails.

It looks like the problem of fix_latin.diff is the decoding_state: 
it's set to STATE_NORMAL whereas current behaviour is to stay in state 
STATE_RAW.
I wrote another patch which is a mix of case 1 (utf-8: just set 
tok->encoding) and case 2 (another charset: set tok->enc, 
tok->encoding and tok>decoding_state): a new case 3 which set enc, 
encoding but stay a the state STATE_RAW. I don't understand my patch, 
so review it (twice or more :-D). Using my patch:
 - compile(...) works
 - test_shlex.py works
 - test_pep3120.py
 - iso.py works

But why does iso-8859-1 need to be treated as a special case? UTF-8 is
special because it is the default encoding for source. But iso-8859-1
really shouldn't be special, IMO.
Your patch does exactly what happens lower when set_readline() succeeds.
So I want to know why set_readline() is not returning something for
iso-8859-1 (which your patch seems to be suggesting).

Sorry, I mis-spoke: your patch, Victor, doesn't change the state to
NORMAL. But my worry still stands; why does iso-8859-1 need to be
special-cased? It suggests to me that some more fundamental needs to be
dealt with instead of just patching around iso-8859-1.

@brett.cannon: I found it: ast.c used a hack for iso-8859-1! Since 
this hack introduces a bug (your compile(...) example), I prefer to 
remove it to simplify to code. The new patch just removes the hack in 
tokenizer.c and ast.c. It does also document encoding and enc 
attributes of tok_state in tokenizer.h.
Using tokenizer_iso-8859-1-patch2.patch, all tests (test_pep3120.py, 
iso.py, test_shlex.py, etc.) are OK.

Thanks for finding that, Victor! I will do a patch review when I have a
chance (it won't be until after the weekend).

After reading tokenizer.c 1000 times, I finally used grep:
$ grep -l -i 'iso.8859.1' $(find -name "*.c")
./Python/ast.c <~~~ WTF?
./Objects/unicodeobject.c
./Parser/tokenizer.c
./Modules/cjkcodecs/_codecs_iso2022.c
./Modules/expat/xmltok.c

My patch version 2 included an "unrelated" fix for the issue2384.

So I tried the patch (I attached my version with different comments in
the header file and removed some redundant change in whitespacing), and
test_sys consistently fails for me:
 test_current_frames (__main__.SysModuleTest)
 AssertionError: '' != 'g456()'
And this is after a ``make distclean`` and a new compile. This failure
doesn't occur on a clean checkout made after a 'distclean'.

test_sys failure is fixed by the issue #2384.

Amaury applied my both patches for issues #2384 and #3975. So all 
tests now pass with python trunk + alt_latin_1.diff.

Yep, it passes for me now.
Martin, have any objection to this patch?

The patch looks fine to me, please apply.
I notice that the diff file reports changes to test_pep3120.py. No such
changes should be necessary, so please exclude them from committing.

On Tue, Oct 14, 2008 at 11:05 PM, Martin v. Löwis
<report@bugs.python.org> wrote:
>
> Martin v. Löwis <martin@v.loewis.de> added the comment:
>
> The patch looks fine to me, please apply.
>
Great!
> I notice that the diff file reports changes to test_pep3120.py. No such
> changes should be necessary, so please exclude them from committing.
>
I added a test for compile() in there, which is why the patch is
claiming that. There is an uploaded version of test_pep3120.py on the
issue.

> I added a test for compile() in there, which is why the patch is
> claiming that. There is an uploaded version of test_pep3120.py on the
> issue.
Ah, ok. I missed that - that change is also fine.

Brett, please apply and close the issue.

Sorry about that; been one of those days.
Doing a svn up and making sure it still compiles fine.

Applied in r66951.