Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String interpolation doesn't work with sys.version_info #52660

Closed
Arfrever mannequin opened this issue Apr 16, 2010 · 23 comments
Closed

String interpolation doesn't work with sys.version_info #52660

Arfrever mannequin opened this issue Apr 16, 2010 · 23 comments
Assignees
Labels
interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@Arfrever
Copy link
Mannequin

Arfrever mannequin commented Apr 16, 2010

BPO 8413
Nosy @rhettinger, @abalkin, @pitrou, @ericvsmith, @benjaminp, @ezio-melotti, @briancurtin
Files
  • structseq-inheritance.patch
  • structseq-inheritance2.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/benjaminp'
    closed_at = <Date 2010-07-07.20:54:27.486>
    created_at = <Date 2010-04-16.01:28:08.841>
    labels = ['interpreter-core', 'type-bug']
    title = "String interpolation doesn't work with sys.version_info"
    updated_at = <Date 2010-07-08.19:33:03.669>
    user = 'https://bugs.python.org/Arfrever'

    bugs.python.org fields:

    activity = <Date 2010-07-08.19:33:03.669>
    actor = 'belopolsky'
    assignee = 'benjamin.peterson'
    closed = True
    closed_date = <Date 2010-07-07.20:54:27.486>
    closer = 'benjamin.peterson'
    components = ['Interpreter Core']
    creation = <Date 2010-04-16.01:28:08.841>
    creator = 'Arfrever'
    dependencies = []
    files = ['17890', '17896']
    hgrepos = []
    issue_num = 8413
    keywords = ['patch']
    message_count = 23.0
    messages = ['103282', '103305', '103797', '104519', '104521', '104523', '104524', '104525', '104529', '104530', '104531', '108497', '109445', '109495', '109496', '109497', '109498', '109499', '109500', '109501', '109502', '109556', '109577']
    nosy_count = 8.0
    nosy_names = ['rhettinger', 'belopolsky', 'pitrou', 'eric.smith', 'benjamin.peterson', 'ezio.melotti', 'Arfrever', 'brian.curtin']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue8413'
    versions = ['Python 3.1', 'Python 2.7', 'Python 3.2']

    @Arfrever
    Copy link
    Mannequin Author

    Arfrever mannequin commented Apr 16, 2010

    String interpolation doesn't work with sys.version_info in Python versions in which sys.version_info is a named tuple. It's a regression in Python 2.7 and 3.1. This problem doesn't concern named tuples created using collections.namedtuple().

    This problem might also concern sys.getwindowsversion(), but I don't have access to Windows system, so I can't check it.

    $ python2.6 -c 'import sys; print("%s.%s.%s-%s-%s" % sys.version_info)'
    2.6.5-final-0
    $ python2.7 -c 'import sys; print("%s.%s.%s-%s-%s" % sys.version_info)'
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    TypeError: not enough arguments for format string
    $ python3.0 -c 'import sys; print("%s.%s.%s-%s-%s" % sys.version_info)'
    3.0.1-final-0
    $ python3.1 -c 'import sys; print("%s.%s.%s-%s-%s" % sys.version_info)'
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    TypeError: not enough arguments for format string

    @Arfrever Arfrever mannequin added the stdlib Python modules in the Lib dir label Apr 16, 2010
    @ericvsmith
    Copy link
    Member

    ericvsmith commented Apr 16, 2010

    This affects any type implemented as PyStructSequence. For example, sys.flags has the same behavior.

    @ericvsmith ericvsmith added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error and removed stdlib Python modules in the Lib dir labels Apr 16, 2010
    @ericvsmith
    Copy link
    Member

    ericvsmith commented Apr 21, 2010

    I think the right way to handle this is to modify the test:

    if (Py_TYPE(args)->tp_as_mapping && !PyTuple_Check(args) &&
    !PyObject_TypeCheck(args, &PyBaseString_Type))

    to also exclude PyStructSequence's, but since they're all distinct types I don't see how to do that. I don't really think listing all of the PyStructSequence types would be acceptable.

    This is the test that's trying to figure out if %-formatting should use mapping or tuple expansion.

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Apr 29, 2010

    I am not sure fixing this quirk of %-formatting is worth the trouble given that one simply use new style {}-formatting:

    $ python-c 'import sys; print("{}.{}.{}-{}-{}".format(*sys.version_info))'
    3.2.0-alpha-0

    or a work-around

    $ python -c 'import sys; print("%s.%s.%s-%s-%s" % tuple(sys.version_info))'
    3.2.0-alpha-0

    @pitrou
    Copy link
    Member

    pitrou commented Apr 29, 2010

    Well, the problem is that it's a regression from 2.6.
    It also shows why these "fancy" structs aren't necessarily a good idea.
    I agree fixing in py3k is less important, though it would be nice too.

    @ericvsmith
    Copy link
    Member

    ericvsmith commented Apr 29, 2010

    The other thing to do would be to convince PyTuple_Check that PyStructSequences are tuples, but that would no doubt come with its own set of regressions.

    Since this problem is 1) hard to fix and 2) probably of minimal impact, I think the best course is to do nothing.

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Apr 29, 2010

    The recommended way to check that an object is an instance of a namedtuple is to check for the _fields attribute. See bpo-7796, msg99869.

    @ericvsmith
    Copy link
    Member

    ericvsmith commented Apr 29, 2010

    Yes, but sys.version_info isn't a namedtuple (which are in fact tuples), it's the (sort-of) C equivalent, which isn't a real tuple.

    >>> from collections import namedtuple
    >>> x = namedtuple('x', 'a b c')
    >>> '%s %s %s' % x(1, 2, 3)
    '1 2 3'
    
    Hmm, but sys.version_info is a tuple:
    >>> isinstance(sys.version_info, tuple)
    True

    I'll have to check if PyTuple_Check returns true or not. Maybe the problem is in the mapping check.

    I'll investigate more.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Apr 29, 2010

    IIRC, there is an open ticket to make structseq inherit from tuple to avoid just this kind of problem.

    @ericvsmith
    Copy link
    Member

    ericvsmith commented Apr 29, 2010

    Looks like that's bpo-1820.

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Apr 29, 2010

    On Thu, Apr 29, 2010 at 11:52 AM, Raymond Hettinger
    <report@bugs.python.org> wrote:
    ..

    IIRC, there is an open ticket to make structseq inherit from tuple to avoid just this kind of problem.

    Indeed, see bpo-1820.

    @Arfrever
    Copy link
    Mannequin Author

    Arfrever mannequin commented Jun 24, 2010

    Should this regression block final release of 2.7 or can it be fixed in e.g. 2.7.1?

    @benjaminp
    Copy link
    Contributor

    benjaminp commented Jul 7, 2010

    Here's a patch. It makes structseq a subclass of tuple and along the way deletes tons of code. Please review.

    @benjaminp benjaminp assigned benjaminp and unassigned ericvsmith Jul 7, 2010
    @abalkin
    Copy link
    Member

    abalkin commented Jul 7, 2010

    This is definitely the right way to do it. I expect that I will have only minor nit-picks as I go through the patch.

    1. You can probably just do
    #define PyStructSequence_SET_ITEM PyTuple_SET_ITEM
    #define PyStructSequence_GET_ITEM PyTuple_GET_ITEM

    there is no need to repeat the argument lists.

    1. I am comparing PyStructSequence_New and PyTuple_New:
    • PyStructSequence_New does not fill ob_item array with NULLs.
    • PyStructSequence_New does not call _PyObject_GC_TRACK

    I believe tp_free gets inherited, so structseq tp_new should follow what tuple's tp_new does. I am not 100% sure on the second point, though _PyObject_GC_TRACK may be redundant after PyObject_GC_NewVar.

    1. In structseq_repr, do you need PyTuple_GetItem? I think you can get away with PyTuple_GET_ITEM, or better PyStructSequence_GET_ITEM.

    @benjaminp
    Copy link
    Contributor

    benjaminp commented Jul 7, 2010

    2010/7/7 Alexander Belopolsky <report@bugs.python.org>:

    Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

    This is definitely the right way to do it.  I expect that I will have only minor nit-picks as I go through the patch.

    1. You can probably just do

    #define PyStructSequence_SET_ITEM PyTuple_SET_ITEM
    #define PyStructSequence_GET_ITEM PyTuple_GET_ITEM

    there is no need to repeat the argument lists.

    I think the preprocessor requires this. (Anyway it fails without.)

    1. I am comparing PyStructSequence_New and PyTuple_New:
       - PyStructSequence_New does not fill ob_item array with NULLs.

    I'm not sure it really matters, since the fields should always be
    filled in, but I suppose it can't hurt...

     - PyStructSequence_New does not call _PyObject_GC_TRACK

    I believe tp_free gets inherited, so structseq tp_new should follow what tuple's tp_new does.  I am not 100% sure on the second point, though _PyObject_GC_TRACK may be redundant after PyObject_GC_NewVar.

    This is only needed when doing freelist magic in tupleobject.c.

    1. In structseq_repr, do you need PyTuple_GetItem?  I think you can get away with PyTuple_GET_ITEM, or better PyStructSequence_GET_ITEM.

    Yeah, that's fine.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 7, 2010

    On Wed, Jul 7, 2010 at 4:19 PM, Benjamin Peterson
    <report@bugs.python.org> wrote:

    >  - PyStructSequence_New does not fill ob_item array with NULLs.

    I'm not sure it really matters, since the fields should always be
    filled in, but I suppose it can't hurt...

    The following pattern is quite common:

    tuple = PyTuple_New(size):
    for (i = 0; i < size; ++i) {
       item = create_item(...);
       if (item == NULL)
          goto fail;
    }
    ...
    fail:
    Py_DECREF(tuple);

    I don't think this should be outlawed.

    @benjaminp
    Copy link
    Contributor

    benjaminp commented Jul 7, 2010

    (New patch addresses review.)

    @abalkin
    Copy link
    Member

    abalkin commented Jul 7, 2010

    Looks great. I love the patches where few '+'s are drowning in a sea of '-'s!

    A gratuitous nitpick (feel free to ignore). In

    +        val = PyStructSequence_GET_ITEM(obj, i);
             if (cname == NULL || val == NULL) {

    The null check for val is now redundant, but won't hurt.

    @benjaminp
    Copy link
    Contributor

    benjaminp commented Jul 7, 2010

    Applied in r82636.

    @Arfrever
    Copy link
    Mannequin Author

    Arfrever mannequin commented Jul 7, 2010

    I would suggest to document changed behavior of string interpolation with sys.version_info and sys.getwindowsversion() in "What’s New in Python 2.7" page and maybe "What’s New In Python 3.1" page.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 7, 2010

    This was a fantastic check-in. Thanks Benjamin.

    @briancurtin
    Copy link
    Member

    briancurtin commented Jul 8, 2010

    A side effect of this change is that it kills the ability to have a PyStructSequence which has a smaller visible size than the total number of items. For example, sys.getwindowsversion used to have 5 items in the sequence and 4 items accessible by name only (for backwards compatibility) -- now it's a 9 item tuple.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 8, 2010

    A side effect of this change is that it kills the ability to have a
    PyStructSequence which has a smaller visible size than the total
    number of items.

    Hmm. I was looking for this precise issue when I was reviewing the code and it appeared right to me. I know, I should have tested instead. This should be easy to fix, though. I'll look into it now.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants