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

gh-96397: Document that keywords in calls need not be identifiers #96393

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

jeff5
Copy link
Contributor

@jeff5 jeff5 commented Aug 29, 2022

This is to clarify that the keys of keyword arguments (necessarily given as
a mapping) are acceptable in a call even if they would not be valid
as the identifiers of formal parameters. It responds to the discussion at:

https://discuss.python.org/t/supporting-or-not-invalid-identifiers-in-kwargs/17147/31

Respectfully note the continuing desire of some participants there there to debate this further.

Also, I may not have done full justice to @ChrisBarker-NOAA's contribution. In particular, I think it is worth documenting, as a further changeset here or on another PR, the apparent acceptability of non-identifier names in the __dict__ of objects, supported by (get|set)attr, which he pointed out.

We clarify that the keys of keyword arguments (necessarily given as
a mapping) are acceptable in a call even if they would not be valid
as the identifiers of formal parameters.
@merwok
Copy link
Member

merwok commented Aug 29, 2022

Let’s create an issue so that it can be referenced properly!

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIth two small wording nits I am in favor of this PR. Given the controversy around it I will request a SC member as a second reviewer.

Doc/reference/expressions.rst Outdated Show resolved Hide resolved
Doc/reference/expressions.rst Outdated Show resolved Hide resolved
@gvanrossum gvanrossum requested a review from Yhg1s August 29, 2022 18:19
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guido's suggestions are the only comments I would have made.

@gvanrossum
Copy link
Member

Let’s create an issue so that it can be referenced properly!

Agreed. @merwok could you create the issue?

@nascheme
Copy link
Member

I agree this is behaviour of CPython that we don't intend to change. I.e. it is not a bug. However, is it a feature of the Python language that these non-identifier keywords are allowed or is it an implementation detail of CPython? The reality seems to be that other Python implementations like pypy and Graal have to do the same thing, in order to be compatible. It seems the trend is to avoid having undefined behaviour in languages. So, maybe it should be defined at the language level.

@merwok merwok changed the title Document that keywords in calls need not be identifiers gh-96397: Document that keywords in calls need not be identifiers Aug 29, 2022
@merwok
Copy link
Member

merwok commented Aug 29, 2022

Issue created: #96397

@merwok merwok linked an issue Aug 29, 2022 that may be closed by this pull request
@ChrisBarker-NOAA
Copy link
Contributor

ChrisBarker-NOAA commented Aug 29, 2022

@nascheme: There was a bunch of discussion about that in discourse, I don't think any consensus was reached.[*]

Once an issue has been created, I guess that's where we could discuss it.

OOPS -- I see it was created now -- I'll go there.

I'd also like to document the same thing with __dict__, setattr(), getattr() (anywhere else?) -- but I just looked through the docs, and I can't see where it would fit -- it's seems it belongs in the Language Reference, but I can't find where to put it -- maybe that can be discussed in the issue as well.

[*} My $0.02: using non-valid identifiers as attribute names via __dict__, **kwargs, getattr(), setattr() is being done in live production code right now, at least in cPython (https://pypi.org/project/netCDF4/ and I'm sure others). I have no idea what other implementations are doing, but I suspect it's the same, at least with PyPy. Given that, regardless of original intent -- I think it's better to define this as a language feature rather than an implementation detail.

jeff5 and others added 2 commits August 29, 2022 23:45
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do setattr() in the same PR or in a separate PR linked to the same issue. Either way this won't be merged until someone from the SC agrees.

@gvanrossum
Copy link
Member

I would not do __dict__ yet, since that is currently pretty much defined to return a dict, which doesn't easily let us require for string keys (except for the class dict, which is a readonly proxy guarded by the class setattr operation). I'd like this to remain strictly a docs change, not a code change.

@ChrisBarker-NOAA
Copy link
Contributor

ChrisBarker-NOAA commented Aug 30, 2022

I would not do __dict__ yet, since that is currently pretty much defined to return a dict

Interesting -- so you can stuff pretty much whatever want into an object's dict. However, if you do put a non-string key into a dict, it will store it just fine, but not be accessible by getattr(), and only be accessible by directly accessing the dict.

I suppose it's possible that that could be a feature someone would (ab)use, but maybe the docs could advise against it, and/or call it unsupported?

@merwok
Copy link
Member

merwok commented Aug 30, 2022

IMO it’s best for the docs to define the spec precisely, say something about strings that are not identifiers, but not also mention details of non-strings in __dict__ — so document how things work, but not all the ways in which things may not work.

@Gouvernathor
Copy link
Contributor

Wouldn't the safest option in short term be to precisely describe how the implementation works, and to categorize all of it as implementation details ? That way, programmers won't assume something is documented when it's just a behavior happening in their console.
Then, anything can always be decided later to be part of the spec (like dict ordering was) or dropped from the implementation. Is there precedent on this though ?

@gpshead
Copy link
Member

gpshead commented Aug 30, 2022

if you do put a non-string key into a dict, it will store it just fine, but not be accessible by getattr(), and only be accessible by directly accessing the dict.

I suppose it's possible that that could be a feature someone would (ab)use, but maybe the docs could advise against it, and/or call it unsupported?

Sometimes documenting something that can be abused even if just to advise against it just leads to more abuse. So I'd lean towards not mentioning that you might be able to get away with stuffing a .__dict__ full of other unexpected key types. (There likely exists code that iterates an object dict and assumes the keys are str that such a thing could trigger an exception out of).


Regardless, for this PR as is, documenting that ** arguments allow any string keys, that seems reasonable to mention. 👍🏾

There are a pile of existing APIs in the world that take arbitrary keyword arguments and depend on this as a convenient feature. Many intentional, some maybe not wholly intentional but users who realize it enjoy the convenience anyways.

@jeff5
Copy link
Contributor Author

jeff5 commented Aug 30, 2022

Wouldn't the safest option in short term be to precisely describe how the implementation works, and to categorize all of it as implementation details ?

@Gouvernathor This would be very much second best and not provide the consistency between implementations we hope for. The Language Reference aspires to document Python the language, and only occasionally (in grey boxes if we remember them) CPython the implementation.

@ChrisBarker-NOAA
Copy link
Contributor

I would not do __dict__ yet, since that is currently pretty much defined to return a dict

Now that I think about it -- it seems it would not be too hard to have a dict that only accepts string keys -- but yes, that's a code change, so a new topic and not part of this PR.

For the Typing folks: are the dunders often typed? that may be a place to define what a __dict__ is, even if not enforced.

@jeff5
Copy link
Contributor Author

jeff5 commented Sep 13, 2022

this won't be merged until someone from the SC agrees.

Am I the one to request that? I would include #96454, but in a way that allowed a separate decision.

Is there a proper place (like the PEP repo) that puts it on the agenda. E-mail to python-dev or SC secretary maybe?

@merwok
Copy link
Member

merwok commented Sep 13, 2022

Requests to the council are sent here: https://github.com/python/steering-council/issues/

@gvanrossum
Copy link
Member

Still waiting for the SC…

@jeff5
Copy link
Contributor Author

jeff5 commented Sep 16, 2022

Still waiting for the SC…

Which I think this is waiting on me to ask it?

I can draft this weekend (if I can slip by the King's forces currently encamped around PyConUK).

@gvanrossum
Copy link
Member

Still waiting for the SC…

Which I think this is waiting on me to ask it?

I can draft this weekend (if I can slip by the King's forces currently encamped around PyConUK).

Thanks!

@jeff5
Copy link
Contributor Author

jeff5 commented Sep 21, 2022

In view of the SC decision, can this (and #96454) now be merged?

@gvanrossum gvanrossum added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes and removed DO-NOT-MERGE labels Sep 22, 2022
@gvanrossum gvanrossum merged commit 9d432b4 into python:main Sep 22, 2022
@miss-islington
Copy link
Contributor

Thanks @jeff5 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-97025 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 22, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 22, 2022
…rs (pythonGH-96393)

This represents the official SC stance, see
https://github.com/python/steering-council/issues/142GH-issuecomment-1252172695
(cherry picked from commit 9d432b4)

Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
@bedevere-bot
Copy link

GH-97026 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 22, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 22, 2022
…rs (pythonGH-96393)

This represents the official SC stance, see
https://github.com/python/steering-council/issues/142GH-issuecomment-1252172695
(cherry picked from commit 9d432b4)

Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
miss-islington added a commit that referenced this pull request Sep 22, 2022
…-96393)

This represents the official SC stance, see
https://github.com/python/steering-council/issues/142GH-issuecomment-1252172695
(cherry picked from commit 9d432b4)

Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
miss-islington added a commit that referenced this pull request Sep 22, 2022
…-96393)

This represents the official SC stance, see
https://github.com/python/steering-council/issues/142GH-issuecomment-1252172695
(cherry picked from commit 9d432b4)

Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
pablogsal pushed a commit that referenced this pull request Oct 22, 2022
…-96393)

This represents the official SC stance, see
https://github.com/python/steering-council/issues/142GH-issuecomment-1252172695
(cherry picked from commit 9d432b4)

Co-authored-by: Jeff Allen <ja.py@farowl.co.uk>
@jeff5 jeff5 deleted the doc-accept-nonid-kwargs branch February 20, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify status of non-identifier unpacked keyword arguments