Title: str.format_map raises a SystemError for format strings with positional arguments
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.2, Python 3.3
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.smith Nosy List: Julian, eric.smith, python-dev
Priority: normal Keywords: patch

Created on 2011-07-17 14:43 by Julian, last changed 2011-07-18 18:03 by python-dev. This issue is now closed.

File name Uploaded Description Edit
format_map_err.patch Julian, 2011-07-18 01:00
Messages (13)
msg140528 - (view) Author: Julian Berman (Julian) * Date: 2011-07-17 14:43
Attached is just a failing test case (just `print("{}".format_map(12))`), haven't been able to decipher the chain of calls in the unicodeobject.c code yet to see where the best place to put the fix would be (inside do_format_map before the pass back upwards maybe?).

Assuming this should be a TypeError obviously, soon as I can figure out where to put the fix I'll update the patch.
msg140530 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-07-17 17:53
If you want to look at this, I think there's a missing check for args being non-null in string_format.h, line 515.

I'd have to think to see if there are other possible failure scenarios.

Thanks for the report.
msg140537 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-07-17 22:00
Actually that's probably not the place to catch it. Let me look at it closer. Of course, patches welcome!
msg140539 - (view) Author: Julian Berman (Julian) * Date: 2011-07-17 22:11
Yeah, I saw the line you suggested and didn't think that was it.

To expand on this, perhaps "{foo}".format(12) should raise a TypeError as well? I'd expect that more than the current behavior which is a KeyError. That KeyError is getting raised in get_field_object, which is where the fix needs to be, or at least needs to be changed, I'm just trying not to duplicate code.
msg140540 - (view) Author: Julian Berman (Julian) * Date: 2011-07-17 22:12
Sorry for the double post, meant to say, in line 506.
msg140541 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-07-17 22:20
I think KeyError for "{foo}".format(12) is correct. It's looking up "foo" in the empty dict of **kwargs.
msg140542 - (view) Author: Julian Berman (Julian) * Date: 2011-07-17 22:23
Fair enough. I suppose I take .format_map to mean, here is a mapping, call __getitem__ on it for each of the keys inside the format string, to which calling 12["foo"] would get me a TypeError.

I suppose I see both as appropriate, but the fact that it's implemented by passing the mapping as kwargs seemed less relevant. Less concerned about that part though I guess.
msg140546 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-07-17 22:44
I think the issue is that it should be an error in any string used for format_map() to have a positional argument. So the right thing to do is detect this case in get_field_object (index != -1 and args is NULL). So I think a new test near line 515 really is the right thing to do. With a good comment explaining what's going on.

I'm pretty sure the only case where args is NULL is when used with format_map(), but that needs to be verified.
msg140547 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-07-17 22:46
Changing the title to reflect the real problem.

You get a SystemError even when using a mapping, if you have a format string with positional arguments:
>>> '{}'.format_map({'a':0})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: null argument to internal routine
msg140549 - (view) Author: Julian Berman (Julian) * Date: 2011-07-18 00:17
Well you're right :). I appreciate you taking more time to help me with this than you could have yourself. I made the change (and changed the TypeError to a ValueError as per your discovery that this was just a positional value issue, hope you agree with that).

Ran the whole test suite, all passes, other than the one test that fails for me without the patch too.

I'm not too happy with the wording on the error, "Format string contained positional fields" doesn't sound nearly specific enough, but I was weary to put something specifically referencing str.format_map in a function that handles str.format as well?

Anyways, appreciate the help again, please let me know if anything needs fixing :).
msg140551 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-07-18 00:53
I definitely agree it should be a ValueError.

How about including in your patch adding your name to Misc/ACKS, if it isn't already there? I don't have your full name.

I might play with the exception wording and add a few more comments. Thanks for your work on this.
msg140553 - (view) Author: Julian Berman (Julian) * Date: 2011-07-18 01:00
Added, updated the patch :). Thank you!
msg140610 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-18 18:03
New changeset f6d074a7bbd4 by Eric V. Smith in branch '3.2':
Closes #12579. Positional fields with str.format_map() now raise a ValueError instead of SystemError.
Date User Action Args
2011-07-18 18:03:52python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg140610

resolution: fixed
stage: patch review -> resolved
2011-07-18 01:01:04Juliansetfiles: - format_map_err.patch
2011-07-18 01:00:43Juliansetfiles: + format_map_err.patch

messages: + msg140553
2011-07-18 00:59:56Juliansetfiles: - format_map_err.patch
2011-07-18 00:53:31eric.smithsetmessages: + msg140551
stage: patch review
2011-07-18 00:17:13Juliansetfiles: + format_map_err.patch

messages: + msg140549
2011-07-17 22:46:19eric.smithsetmessages: + msg140547
title: str.format_map raises a SystemError for non-mapping -> str.format_map raises a SystemError for format strings with positional arguments
2011-07-17 22:44:38eric.smithsetmessages: + msg140546
2011-07-17 22:23:44Juliansetmessages: + msg140542
2011-07-17 22:20:37eric.smithsetmessages: + msg140541
2011-07-17 22:12:58Juliansetmessages: + msg140540
2011-07-17 22:11:48Juliansetmessages: + msg140539
2011-07-17 22:00:23eric.smithsetmessages: + msg140537
2011-07-17 17:53:05eric.smithsetmessages: + msg140530
2011-07-17 17:49:18eric.smithsetassignee: eric.smith
2011-07-17 14:54:39pitrousetnosy: + eric.smith
2011-07-17 14:43:42Juliancreate