str.format_map raises a SystemError for format strings with positional arguments

[Julian]

BPO	12579
Nosy	@ericvsmith, @Julian
Files	format_map_err.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/ericvsmith'
closed_at = <Date 2011-07-18.18:03:52.738>
created_at = <Date 2011-07-17.14:43:42.190>
labels = ['interpreter-core']
title = 'str.format_map raises a SystemError for format strings with positional arguments'
updated_at = <Date 2011-07-18.18:03:52.736>
user = 'https://github.com/Julian'

bugs.python.org fields:

activity = <Date 2011-07-18.18:03:52.736>
actor = 'python-dev'
assignee = 'eric.smith'
closed = True
closed_date = <Date 2011-07-18.18:03:52.738>
closer = 'python-dev'
components = ['Interpreter Core']
creation = <Date 2011-07-17.14:43:42.190>
creator = 'Julian'
dependencies = []
files = ['22683']
hgrepos = []
issue_num = 12579
keywords = ['patch']
message_count = 13.0
messages = ['140528', '140530', '140537', '140539', '140540', '140541', '140542', '140546', '140547', '140549', '140551', '140553', '140610']
nosy_count = 3.0
nosy_names = ['eric.smith', 'Julian', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue12579'
versions = ['Python 3.2', 'Python 3.3']

[Julian]

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.

[ericvsmith]

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.

[ericvsmith]

Actually that's probably not the place to catch it. Let me look at it closer. Of course, patches welcome!

[Julian]

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.

[Julian]

Sorry for the double post, meant to say, in line 506.

[ericvsmith]

I think KeyError for "{foo}".format(12) is correct. It's looking up "foo" in the empty dict of **kwargs.

[Julian]

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.

[ericvsmith]

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.

[ericvsmith]

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

[Julian]

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 :).

[ericvsmith]

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.

[Julian]

Added, updated the patch :). Thank you!

[python-dev]

New changeset f6d074a7bbd4 by Eric V. Smith in branch '3.2':
Closes bpo-12579. Positional fields with str.format_map() now raise a ValueError instead of SystemError.
http://hg.python.org/cpython/rev/f6d074a7bbd4