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

Return function locals() in order of creation? #76871

Closed
gvanrossum opened this issue Jan 28, 2018 · 21 comments
Closed

Return function locals() in order of creation? #76871

gvanrossum opened this issue Jan 28, 2018 · 21 comments
Assignees
Labels
3.7 interpreter-core type-bug

Comments

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jan 28, 2018

BPO 32690
Nosy @gvanrossum, @rhettinger, @ncoghlan, @vstinner, @ned-deily, @njsmith, @1st1
PRs
  • #5379
  • #5390
  • #5439
  • #5586
  • 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/ned-deily'
    closed_at = <Date 2018-01-30.05:28:39.952>
    created_at = <Date 2018-01-28.03:16:31.510>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'Return function locals() in order of creation?'
    updated_at = <Date 2018-02-08.21:27:40.502>
    user = 'https://github.com/gvanrossum'

    bugs.python.org fields:

    activity = <Date 2018-02-08.21:27:40.502>
    actor = 'rhettinger'
    assignee = 'ned.deily'
    closed = True
    closed_date = <Date 2018-01-30.05:28:39.952>
    closer = 'ncoghlan'
    components = ['Interpreter Core']
    creation = <Date 2018-01-28.03:16:31.510>
    creator = 'gvanrossum'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32690
    keywords = ['patch']
    message_count = 21.0
    messages = ['310905', '310907', '310915', '310925', '310933', '310972', '310976', '310977', '311000', '311003', '311004', '311010', '311014', '311017', '311208', '311223', '311225', '311227', '311231', '311232', '311293']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'rhettinger', 'ncoghlan', 'vstinner', 'ned.deily', 'njs', 'dabeaz', 'yselivanov']
    pr_nums = ['5379', '5390', '5439', '5586']
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue32690'
    versions = ['Python 3.7']

    @gvanrossum
    Copy link
    Member Author

    @gvanrossum gvanrossum commented Jan 28, 2018

    Found in this thread: https://twitter.com/dabeaz/status/956264950430912512

    Note that at the global level locals() indeed returns variables in order of definition. Ditto at class scope. Because those calls just return the actual dict. But in a function, a dict is constructed. I agree with Dave that it would be nice if the order there was reversed.

    @gvanrossum gvanrossum added 3.7 interpreter-core type-bug labels Jan 28, 2018
    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Jan 28, 2018

    What should happen for:

    def f():
        if random.random() < 0.5:
            a = 1
            b = 2
        else:
            b = 1
            a = 2
        return locals()

    ? Right now co_varnames preserves the order that names were encountered when compiling, so it'd be easy to make 'a' come before 'b' in locals(), so the above function returns either {'a': 1, 'b': 2} or {'a': 2, 'b': 1}.

    If we want to preserve the illusion that local variables are entries in the locals() dict, though, then the dict ought to be either {'a': 1, 'b': 2}, {'b': 1, 'a': 2}. Making this happen would require extra machinery for an extreme edge case so I'm guessing it's not worth it though.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Jan 28, 2018

    The current oddity where the names will always appear in the reverse of declaration order comes from a C level loop in the frame object's "map_to_dict" helper function that loops in reverse [1]:

        ...
        for (j = nmap; --j >= 0; ) {
            ...
        }

    While I haven't tried reversing that yet, I haven't found anything to indicate that it *needs* to be a reverse iteration - it looks like Guido just wrote it that way back in 1994 [2], and everyone that touched the code since then preserved the original iteration order since they didn't have a compelling reason to change it.

    [1] https://github.com/python/cpython/blob/master/Objects/frameobject.c#L794
    [2] 1d5735e

    @gvanrossum
    Copy link
    Member Author

    @gvanrossum gvanrossum commented Jan 28, 2018

    Heh, that's old code! IIRC I wrote a lot of loops like that, it was a little C trick I had picked up that reduced the loop overhead (since comparing to 0 is quicker than comparing to a variable). And until 3.6 it didn't really matter. I very much doubt the loop speed matters. So go ahead and try changing it!

    @ncoghlan ncoghlan self-assigned this Jan 28, 2018
    @dabeaz
    Copy link
    Mannequin

    @dabeaz dabeaz mannequin commented Jan 28, 2018

    Some context: I noticed this while discussing (in a course) a programming trick involving instance initialization and locals() that I'd encountered in the past:

        def _init(locs):
            self = locs.pop('self')
            for name, val in locs.items():
                setattr(self, name, val)
    
        class Spam:
            def __init__(self, a, b, c, d):
                _init(locals())

    In looking at locals(), it was coming back in reverse order of method arguments (d, c, b, a, self). To be honest, it wasn't a critical matter, but more of an odd curiosity in light of recent dictionary ordering.

    I could imagine writing a slightly more general version of _init() that didn't depend on a named 'self' argument if order was preserved:

        def _init(locs):
           items = list(locs.items())
           _, self = items[0]
           for name, val in items[1:]:
               setattr(self, name, val)

    Personally, I don't think the issue Nathaniel brings up is worth worrying about because it would be such a weird edge case on something that is already an edge case. Returning variables in "lexical order"--meaning the order in which first encountered in the source seems pretty sensible to me.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 28, 2018

    New changeset a4d0001 by Raymond Hettinger in branch 'master':
    bpo-32690: Preserve order of locals() (bpo-5379)
    a4d0001

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 28, 2018

    New changeset 9105879 by Raymond Hettinger (Miss Islington (bot)) in branch '3.6':
    bpo-32690: Preserve order of locals() (GH-5379) (bpo-5390)
    9105879

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 28, 2018

    Why did this fix go to 3.6?

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 28, 2018

    Why did this fix go to 3.6?

    The patch is trivially small and fixes an odd an unnecessary implementation quirk. Backporting will help avoid confusing dissimilarity between 3.6 and 3.7. It also makes maintenance easier by keeping the versions in-sync (there are a number of precedents for this).

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 28, 2018

    I don't have a strong opinion on this, but I worry a bit that while the change is trivial, it still might break something (doctests? some obscure code?) in the next 3.6 point release.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 28, 2018

    While I appreciate the desirability of changing the behavior, it does introduce a behavior change in a 3.6.x maintenance release. I don't have a good feel for the probability that current users or 3-party libraries are depending on the old behavior. Anyone want to hazard a guess? If there is a significantly non-zero probability, perhaps this should wait for 3.7.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 28, 2018

    If you guys think the old code is better in some way, feel free to revert the backport :-)

    I'm -0 on undoing the backpport. ISTM that of all of changes to 3.6, this is one of the safest and it has minor benefits of removing a small irritant and keeping the code in-sync. IIRC, 3.6 dict order is only assured for CPython but not for the language spec as a whole, so this is arguably a CPython buglet or least a surprising and unexpected behavior.

    While my instincts say forward-is-better-than-scrambled, I don't care much either way. The only way it affects me is that as an educator, the reversed order is a unnecessary distraction from live demos that show the locals dict.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 28, 2018

    If you guys think the old code is better in some way, feel free to revert the backport :-)

    Hey, it's Guido's code, it must be good :)

    My only concern is that this seems to be more of an issue of unspecified behavior rather than a bug and, as such, our general policy is that the status quo wins as far as changing behavior in a maintenance release. I don't have a really strong feeling one way or another myself; I'm just trying to play devil's advocate on behalf of the user community. Let's see if anyone else can make a case one way or another.

    @gvanrossum
    Copy link
    Member Author

    @gvanrossum gvanrossum commented Jan 28, 2018

    I think consistency between the 3.6.x releases ought to win here.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 30, 2018

    Can someone please revert the 3.6 change, as asked by Guido?

    @vstinner vstinner reopened this Jan 30, 2018
    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 30, 2018

    Ned, can you do the reversion? I'm a little git/github challenged on how to revert this one.

    @rhettinger rhettinger assigned ned-deily and unassigned ncoghlan Jan 30, 2018
    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 30, 2018

    I'll make sure it's taken care of prior to 3.6.5rc1.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Jan 30, 2018

    Noting the git-fu required for non-master reverts:

    • check out the branch of interest and ensure it's up to date
    • git checkout -b bpo-32690-revert-3.6-backport
    • git revert 9105879bfd7133ecbac67f3e9c0bacf6e477de5a
    • edit commit message as appropriate
    • create a PR targeting the branch of interest: #5439

    Devguide issue here: python/devguide#319

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Jan 30, 2018

    New changeset 05f91a4 by Nick Coghlan in branch '3.6':
    [3.6] Revert "bpo-32690: Preserve order of locals() (GH-5379) (bpo-5390)"
    05f91a4

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Jan 30, 2018

    OK, this is back the way it was on the 3.6 branch now, while keeping the change on the 3.7 branch.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 30, 2018

    Thank Nick.

    @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
    3.7 interpreter-core type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants