python / cpython

The Python programming language
https://www.python.org
Other
63.19k stars 30.26k forks source link

Document that CPython accepts "invalid" identifiers #79286

Closed vstinner closed 2 years ago

vstinner commented 5 years ago
BPO 35105
Nosy @rhettinger, @terryjreedy, @nedbat, @stevendaprano, @cjerdonek, @serhiy-storchaka, @pablogsal, @Windsooon, @tirkarthi, @orlnub123, @akulakov
PRs
  • python/cpython#11263
  • 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: ```python assignee = None closed_at = None created_at = labels = ['3.8', 'docs'] title = 'Document that CPython accepts "invalid" identifiers' updated_at = user = 'https://github.com/vstinner' ``` bugs.python.org fields: ```python activity = actor = 'andrei.avk' assignee = 'docs@python' closed = False closed_date = None closer = None components = ['Documentation'] creation = creator = 'vstinner' dependencies = [] files = [] hgrepos = [] issue_num = 35105 keywords = ['patch'] message_count = 20.0 messages = ['328816', '329034', '329038', '329039', '329040', '329161', '329169', '329208', '329209', '329215', '329289', '329496', '331404', '331405', '332418', '332420', '333033', '372885', '399549', '399566'] nosy_count = 13.0 nosy_names = ['rhettinger', 'terry.reedy', 'roysmith', 'nedbat', 'steven.daprano', 'chris.jerdonek', 'docs@python', 'serhiy.storchaka', 'pablogsal', 'Windson Yang', 'xtreak', 'orlnub123', 'andrei.avk'] pr_nums = ['11263'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue35105' versions = ['Python 3.8'] ```

    vstinner commented 5 years ago

    The Python 3 language has a strict definition for an identifier: https://docs.python.org/dev/reference/lexical_analysis.html#identifiers

    ... but in practice, it's possible to create "invalid" identifiers using setattr().

    Example with PyPy:

    $ pypy
    Python 2.7.13 (0e7ea4fe15e82d5124e805e2e4a37cae1a402d4b, Apr 12 2018, 14:50:12)
    >>>> class A: pass
    >>>> 
    >>>> a=A()
    >>>> setattr(a, "1", 2)
    >>>> getattr(a, "1")
    2
    >>>> vars(a)
    {'1': 2}
    >>>> a.__dict__
    {'1': 2}
    
    >>>> a.1
      File "<stdin>", line 1
        a.1
        ^
    SyntaxError: invalid syntax

    The exact definition of "identifiers" is a common question. Recent examples:

    It would be nice to document the answer. Maybe in the Langage Specification, maybe in the setattr() documentation, maybe in a FAQ, maybe everywhere?

    3507a70f-31cb-4f20-84c0-929d0c39a164 commented 5 years ago

    I'd argue that it's an implementation detail. Documenting it might be nice as some projects such as pytest do use it but I don't think it would make sense in setattr() or getattr() since all they do (at least in this case) is assign/retrieve from the __dict. One thing to note is that __slots doesn't accept them.

    vstinner commented 5 years ago

    I'd argue that it's an implementation detail. Documenting it might be nice as some projects such as pytest do use it but I don't think it would make sense in setattr() or getattr() since all they do (at least in this case) is assign/retrieve from the __dict. One thing to note is that __slots doesn't accept them.

    Maybe it's obvious to you, but the question is asked again and again, at least once per year. So it seems like we need to document it somewhere.

    3db4c488-648f-4df6-97a9-da1ac9fc355c commented 5 years ago

    I agreed we should document it, it' not obvious to me at least.

    3507a70f-31cb-4f20-84c0-929d0c39a164 commented 5 years ago

    The customizing attribute access section of the data model might be a suitable place.

    terryjreedy commented 5 years ago

    It is an implementation detail that some people need to know, and that is very unlikely to change. In the pydev thread, Guido said " My feeling is that limiting it to strings is fine, but checking those strings for resembling identifiers is pointless and wasteful."

    We occasionally document such things in a 'CPython implementation detail' note. I don't know the proper markup for these. At present, I think the note should be in setattr and **kwargs docs.

    vstinner commented 5 years ago

    I don't know the proper markup for these.

    It's ".. impl-detail::". See for example: https://docs.python.org/dev/library/codecs.html#standard-encodings

    nedbat commented 5 years ago

    This seems like a confusion of two things: identifiers are lexical elements of the language. Attributes are not limited to identifiers.

    We could add to the docs for setattr: "The attribute name does not have to be a valid identifier." I don't know what the language guarantees about what strings are valid as attribute names.

    cjerdonek commented 5 years ago

    In the pydev thread, Guido said "My feeling is that limiting it to strings is fine, but checking those strings for resembling identifiers is pointless and wasteful."

    But in a later message, after additional discussion, he acknowledged there could be reasons to change and said, "we needn't rush this."

    So if the docs do describe the current implementation, I think it should warn people that this behavior might not be subject to the same backwards compatibility guarantees as other documented behavior.

    terryjreedy commented 5 years ago

    Documenting something as an 'implementation detail' denies that it is a language feature and does not offer stability guarantees.

    3507a70f-31cb-4f20-84c0-929d0c39a164 commented 5 years ago

    I take back my previous suggestion, I agree that documenting it in setattr() (and **kwargs) is the way to go. It's obvious that you can assign anything to the __dict, since it represents a dict, but setattr() is more ambiguous. 'Anything' was the key word for me here. For example you can assign ints to __dict and it won't complain but try to do the same with setattr()/getattr() and it results in an error.

    3db4c488-648f-4df6-97a9-da1ac9fc355c commented 5 years ago

    I try to create a PR for it. Should we add 'CPython implementation detail' at the document? Because this happens at cpython as well as pypy. BTW, where should we add the document? I have two choices.

    3db4c488-648f-4df6-97a9-da1ac9fc355c commented 5 years ago

    Any ideas? Or I will create a PR in a week without 'CPython implementation detail'

    stevendaprano commented 5 years ago

    Any ideas? Or I will create a PR in a week without 'CPython implementation detail'

    I don't think we want to give any stability guarantees for this. Perhaps we should explicitly state that this is not guaranteed behaviour and may change in the future.

    I would be happy for it to be stated as an CPython implementation detail. If PyPy or any other implementation happen to duplicate it, we're not responsible for documenting that fact.

    Please go ahead and make a PR.

    rhettinger commented 5 years ago

    I don't think we can mark this as an implementation detail for setattr(). The details are downstream and determined by the target object, not by setattr() itself.

    Suggested wording:

    ''' Note, setattr() attempts to update the object with the given attr/value pair. Whether this succeeds and what its affect is is determined by the target object. If an object's class defines __slots__, the attribute may not be writeable. If an object's class defines property with a setter method, the setattr() will trigger the setter method which may or may not actually write the attribute. For objects that have a regular dictionary (which is the typical case), the setattr() call can make any string keyed update allowed by the dictionary including keys that aren't valid identifiers -- for example setattr(a, '1', 'one') will be the equivalent of vars()['1'] ='one'. This issue has little to do with setattr() and is more related to the fact that instance dictionaries can hold any valid key. In a way, it is no different than a user writing a.dict['1'] = 'one'. That has always been allowed and the dict attribute is documented as writeable, so a user is also allowed to write `a.dict = {'1': 'one'}. '''

    In short, we can talk about this in the setattr() docs but it isn't really a setattr() issue. Also, the behavior is effectively guaranteed by the other things users are allowed to do, so there is no merit in marking this as an implementation detail. Non-identifier keys can make it into an instance dictionary via multiple paths that are guaranteed to work.

    rhettinger commented 5 years ago

    FWIW, the only restriction added by setattr() is that *name* must be a string.

    3db4c488-648f-4df6-97a9-da1ac9fc355c commented 5 years ago

    I agreed with @Raymond Hettinger, I will update the PR from your suggestion if no other ideas in next week.

    013be167-c92f-46a8-afa1-7818ebd81a32 commented 4 years ago

    Just as another edge case, type() can do the same thing:

    Foo = type("Foo", (object,), {"a b": 1})
    f = Foo()

    for example, will create a class attribute named "a b". Maybe this actually calls setattr() under the covers, but if it's going to be documented, it should be noted in both places.

    akulakov commented 3 years ago

    It seems like the documentation is lacking and perhaps misleading in regard to attributes.

    Is the reason for allowing that, - only performance (obviously that's a strong enough reason in this case)? Or can this be useful in some other corner cases?

    In addition to just being confusing, I think this can create an impression for users that setattr() allows you to set 'private' or 'hidden' attrs, and setting attrs via __dict__ allows you to set even more 'private', 'top secret' attrs.

    Since attributes are such a core concept in Python, it might be good to have a section that lays out all of these corner cases and reasons for them, so that it can be linked from docs for setattr(), __dict__, dir(), inspect.getmembers(), etc.

    akulakov commented 3 years ago

    In the last message I've said that according to __dict docs, anything in __dict is an attribute of respective obj. That's a bit too-strongly worded, the docs can be understood in the sense that anything that ends up in __dict__ via other mechanisms, such as dotted notation or setattr(), is an attribute.

    Since direct manipulation of __dict is not prohibited, and no limits are set, AFAIK, on keys that can be used for __dict, the more natural reading of the docs is that anything that can be directly set in __dict__ is also an attribute.

    The only thing that would make a user doubt this reading is if he or she finds that getattr() cannot get non-string attrs, and going by its name, user would assume you can get any valid attrs using getattr().

    AlexWaygood commented 2 years ago

    @gvanrossum, it looks like this issue is an older version of

    Do you think there's anything left to do here now that #96393 has been merged?

    gvanrossum commented 2 years ago

    Let's close this issue, it was independently discovered and solved (by updating the docs).