Decode __file__ and co_filename to unicode using fs default

[tiran]

BPO	1272
Nosy	@gvanrossum, @tiran, @avassalotti
Files	py3k_file_fsenc2.patch updated_file_fsenc.patch updated_file_fsenc-2.patch updated_file_fsenc-3.patch updated_file_fsenc-4.patch updated_file_fsenc-5.patch updated_file_fsenc-6.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gvanrossum'
closed_at = <Date 2007-10-15.02:53:16.535>
created_at = <Date 2007-10-12.13:41:38.732>
labels = ['interpreter-core']
title = 'Decode __file__ and co_filename to unicode using fs default'
updated_at = <Date 2007-10-16.01:25:31.260>
user = 'https://github.com/tiran'

bugs.python.org fields:

activity = <Date 2007-10-16.01:25:31.260>
actor = 'christian.heimes'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2007-10-15.02:53:16.535>
closer = 'gvanrossum'
components = ['Interpreter Core']
creation = <Date 2007-10-12.13:41:38.732>
creator = 'christian.heimes'
dependencies = []
files = ['8524', '8530', '8532', '8533', '8534', '8537', '8538']
hgrepos = []
issue_num = 1272
keywords = ['patch']
message_count = 31.0
messages = ['56374', '56388', '56389', '56390', '56391', '56393', '56394', '56396', '56397', '56398', '56401', '56402', '56404', '56405', '56406', '56408', '56414', '56415', '56417', '56419', '56420', '56422', '56425', '56426', '56427', '56428', '56429', '56430', '56432', '56482', '56483']
nosy_count = 3.0
nosy_names = ['gvanrossum', 'christian.heimes', 'alexandre.vassalotti']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1272'
versions = ['Python 3.0']

[tiran]

I'm sending the patch in for review.

[gvanrossum]

Couple of nits:

• You added a removal of hotshot from setup.py to the patch; but that's
  been checked in in the mean time.

• Why add an 'errors' argument to the function when it's a fatal error
  to use it?

• Using 0 to autodetect the length is scary. Normally we have two APIs
  for that, one ..._FromString and one ...FromStringAndSize. If you
  really don't want that, please use -1, which is at least an illegal value.

• Why is there code in codeobject.c::PyCode_New() that still accepts a
  PyString for the filename?

• In that file (and possibly others, I didn't check) your code uses
  spaces to indent while the surrounding code uses tabs. Moreover, your
  space indent seems to assume there are 4 spaces to a tab, but all our
  code (Python and C) is formatted assuming tabs are 8 spaces. (The
  indent isn't always 8 spaces -- but ASCII TAB characters always are 8,
  for us.)

• Why copy the default encoding before mangling it? With a little extra
  care you will only have to copy it once. Also, consider not mangling at
  all, but assuming the encoding comes in a canonical form -- several
  other functions assume that, e.g. PyUnicode_Decode() and
  PyUnicode_AsEncodedString().

• I haven't run the unit tests yet. Will be doing that next...

[avassalotti]

I found a few problems in your patch. In PyCode_New() the type check
for the filename argument is incorrect:

--- Objects/codeobject.c	(revision 58412)
+++ Objects/codeobject.c	(working copy)
@@ -59,7 +59,7 @@
 	    freevars == NULL || !PyTuple_Check(freevars) ||
 	    cellvars == NULL || !PyTuple_Check(cellvars) ||
 	    name == NULL || (!PyString_Check(name) && !PyUnicode_Check(name)) ||
-	    filename == NULL || !PyString_Check(filename) ||
+	    filename == NULL || (!PyString_Check(name) &&
!PyUnicode_Check(name)) ||
 	    lnotab == NULL || !PyString_Check(lnotab) ||
 	    !PyObject_CheckReadBuffer(code)) {
 		PyErr_BadInternalCall();

@@ -260,6 +267,8 @@
 		ourcellvars = PyTuple_New(0);
 	if (ourcellvars == NULL)
 		goto cleanup;
+    filename = PyUnicode_DecodeFSDefault(PyString_AS_STRING(filename),
+                                         0, NULL);

The following is unnecessary and will cause a reference leak:

@@ -260,6 +267,8 @@
 		ourcellvars = PyTuple_New(0);
 	if (ourcellvars == NULL)
 		goto cleanup;
+    filename = PyUnicode_DecodeFSDefault(PyString_AS_STRING(filename),
+                                         0, NULL);
 
 	co = (PyObject *)PyCode_New(argcount, kwonlyargcount,
 				    nlocals, stacksize, flags,

I think the interface of PyUnicode_DecodeFSDefault() could be improved
a bit. The function doesn't use the last argument 'errors', so why is
there? I am not sure if it is useful to keep second argument,
'length', either. So, I believe the function prototype should be
changed to:

    PyObject *PyUnicode_Decode_FSDefault(const char *s);

Another thing that I am not sure about is whether it is correct to
consider ISO-8859-15 the same thing as Latin-1.

Overall, the patch looks good to me and doesn't cause any test to
fail. I attached an updated patch with the above issues fixed.

Thank you, Christian, for the patch. :)

[avassalotti]

Guido wrote:

Why copy the default encoding before mangling it? With a little
extra care you will only have to copy it once. Also, consider not
mangling at all, but assuming the encoding comes in a canonical form
-- several other functions assume that, e.g. PyUnicode_Decode() and
PyUnicode_AsEncodedString().

It is impossible guarantee that Py_FileSystemDefaultEncoding is
normalized, since its value can be set using nl_langinfo(CODESET)
during the bootstrapping process. PyUnicode_Decode() and other
decoding/encoding functions use the codec module, which is not available
during the early bootstrapping process, to normalize the encoding name.

[tiran]

Guido van Rossum wrote:

• You added a removal of hotshot from setup.py to the patch; but that's
  been checked in in the mean time.

Oh, the change shouldn't make it into the patch. I guess I forgot a svn
revert on setup.py

• Why add an 'errors' argument to the function when it's a fatal error
  to use it?

I wanted the signature of the method be equal to the other methods
PyUnicode_Decode*. I copied the FatalError from
*_PyUnicode_AsDefaultEncodedString().

• Using 0 to autodetect the length is scary. Normally we have two APIs
  for that, one ..._FromString and one ...FromStringAndSize. If you
  really don't want that, please use -1, which is at least an illegal value.

Oh right, -1 is *much* better for autodetect than 0. What do you prefer,
a second method or -1 as auto detect?

• Why is there code in codeobject.c::PyCode_New() that still accepts a
  PyString for the filename?

Because it's my fault that I've overseen it. :/

• In that file (and possibly others, I didn't check) your code uses
  spaces to indent while the surrounding code uses tabs. Moreover, your
  space indent seems to assume there are 4 spaces to a tab, but all our
  code (Python and C) is formatted assuming tabs are 8 spaces. (The
  indent isn't always 8 spaces -- but ASCII TAB characters always are 8,
  for us.)

Some C files like unicodeobject.c are using 4 spaces while other files
are using tabs for indention. My editor may got confused by the mix.
I've manually fixed it in the patch but I may have overseen a line or two.

• Why copy the default encoding before mangling it? With a little extra
  care you will only have to copy it once. Also, consider not mangling at
  all, but assuming the encoding comes in a canonical form -- several
  other functions assume that, e.g. PyUnicode_Decode() and
  PyUnicode_AsEncodedString().

My C is a bit rusty and still need to learn news tricks. I'm trying to
see if I can remove the extra copy without causing a problem.
The other part of your question was already answered by Alexandre. The
aliases map is defined in Python code. It's not available so early in
the boot strapping process.
We'd have to redesign the assignment of co_filename and __file__
completely if we want to use the aliases and other codecs. For example
we could store a PyString at first and redo all names once the codecs
are set up.

Christian

[gvanrossum]

On 10/13/07, Christian Heimes <report@bugs.python.org> wrote:

Guido van Rossum wrote:
> - Why add an 'errors' argument to the function when it's a fatal error
> to use it?

I wanted the signature of the method be equal to the other methods
PyUnicode_Decode*. I copied the FatalError from
*_PyUnicode_AsDefaultEncodedString().

But that function is a terrible example; it was done that way because
an earlier version of the function *did* allow using the errors
argument and I wanted to make sure to catch all calls that were still
passing an errors value. This doesn't apply here, we're creating a
brand new API.

> - Using 0 to autodetect the length is scary. Normally we have two APIs
> for that, one ..._FromString and one ...FromStringAndSize. If you
> really don't want that, please use -1, which is at least an illegal value.

Oh right, -1 is *much* better for autodetect than 0. What do you prefer,
a second method or -1 as auto detect?

Even better is Alexandre's version: always autodetect. I think we can
assume that filenames are always available as 0-terminated byte
arrays, since that's how all system calls need them.

Anyway, let me know if you want to change something in Alexandre's
version or if you want him to check it in.

Oh. Hm. I still wish that PyCode_New() could just insist that the
filename argument is a PyUnicode instance. Why can't it? Perhaps the
caller should be fixed instead?

[tiran]

Guido van Rossum wrote:

But that function is a terrible example; it was done that way because
an earlier version of the function *did* allow using the errors
argument and I wanted to make sure to catch all calls that were still
passing an errors value. This doesn't apply here, we're creating a
brand new API.

Ahhh! I really didn't know it. I thought that the non functional
arguments have a purpose.

Anyway, let me know if you want to change something in Alexandre's
version or if you want him to check it in.

I'm going to create a new patch based on his fixes and your recommendations.

Oh. Hm. I still wish that PyCode_New() could just insist that the
filename argument is a PyUnicode instance. Why can't it? Perhaps the
caller should be fixed instead?

I'll try.

[gvanrossum]

> Oh. Hm. I still wish that PyCode_New() could just insist that the
> filename argument is a PyUnicode instance. Why can't it? Perhaps the
> caller should be fixed instead?

I'll try.

I figured out the problem -- it came from marshalled old code objects.
If you throw away all .pyc files the problem goes away. You can also
get rid of the similar checks for the 'name' argument -- this should
just be a PyUnicode too. A systematic approach to invalidating all the
.pyc files is updating the magic number in import.c.

[tiran]

Guido van Rossum wrote:

• Why copy the default encoding before mangling it? With a little extra
  care you will only have to copy it once.

Now I remember why I added the strncpy() call plus encoding[31] = '\0'.
I wanted to make sure that the code doesn't break even if the encoding
name is longer than 31 + 1 chars long. I'm aware that it's very unlikely
but I didn't want to take chances. You are allowed to call me paranoid. *g*

Christian

[gvanrossum]

Well, you could ensure that by checking that you haven't reached the
end of the mangling buffer. That will have the added advantage that
when the input is something silly like 32 spaces followed by "utf-8"
it will be still be mangled correctly. The slight extra cost of the
check (could be a single pointer compare) is offset by saving a call
to strncpy().

--Guido

On 10/13/07, Christian Heimes <report@bugs.python.org> wrote:

Christian Heimes added the comment:

Guido van Rossum wrote:
> - Why copy the default encoding before mangling it? With a little extra
> care you will only have to copy it once.

Now I remember why I added the strncpy() call plus encoding[31] = '\0'.
I wanted to make sure that the code doesn't break even if the encoding
name is longer than 31 + 1 chars long. I'm aware that it's very unlikely
but I didn't want to take chances. You are allowed to call me paranoid. *g*

Christian

---

Tracker <report@bugs.python.org>
<http://bugs.python.org/issue1272\>

---

[avassalotti]

Guido wrote:

I figured out the problem -- it came from marshalled old code objects.
If you throw away all .pyc files the problem goes away. You can also
get rid of the similar checks for the 'name' argument -- this should
just be a PyUnicode too. A systematic approach to invalidating all the
.pyc files is updating the magic number in import.c.

Done.

I had to remove a few another PyString instances in pyexpat.c and
_ctypes.c. So, here (hopefully) the final version of the patch.

The changes from the last version are:

• Correct a typo in of the comments in PyUnicode_DecodeFSDefault
• Specified in the API doc of PyUnicode_DecodeFSDefault that the
  function take a null-terminated string.
• Bumped the magic number in import.c
• Fix PyCode_New calls in _ctypes and pyexpat module.
• Remove the PyString type check on 'filename' and 'name' in PyCode_New.
• Remove the unneeded string coercion code from PyCode_New.

[avassalotti]

Remove the PyString type check on 'filename' and 'name' in PyCode_New.

Oops. I removed one of the ! the checks by mistake.

[gvanrossum]

Crys, is this OK with you?

On 10/13/07, Alexandre Vassalotti <report@bugs.python.org> wrote:

Alexandre Vassalotti added the comment:

Guido wrote:
> I figured out the problem -- it came from marshalled old code objects.
> If you throw away all .pyc files the problem goes away. You can also
> get rid of the similar checks for the 'name' argument -- this should
> just be a PyUnicode too. A systematic approach to invalidating all the
> .pyc files is updating the magic number in import.c.

Done.

I had to remove a few another PyString instances in pyexpat.c and
_ctypes.c. So, here (hopefully) the final version of the patch.

The changes from the last version are:

• Correct a typo in of the comments in PyUnicode_DecodeFSDefault
• Specified in the API doc of PyUnicode_DecodeFSDefault that the
  function take a null-terminated string.
• Bumped the magic number in import.c
• Fix PyCode_New calls in _ctypes and pyexpat module.
• Remove the PyString type check on 'filename' and 'name' in PyCode_New.
• Remove the unneeded string coercion code from PyCode_New.

---

Tracker <report@bugs.python.org>
<http://bugs.python.org/issue1272\>

---