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

.arguments property ignores keyword-only args, *args, and **kwargs #2213

Closed
jacobtylerwalls opened this issue Jun 18, 2023 · 5 comments · Fixed by #2240 or #2303
Closed

.arguments property ignores keyword-only args, *args, and **kwargs #2213

jacobtylerwalls opened this issue Jun 18, 2023 · 5 comments · Fixed by #2240 or #2303
Labels
breaking-change Bug 🪳 Good first issue Friendly and approachable by new contributors
Milestone

Comments

@jacobtylerwalls
Copy link
Member

>>> from astroid import extract_node
>>> node = extract_node("""def a(*args, b=None, c=None, **kwargs): ...""")
>>> node.args.arguments
[]

Expected to find all the arguments from the function signature.

The wanted data can be found here:

>>> node.args.vararg
'args'
>>> node.args.kwarg
'kwargs'
>>> node.args.kwonlyargs
[<AssignName.b l.1 at 0x1048189b0>, <AssignName.c l.1 at 0x104818830>]

Discussed at pylint-dev/pylint#7577 (comment).

Notice that positional-only args are found for some reason 🤷

@crazybolillo
Copy link
Contributor

Should the definition be changed as well? It states args.arguments returns required arguments, and AFAIK in the example none are required (I can call a without supplying any arguments).

I tried running the following:

>>> node = extract_node("""def a(kiwi, apple, *args, b=None, c=None, **kwargs): ...""")
>>> node.args.arguments
[<AssignName.kiwi l.1 at 0x7f5c55986b90>, <AssignName.apple l.1 at 0x7f5c55985a50>]

And it seems correct to me 🤔

@DanielNoord
Copy link
Collaborator

DanielNoord commented Jun 27, 2023

@cached_property
def arguments(self):
"""Get all the arguments for this node, including positional only and positional and keyword"""
return list(itertools.chain((self.posonlyargs or ()), self.args or ()))

The docstring seems to be correct?

@jacobtylerwalls
Copy link
Member Author

Depends on how you parse the language. "positional and keyword" could describe the argument kiwi and exclude keyword-only arguments.

Essentially, the crux of this is whether we should

  • leave the function as is, and audit everywhere that uses it (given that we keep finding bugs)
  • change the function

@crazybolillo have you happened to sample the places that use this function to be able to offer a view on that? I'd be eager to hear it!

@crazybolillo
Copy link
Contributor

I think I got confused about the documentation 😭. I was reading the docstring for args (node.args.args in the example):

args: list[AssignName] | None
"""The names of the required arguments.

but we are dealing with arguments (node.args.arguments). I will review the code further to see if I can come up with something

@jacobtylerwalls jacobtylerwalls removed this from the 3.0.0a6 milestone Jul 4, 2023
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 6, 2023
Fixes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 6, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 6, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 6, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 7, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
@Pierre-Sassoulas
Copy link
Member

I also misunderstood the documentation or it's wrong and #2240 is the fix.

crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 10, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 11, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 12, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 15, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.

Methods which counted on arguments() not containing vararg and kwonlyargs
have also been modified so they work with this new change.
@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a9 milestone Jul 20, 2023
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 20, 2023
Closes pylint-dev#2213.

Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.

Methods which counted on arguments() not containing vararg and kwonlyargs
have also been modified so they work with this new change.
jacobtylerwalls pushed a commit that referenced this issue Jul 20, 2023
Arguments.arguments() has been modified so that it returns all arguments
as it should (according to its own doc). A test case was also added to
verify this.

Methods which counted on arguments() not containing vararg and kwonlyargs
have also been modified so they work with this new change.

Provide more info on *args and **kwargs: Iif available, these nodes (accessed
through arguments()) now contain lineno and col offset information.

find_argname now works when requesting vararg or kwargs.

Closes #2213.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 22, 2023
Related pylint-dev#2213.

Requesting the default value for a kwonlyarg with no default would fail with
an IndexError, it now correctly raises a NoDefault exception. This issue
arised after changes in PR#2240.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 22, 2023
Refs pylint-dev#2213.

Requesting the default value for a kwonlyarg with no default would fail with
an IndexError, it now correctly raises a NoDefault exception. This issue
arised after changes in PR#2240.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 22, 2023
Refs pylint-dev#2213.

Requesting the default value for a kwonlyarg with no default would fail with
an IndexError, it now correctly raises a NoDefault exception. This issue
arised after changes in PR pylint-dev#2240.
crazybolillo added a commit to crazybolillo/astroid that referenced this issue Jul 22, 2023
Refs pylint-dev#2213.

Requesting the default value for a kwonlyarg with no default would fail with
an IndexError, it now correctly raises a NoDefault exception. This issue
arised after changes in PR pylint-dev#2240.
jacobtylerwalls pushed a commit that referenced this issue Jul 22, 2023
Refs #2213.

Requesting the default value for a kwonlyarg with no default would fail with
an IndexError, it now correctly raises a NoDefault exception. This issue
arose after changes in PR #2240.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Bug 🪳 Good first issue Friendly and approachable by new contributors
Projects
None yet
4 participants