Fix Arguments.arguments so it actually returns all arguments

[crazybolillo]

Closes #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.

Type of Changes

	Type
✓	🐛 Bug fix

[crazybolillo]

Started out writing a test to verify the behavior, apparently I broke everything else with the changes, will look into fixing that.

[crazybolillo]

Not so destructive now, with only 13 tests failing. At least some of them seem to fail because of this:

astroid/astroid/arguments.py

Lines 267 to 288 in a7ab088

	if funcnode.args.vararg == name:
	# It wants all the args that were passed into
	# the call site.
	if self.has_invalid_arguments():
	raise InferenceError(
	"Inference failed to find values for all positional "
	"arguments to {func!r}: {unpacked_args!r} doesn't "
	"correspond to {positional_arguments!r}.",
	positional_arguments=self.positional_arguments,
	unpacked_args=self._unpacked_args,
	call_site=self,
	func=funcnode,
	arg=name,
	context=context,
	)
	args = nodes.Tuple(
	lineno=funcnode.args.lineno,
	col_offset=funcnode.args.col_offset,
	parent=funcnode.args,
	)
	args.postinit(vararg)
	return iter((args,))

Previously the code would get up to that point (if funcnode.args.vararg == name) but with the new behavior for arguments it is exiting here:

astroid/astroid/arguments.py

Line 237 in a7ab088

return self.positional_arguments[argindex].infer(context)

The specific test that goes through that code is:

astroid/tests/test_inference.py

Line 4969 in a7ab088

def test_args(self) -> None:

[DanielNoord]

Seems like we need to change how we determine argindex. Do you need help with that?

[crazybolillo]

Yes, seems like it. I stopped there for the day and haven't had the chance to look into it, but any insights or tips are appreciated. Probably I will be able to work on this tomorrow.

[crazybolillo]

Fixed (I think?) the inference stuff related to arg index and also some other mechanics caused by the original fix, like considering vararg and kwarg for obtaining default values (I excluded them from the computation). Just two tests failing now, light at the end of the tunnel seems closer.

[codecov]

Codecov Report

Merging #2240 (b0d2153) into main (cf8763a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2240      +/-   ##
==========================================
- Coverage   92.78%   92.78%   -0.01%     
==========================================
  Files          94       94              
  Lines       10937    10947      +10     
==========================================
+ Hits        10148    10157       +9     
- Misses        789      790       +1     

Flag	Coverage Δ
linux	92.59% <100.00%> (-0.01%)	⬇️
pypy	90.89% <100.00%> (-0.01%)	⬇️
windows	92.37% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
astroid/nodes/scoped_nodes/scoped_nodes.py	92.48% <ø> (-0.06%)	⬇️
astroid/arguments.py	99.24% <100.00%> (+0.01%)	⬆️
astroid/nodes/node_classes.py	94.80% <100.00%> (-0.02%)	⬇️
astroid/protocols.py	90.21% <100.00%> (+0.02%)	⬆️
astroid/rebuilder.py	98.45% <100.00%> (+<0.01%)	⬆️

[crazybolillo]

All tests working now, so at least we have a functioning implementation. Can you review?

[jacobtylerwalls]

Thanks!

I found a case that breaks and should be added, potentially in a new test method.

setup

from astroid import extract_node
node = extract_node("def fruit(eat=True, *, peel=False, trim=False, **kwargs): ...")

main

>>> node.args.default_value('eat')
<Const.bool l.1 at 0x1054a9730>

pr

>>> node.args.default_value('eat')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/<user>/astroid/astroid/nodes/node_classes.py", line 934, in default_value
    raise NoDefault(func=self.parent, name=argname)
astroid.exceptions.NoDefault: <FunctionDef.fruit l.1 at 0x10f5c2510> has no default for 'eat'.

main

[crazybolillo]

Will first try to improve default_value and then change Name to AssignName so I only break one thing at a time hehe.

[crazybolillo]

I changed the PR a little bit to provide more info on the arguments since before they were just a name wrapped in AssignName, now they have their lineno and col offsets set. What do you think?

[jacobtylerwalls]

Looking good!

[jacobtylerwalls]

Oh, and don't forget to add a changelog.

[crazybolillo]

I made all the suggested changes and added the change log. If you want me to rebase squash this or have any other suggestions let me know 🤝

[jacobtylerwalls]

Thank you for this helpful contribution!

[cdce8p]

This causes a new error with pylint. A code sample is below. The pylint functional test suite does also fail.

class Sensor:
    def __init__(self, *, description):
        self._id = description.key

...
  File "/.../astroid/astroid/decorators.py", line 49, in wrapped
    for res in _func(node, context, **kwargs):
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/.../astroid/astroid/nodes/node_classes.py", line 457, in _infer
    stmts = list(self.assigned_stmts(context=context))
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/.../astroid/astroid/protocols.py", line 406, in _arguments_infer_argname
    yield from self.default_value(name).infer(context)
               ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/.../astroid/astroid/nodes/node_classes.py", line 957, in default_value
    return self.defaults[idx]
           ~~~~~~~~~~~~~^^^^^
IndexError: list index out of range

A more complete example (although not necessary to reproduce the failure)

from dataclasses import dataclass

@dataclass
class EntityDescription:
    key: str

class Sensor:
    def __init__(self, *, description: EntityDescription) -> None:
        self._id = description.key

[jacobtylerwalls]

Yes, I expected changes to be needed in pylint. I'm eager for a new alpha of astroid soon.

[jacobtylerwalls]

But maybe some of the fixes will involve astroid changes?

[jacobtylerwalls]

@crazybolillo Would you like to take the lead on the necessary follow-ups?

[crazybolillo]

@crazybolillo Would you like to take the lead on the necessary follow-ups?

Sure, I will try to reproduce and add it to astroid's test suite.

[jacobtylerwalls]

Thank you!

[crazybolillo]

It seems like this was a scare. I fixed it with the following diff:

diff --git a/astroid/nodes/node_classes.py b/astroid/nodes/node_classes.py
index 014955cf..6e78a7b1 100644
--- a/astroid/nodes/node_classes.py
+++ b/astroid/nodes/node_classes.py
@@ -947,8 +947,11 @@ class Arguments(
         ]
 
         index = _find_arg(argname, self.kwonlyargs)[0]
-        if index is not None and self.kw_defaults[index] is not None:
-            return self.kw_defaults[index]
+        if (index is not None) and (len(self.kw_defaults) > index):
+            if self.kw_defaults[index] is not None:
+                return self.kw_defaults[index]
+            else:
+                raise NoDefault(func=self.parent, name=argname)
 
         index = _find_arg(argname, args)[0]
         if index is not None:

Should I make a new PR and reference the same issue?

[jacobtylerwalls]

Yes, and I'll take a look. No change log will be necessary.