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

bpo-36290: Fix kwargs handling in ast node constructors #12382

Merged

Conversation

remilapeyre
Copy link
Contributor

@remilapeyre remilapeyre commented Mar 17, 2019

@matrixise matrixise self-assigned this Sep 13, 2019
@remilapeyre
Copy link
Contributor Author

remilapeyre commented Sep 13, 2019

@matrixise, sorry I missed Zackery comment, I will fix this when I get home.

@matrixise
Copy link
Member

@remilapeyre Nice, I wanted to check all of your PRs.

@remilapeyre
Copy link
Contributor Author

Hi @matrixise, I've made the requested changes and resolved the conflicts, it should be ready for a new review :)

Rémi Lapeyre and others added 4 commits April 8, 2020 14:53
Co-Authored-By: Batuhan Taşkaya <isidentical@gmail.com>
Co-Authored-By: Batuhan Taşkaya <isidentical@gmail.com>
@remilapeyre
Copy link
Contributor Author

Hi @isidentical, thanks for the review, it should be good now.

@remilapeyre remilapeyre requested a review from isidentical April 21, 2020 13:36
Copy link
Member

@isidentical isidentical left a comment

Choose a reason for hiding this comment

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

It looks OK (except a few minor nits) but I wonder why you choose EAFP rather then LBYL for this case. I guess you can just do a contains check, no? and it would look simpler then the current version.By the way thanks for your active work on PR.

@remilapeyre
Copy link
Contributor Author

It looks OK (except a few minor nits) but I wonder why you choose EAFP rather then LBYL for this case. I guess you can just do a contains check, no?

I'm not sure what you mean in this context, no exception is raised so I don't see how I could ask for forgiveness. I could have used a contains check on line 714, is this what you mean? It does look better I think, I can make the change if so.

@isidentical
Copy link
Member

I'm not sure what you mean in this context, no exception is raised so I don't see how I could ask for forgiveness.

Sorry about my conversation skills, I'm still trying to improve them. I mean that instead of that try/except for ValueError that is raised from .index when the value doesn't present, you could just use a contains check which would look more strict then just capturing raw ValueErrors.

def _new(cls, *args, **kwargs):
    for key in kwargs:
        if key not in cls._fields:
            continue # arbitrary keyword arguments are accepted        
        pos = cls._fields.index(key)
        if pos < len(args):
            raise TypeError(f"{cls.__name__} got multiple values "
                            f"for argument {key!r}")
    if cls in _const_types:
        return Constant(*args, **kwargs)
    return Constant.__new__(cls, *args, **kwargs)

Something like this is what I meant (of course also in the C implementation). What do you think about this?

@remilapeyre
Copy link
Contributor Author

Ok, I agree it make the Python code better but for the C version all return codes need to be checked anyway so I'm not sure it's better:

        i = 0;  /* needed by PyDict_Next */
        while (PyDict_Next(kw, &i, &key, &value)) {
            int contains = PySequence_Contains(fields, key);
            if (contains == -1) {
                res = -1;
                goto cleanup;
            } else if (contains == 0) {
                continue;
            }
            Py_ssize_t p = PySequence_Index(fields, key);
            if (p == -1) {
                res = -1;
                goto cleanup;
            }
            else {
                if (p < PyTuple_GET_SIZE(args)) {
                    PyErr_Format(PyExc_TypeError,
                        "%.400s got multiple values for argument '%U'",
                        Py_TYPE(self)->tp_name, key);
                    res = -1;
                    goto cleanup;
                }
            }
            res = PyObject_SetAttr(self, key, value);
            if (res < 0) {
                goto cleanup;
            }
        }

@isidentical
Copy link
Member

I dont think it is good idea to assume ValueError only comes when the item is not present, it might come from something else in the future (unlikely, but why dont we make this more strict if we have a chance).

@remilapeyre
Copy link
Contributor Author

That makes sense, here's a new patch without this issue.

Copy link
Member

@isidentical isidentical left a comment

Choose a reason for hiding this comment

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

Looks great, it might worth to consider to add a case about arbitrary keyword arguments (both pseudo compatibility nodes and real nodes)

@isidentical isidentical added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 3, 2020
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @isidentical for commit 1e1d3b5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 3, 2020
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM

@pablogsal pablogsal merged commit c73914a into python:master May 24, 2020
@miss-islington
Copy link
Contributor

Thanks @remilapeyre for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 24, 2020
…ythonGH-12382)

(cherry picked from commit c73914a)

Co-authored-by: Rémi Lapeyre <remi.lapeyre@lenstra.fr>
@bedevere-bot
Copy link

GH-20365 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 24, 2020
@pablogsal
Copy link
Member

Thanks for the PR @remilapeyre and thanks for the review @isidentical !

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 24, 2020
…ythonGH-12382)

(cherry picked from commit c73914a)

Co-authored-by: Rémi Lapeyre <remi.lapeyre@lenstra.fr>
@bedevere-bot
Copy link

GH-20366 is a backport of this pull request to the 3.8 branch.

@isidentical
Copy link
Member

Thank you for your continous work on this PR @remilapeyre and thanks for the merge @pablogsal 🎊 🥳

miss-islington added a commit that referenced this pull request May 24, 2020
…H-12382)

(cherry picked from commit c73914a)

Co-authored-by: Rémi Lapeyre <remi.lapeyre@lenstra.fr>
miss-islington added a commit that referenced this pull request May 24, 2020
…H-12382)

(cherry picked from commit c73914a)

Co-authored-by: Rémi Lapeyre <remi.lapeyre@lenstra.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants