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-differ and default parameters #1482

Closed
JulienPalard opened this issue May 12, 2017 · 6 comments · Fixed by #3001
Closed

arguments-differ and default parameters #1482

JulienPalard opened this issue May 12, 2017 · 6 comments · Fixed by #3001
Labels
Bug 🪲 Good first issue Friendly and approachable by new contributors

Comments

@JulienPalard
Copy link
Contributor

For methods having hard-to-reach default values, their parameters may be hard to immitate, they sometimes even leak internal details that we do not want to imitate. I'm thinking of collections.OrderedDict.__setitem__ but there may be a others:

def __setitem__(self, key, value, dict_setitem=dict.__setitem__, proxy=_proxy, Link=_Link):

I think having a *args, **kwargs should be a sign of "I'm trying hard to be compatible, without depending on implementation details".

Example:

# Module A

IMPLEMENTATION_DETAIL = object()


class OkestClassname:
    """OKest docstring.
    """
    def method_name(self, arg1, impl_detail=IMPLEMENTATION_DETAIL):
        """OKest docstring.
        """
	return 42


# Module B


class SecondClassname(OkestClassname):
    """OKest docstring.
    """
    def method_name(self, arg1, *args, **kwargs):
        """OKest docstring.
        """
	return 2 ** super().method_name(arg1, *args, **kwargs)

This one (obviously) whines about arguments-differ, yet I'm doing everything to allow the implementation details to change while staying compatible. I'm not even loosing semantical information as I'm only obscursing implementation details.

pylint --version output

pylint 1.7.1, 
astroid 1.5.2
Python 3.5.3 (default, Jan 19 2017, 14:11:04) 
[GCC 6.3.0 20170118]
@rogalski
Copy link
Contributor

Thanks for submitting an issue.

What would you expect to be correct checker behaviour here (in terms of formal definition)?

@JulienPalard
Copy link
Contributor Author

Tried to implement it, may look like:

diff --git a/pylint/checkers/classes.py b/pylint/checkers/classes.py
index 9975220b..d88464a1 100644
--- a/pylint/checkers/classes.py
+++ b/pylint/checkers/classes.py
@@ -1217,6 +1217,12 @@ a metaclass class method.'}
                         decorator.attrname == 'setter'):
                     return
 
+        # Ignore methods with vararg and kwargs, probably just trying
+        # to do their best blindly passing implementation dependent
+        # arguments.
+        if method1.args.vararg and method1.args.kwarg:
+            return
+
         if _different_parameters(
                 refmethod, method1,
                 dummy_parameter_regex=self._dummy_rgx):

A better implementation should probably also check if *args and **kwargs are passed to the reference method (not passing them probably mean not cooperating).

Obviously it broke at least a test, this one:

class SuperClass(object):
    @staticmethod
    def impl(arg1, arg2, **kwargs):
        return arg1 + arg2

class MyClass(SuperClass):
    def impl(self, *args, **kwargs): # [arguments-differ]
        super(MyClass, self).impl(*args, **kwargs)

Which is the exact case I describe as being "probably legitimate". Test come from 3788944, it was the first test of this feature…

I they exists, I'd like to hear arguments against this usage of "blindly passing maybe-changing-implementation-dependent arguments".
I see only one: "*args and **kwargs say nothing about what is expected, so there's a semantical loss", which is wrong as the use of *args and **kwargs are in this case just used for implementation-dependent arguments, used ones are still to be written, typically, inheriting:

def __setitem__(self, key, value, dict_setitem=dict.__setitem__, proxy=_proxy, Link=_Link):

may look like:

def __setitem__(self, key, value, *args, **kwargs):
    # work with key and value, semantically correctly expressed in the signature
    # Cooperate by passing any implementation-dependent argument
    super().__setitem__(key, value, *args, **kwargs)

@AWhetter AWhetter added the Good first issue Friendly and approachable by new contributors label Jul 8, 2017
@AWhetter
Copy link
Contributor

AWhetter commented Jul 8, 2017

For any beginner contributors looking at this: this is almost identical to #1553 so fixing one may fix the other. But this includes an extra test case where there are some named arguments mixed with *args, and **kwargs. You may also want to consider the case where positional and keyword arguments are both used with *args and **kwargs. For example def test(a, b=None, *args, **kwargs).

@benf-wspdigital
Copy link

@AWhetter, the code reference you gave shows that, even if ‘arguments-differ’ is avoided, some override methods would then fail with ‘signature-differs’.

For example:

class LoremIpsum:

    def odio(self, elit):
        print(elit)


class DolorSitAmet(LoremIpsum):

    def odio(self, elit="adipiscing"):
        super().odio(elit)

The override method DolorSitAmet.odio now has a different number of defaults from LoremIpsum.odio, so I guess that this condition would be true and so ‘signature-differs’ is emitted.

To be clear, the above code example is another one that I would not expect ‘arguments-differ’, nor ‘signature-differs’. The signature of DolorSitAmet.odio is expressly compatible with its superclass LoremIpsum.odio, so the LSP is not violated.

@Borda
Copy link

Borda commented Jun 11, 2019

Hello, is it fixed already? I am using py3.6 and pylint 2.2.2 and still getting a very similar error for this example - https://stackoverflow.com/a/54155637/4521646

mattlbeck added a commit to mattlbeck/pylint that referenced this issue Jul 11, 2019
No message is emitted if the overriding function provides positional or
keyword variadics in its signature that can feasibly accept and pass on
all parameters given by the overridden function.

Closes pylint-dev#1482
Closes pylint-dev#1553
@mattlbeck
Copy link
Contributor

PR request #3001 fixes the original problem. The test mentioned by @JulienPalard I have simply changed to expect no messages emitted in that scenario.

The signiture-differs issue demonstrated by @benf-wspdigital is definitely a separate issue to do with adding default params to the overriding function, so that can be fixed in its own ticket (#1556).

Pierre-Sassoulas pushed a commit to Pierre-Sassoulas/pylint that referenced this issue Feb 24, 2020
No message is emitted if the overriding function provides positional or
keyword variadics in its signature that can feasibly accept and pass on
all parameters given by the overridden function.

Closes pylint-dev#1482
Closes pylint-dev#1553
Pierre-Sassoulas pushed a commit to mattlbeck/pylint that referenced this issue Mar 27, 2020
No message is emitted if the overriding function provides positional or
keyword variadics in its signature that can feasibly accept and pass on
all parameters given by the overridden function.

Closes pylint-dev#1482
Closes pylint-dev#1553
Pierre-Sassoulas pushed a commit that referenced this issue Mar 27, 2020
No message is emitted if the overriding function provides positional or
keyword variadics in its signature that can feasibly accept and pass on
all parameters given by the overridden function.

Closes #1482
Closes #1553
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Good first issue Friendly and approachable by new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants