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

locals().update doesn't work in Enum body, even though direct assignment to locals() does #78931

Closed
anntzer mannequin opened this issue Sep 20, 2018 · 12 comments
Closed
Assignees
Labels
3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@anntzer
Copy link
Mannequin

anntzer mannequin commented Sep 20, 2018

BPO 34750
Nosy @ethanfurman, @MojoVampire, @mr-nfamous
PRs
  • bpo-34750: [Enum] add _EnumDict.update() support #23725
  • 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/ethanfurman'
    closed_at = <Date 2020-12-10.21:07:59.178>
    created_at = <Date 2018-09-20.12:39:23.864>
    labels = ['type-feature', 'library', '3.10']
    title = "locals().update doesn't work in Enum body, even though direct assignment to locals() does"
    updated_at = <Date 2020-12-10.21:27:37.999>
    user = 'https://github.com/anntzer'

    bugs.python.org fields:

    activity = <Date 2020-12-10.21:27:37.999>
    actor = 'Antony.Lee'
    assignee = 'ethan.furman'
    closed = True
    closed_date = <Date 2020-12-10.21:07:59.178>
    closer = 'ethan.furman'
    components = ['Library (Lib)']
    creation = <Date 2018-09-20.12:39:23.864>
    creator = 'Antony.Lee'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34750
    keywords = ['patch']
    message_count = 12.0
    messages = ['325864', '325881', '325894', '325994', '326001', '326044', '326085', '376814', '376833', '382809', '382845', '382847']
    nosy_count = 3.0
    nosy_names = ['ethan.furman', 'josh.r', 'bup']
    pr_nums = ['23725']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34750'
    versions = ['Python 3.10']

    @anntzer
    Copy link
    Mannequin Author

    anntzer mannequin commented Sep 20, 2018

    A quick check suggests that enum entries can be programmatically created by assigning to locals() in the Enum body:

       class E(Enum): locals()["a"] = 1
       E.a  # -> <E.a: 'a'>

    However, using locals().update(...) doesn't, and silently does the wrong thing:

       class E(Enum): locals().update({"a": "a"})
       E.a  # -> 'a'

    (Yes, in this simple case, I could just use the functional API (E = Enum("E", [("a", "a")])), but the above is simpler if I also want e.g. to define methods for the Enum.

    @anntzer anntzer mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir labels Sep 20, 2018
    @ethanfurman
    Copy link
    Member

    locals() returns the dictionary being used (an _EnumDict) and direct assignment uses the __setitem__ method, which has been overridden -- and it is the only one; so update(), etc., still have normal dict meanings and not Enum ones.

    Next step: compile list of all methods that _EnumDict should override.

    Or just say it's not supported.

    Antony, can you give a more detailed use-case? Meaning an actual example, please.

    @ethanfurman ethanfurman added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Sep 20, 2018
    @ethanfurman ethanfurman self-assigned this Sep 20, 2018
    @anntzer
    Copy link
    Mannequin Author

    anntzer mannequin commented Sep 20, 2018

    I have a personal helper function for writing (Qt) GUIs that generates ComboBoxes from Enums; essentially something like

        class Choices(Enum):
            choice1 = "text for choice 1"
            choice2 = "text for choice 2"
    
        def callback(choice: Choices):
            # do stuff based on choice
            pass
    
        create_combobox(Choices, callback=callback)

    I'm not including the actual code for create_combobox because it's not particularly relevant here.

    So far, if I wanted to add methods to the Choice enum (e.g. for simplifying the code in callback), I could do it directly; if I wanted to dynamically generate the list of choices, I could also do it (by using the functional API). But to do both, it would have been nice to use the locals().update approach in the bug report above. Instead, I need to do something like

        class MethodsMixin(Enum):
            def method(self): ...
    
        ChoicesWithMethods = MethodsMixin("ChoicesWithMethods", [<the choices>])

    (As a side note, I originally thought I could do the inheritance in the opposite direction

        Choices = Enum("Choices", [...])
    
        class ChoicesWithMethods(Choices):
            def method(self): ...

    but that fails because Choices is a final class. It would be nice if it was still possible to inherit from Choices *as long as only methods are added, rather than new members* (I understand why adding new members is problematic). But this is really a side point.)

    Making _EnumDict actually support update() and every other relevant method is actually not particularly difficult: I think you just need to make it inherit from (collections.abc.MutableMapping, dict) (in that order); this is exactly the approach used (since a while ago) by matplotlib's RcParams class (https://github.com/matplotlib/matplotlib/blob/v3.0.0/lib/matplotlib/__init__.py#L783).

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Sep 21, 2018

    The documentation for locals ( https://docs.python.org/3/library/functions.html#locals ) specifically states:

    Note: The contents of this dictionary should not be modified; changes may not affect the values of local and free variables used by the interpreter.

    The docstring for locals is similar, making it clear that any correlation between the returned dict and the state of locals if *either* is subsequently modified is implementation dependent, subject to change without back-compat concerns; even if we made this change, we've given ourselves the freedom to undo it at any time, which makes it useless to anyone who might try to rely on it.

    The fact that even locals()["a"] = 1 happens to work is an implementation detail AFAICT; normally, locals() is and should remain read-only (or at least, modifications don't actually affect the local scope aside from the dict returned by locals()).

    I'm worried that making _EnumDict inherit from collections.abc.MutableMapping in general would slow down Enums (at the very least creation, I'm not clear on whether _EnumDict remains, hidden behind the mappingproxy, for future lookups on the class), since MutableMapping would introduce a Python layer of overhead to most calls.

    I'm also just not inclined to encourage the common assumption that locals() returns a dict where mutating it actually works, since it usually doesn't.

    @anntzer
    Copy link
    Mannequin Author

    anntzer mannequin commented Sep 21, 2018

    encourage the common assumption that locals() returns a dict where mutating it actually works, since it usually doesn't.

    It does at global and class scope, not at function scope.

    FWIW, PEP-558 (admittedly not accepted yet) proposes to modify the documentation for the semantics of locals() at class-scope to match the actual behavior (https://www.python.org/dev/peps/pep-0558/#class-scope):

    At class scope [...] changes to the returned mapping must change the values bound to local variable names in the execution environment.
    

    I'm worried that making _EnumDict inherit from collections.abc.MutableMapping in general would slow down Enums (at the very least creation, I'm not clear on whether _EnumDict remains, hidden behind the mappingproxy, for future lookups on the class), since MutableMapping would introduce a Python layer of overhead to most calls.

    "Normal" calls won't: __setitem__ / __getitem__ stays what they are, they were implemented in Python and stay implemented in Python. Whoever calls update() will go through an extra Python layer, but there's not much you can do about that.

    Alternatively, if you *really* don't want to support the MutableMapping API, at least it should be disabled (e.g. by adding stub implementations of update(), etc. that throw NotImplementedError). Again performance-wise this would only affect those who try to call these methods.

    As a side note, I would be curious to see any realistic code where the performance of enum creation turns out to be critical. I don't think(?) the _EnumDict stays afterwards, at least PEP-558 (which supposedly corresponds to the actual current behavior) also states "The mapping returned by locals() will not be used as the actual class namespace underlying the defined class (the class creation process will copy the contents to a fresh dictionary that is only accessible by going through the class machinery)."

    @mr-nfamous
    Copy link
    Mannequin

    mr-nfamous mannequin commented Sep 21, 2018

    It's working as intended. locals() and vars() simply returns the current frame's f_locals. In functions, modifying this usually accomplishes nothing useful because the code object has OPTIMIZED and NEWLOCALS flags set, meaning local variables are looked up or set via the LOAD_FAST and STORE_FAST opcodes (respectively) which doesn't even look in the f_locals mapping. In this case, vars() and locals() will build a new dict[*] and fill it with the frame's fastlocals and unpack any closure cells into it.

    The code object used for class bodies however is special and actually does use the mapping in f_locals, which for for classes ultimately built by builtins.build_class (aka classes built with a class statement) will be whatever the metaclass's prepare returns, which in the case of enums is an enum._EnumDict instance.

    So that's why metaclasses are so powerful. You don't even need to use a dictionary subclass as the class namespace, since the STORE_NAME opcode will use PyObject_SetItem; however type.__new__ will make you cast it to a dict, and even the dict that is wrapped by a MappingProxy after the class has been created will be a copy anyway.

    So anyway, there's nothing actually wrong with the current behavior. dict.update never calls self.__getitem__, and since _EnumDict.__setitem__ is where all of the magic happens regular dict.update won't trigger it. I agree though that adding an update method would be nice though and can be done in just a few lines of code.

        import enum
        import sys
    
        def local_update(it=(), **kws):
            self = sys._getframe(1).f_locals
            d = dict(it, **kws)
            for k, v in d.items():
                self[k] = v
                
        class MyEnum(enum.Enum):
            local_update(a=1, b=2)
            
        assert MyEnum.a.value == 1

    [*] it doesn't actually build a new one every time but the only practical purpose with the NEWLOCALS code.co_code flag set is for introspection with vars(), locals(), and sys._getframe

    @anntzer
    Copy link
    Mannequin Author

    anntzer mannequin commented Sep 22, 2018

    I agree though that adding an update method would be nice though and can be done in just a few lines of code.

    Again, this can be done just be inheriting the methods from MutableMapping.

    In fact even now one can just write

        class E(Enum): MutableMapping.update(locals(), {"a": 1})

    and this will do the "right" thing but that's hardly an obvious way to do it...

    @ethanfurman
    Copy link
    Member

    Antony,

    My apologies for the delay.

    What I would like to see is a real example of how you would use this new feature if it were implemented. I'm guessing it would look something like:

        class MyEnum(Enum):
    
            locals.update(*some magic here*)
    
            def a_method(self):
                ...

    Am I right? If yes, what does *some magic here* look like? If no, what would your code actually look like?

    @anntzer
    Copy link
    Mannequin Author

    anntzer mannequin commented Sep 13, 2020

    To be honest, I don't really remember what exact use case I had in my mind 2 years ago (as I probably worked around it in one way or another). However, one example that I can think of (and that I have actually implemented before) is auto-conversion of C #defines from a C header file to a Python-level enum (e.g. for semi-automatic generation of a ctypes wrapper):

        # A general header parser (untested, just an example)
        def parse_defines(header_file):
            d = {}
            for line in header_file:
                if line.startswith("#define"):
                    _, k, v = line.split()
                    d[k] = int(v)
            return d
    
        # Now wrapping a specific C library
        foo_defines = parse_defines("foo.h")
    
        class Foo(Enum):
            locals().update({k: v for k, v in foo_defines.items() if k.startswith("FOO_")})
    
            def some_method(self):
                ...
                # e.g. call a C function that takes a FOO_* as parameter.

    Obviously I could always just replace the method by a free function, but that's true for (nearly) all methods. In other words, it seems a bit "unfair" that it is easy to define methods on enums where all options are explicitly listed, but very hard(?) to do so on enums with programatically defined options.

    @ethanfurman ethanfurman added 3.10 only security fixes type-feature A feature request or enhancement and removed 3.8 only security fixes labels Dec 9, 2020
    @ethanfurman
    Copy link
    Member

    Okay, you convinced me. I would ask two things, though:

    • use vars() instead of locals()
    • split the one-liner ;)
        class Foo(Enum):
            vars().update({
                    k: v
                    for k, v in foo_defines.items()
                    if k.startswith('FOO_')
                    })
            def some_method(self):
                # do something

    @ethanfurman
    Copy link
    Member

    New changeset a658287 by Ethan Furman in branch 'master':
    bpo-34750: [Enum] add _EnumDict.update() support (GH-23725)
    a658287

    @anntzer
    Copy link
    Mannequin Author

    anntzer mannequin commented Dec 10, 2020

    Thanks!

    @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.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant