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.

Sign up for GitHub

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

Declare function mutators with inline comment #8332

Closed
wants to merge 3 commits into from
Closed

Declare function mutators with inline comment #8332

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f4b21f
Declare function mutators with inline comment
rmorshea Feb 23, 2023
6af39bb
rename error + better error descriptions
rmorshea Feb 23, 2023
63f0747
Update .gitignore
rmorshea Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,4 @@ build-stamp
.pytest_cache/
.mypy_cache/
.benchmarks/
venv
rmorshea marked this conversation as resolved.
Show resolved Hide resolved
22 changes: 18 additions & 4 deletions pylint/checkers/typecheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,12 @@ def _missing_member_hint(
"Used when a slice step is 0 and the object doesn't implement "
"a custom __getitem__ method.",
),
"E1145": (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"E1145": (
"E1145": (

Should not be an error imo, but I also think it should not be a message either. Or at least some prior design is in order. Isn't the goal to auto-populate the function-mutator option based on comment by the lib authors directly in their code ? I like this the end goal, but this would be something new in pylint, with the expected associated maintenance cost. Maybe a hack like this is using the existing message control framework is good, maybe it would be simpler to suggest to add a value to function-mutator manually in the message ? This is not something that is done often, even less so if we do configuration template, this way it's done once per library and become very reasonable.

Copy link
Author

@rmorshea rmorshea Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should not be an error imo, but I also think it should not be a message either.

Agreed. I just needed a simple way to hook into the inline comment system. It feels like this comment is meant to "mark" decorator definitions as being signature-mutators. Thus, a more ideal comment might read:

def my_decorator(func): # pylint: mark=signature-mutator
    ...

I'm not really sure how to do this though.

Maybe it would be simpler to suggest to add a value to function-mutator manually in the message?

If I understand this correctly you're suggesting that, if PyLint discovers invalid arguments, and the function in question has a decorator, that PyLint report the error, but also recommend adding the decorator to the signature-mutators list manually.

If so, I think this could be problematic. Signature mutating decorators are a fairly advanced concept and users might add decorators to that list, on the recommendation of this message without fully understanding what they're doing just to make the error go away. I foresee bug reports to PyLint from people who have done this and are getting false negatives for invalid function calls.

Further, as a lib author, even with this addition to the error message, I'm still probably going to get bug reports from people complaining about this PyLint false positive because, they either don't understand the suggestion, or because the find it inconvenient. Also, if I ever decide to refactor and change the name of the offending decorator, my users' PyLint configs will need to be updated.

Ultimately, a message would be a mild improvement in some situations and possibly a detriment in others.

Copy link

Choose a reason for hiding this comment

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

I don't have any useful suggestions on implementation (sorry), but just to add a bit of weight to the cause... I'm coming from a completely unrelated project from @rmorshea and was independently looking for a solution to the exact same problem. I agree with everything he's said here. I understand there's no other patterns like this in pylint at the moment, but it would be much, much more convenient if this disable could be somehow specified at point of function definition rather than at point of call. I'm developing a package that defines decorated functions that will unfortunately raise unexpected-keyword-arg when called. At the moment there's a burden on every user that uses pylint and calls my function to update their config; it would be much more maintainable if the burden fell on the package author instead.

Copy link
Member

Choose a reason for hiding this comment

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

Right. though I can see most library maintainers refusing to add it because it's not their job to fix pylint's bad code comprehension (What if all linter needed something like that ? It becomes unreasonable pretty fast, and add a coupling I would not want myself). It then become the same situation than previously for users but we now also have to maintain the more complicated system on top of it. Also wouldn't it raise a useless-suppression in the original lib code if we don't add a new keyword ? I'm not sure of the performance implication of adding a new pylint keyword in the message control (mark or disable-in-calling-code) instead of disable/enable). This is something we need to take into account because this might have a big impact (all comments are parsed).

Copy link

Choose a reason for hiding this comment

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

Perhaps rather than creating a new pattern, should pylint consider disabling this error on all decorated functions?

In theory, this change should only be a temporary until a method is determined on how to properly parse and analyze these functions.

Copy link
Member

Choose a reason for hiding this comment

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

We have function-mutator to handle this. Some functions are just too dynamic and cannot reasonably be parsed but that's not all functions and pylint would be crippled if we just refuse to analyses all decorators. Maybe we could suggest to populate function_mutator only on decorator we're not sure about. Maybe we can add more default values in function-mutator, for well known libs.

Copy link

@Archmonger Archmonger Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Brutal opinion here: If I need to start populating a list of decorated functions within pylintrc, I'd likely remove pylint from my workflow instead.

At least to me, function_mutator is too cumbersome to be worth considering.

"Decorator does not preserve function signature",
"signature-mutator",
"Emitted when a decorator does not preserve the signature "
"of the functions is takes as inputs.",
),
"W1113": (
"Keyword argument before variable positional arguments list "
"in the definition of %s function",
Expand Down Expand Up @@ -1454,10 +1460,18 @@ def visit_call(self, node: nodes.Call) -> None:
return

# Has the function signature changed in ways we cannot reliably detect?
if hasattr(called, "decorators") and decorated_with(
called, self.linter.config.signature_mutators
):
return
if getattr(called, "decorators", None):
if decorated_with(called, self.linter.config.signature_mutators):
return

called_decorator: astroid.NodeNG
for called_decorator in filter(
None, map(safe_infer, called.decorators.nodes)
):
if not self.linter.file_state._module_msgs_state.get("E1145", {}).get(
called_decorator.lineno, True
):
return

num_positional_args = len(call_site.positional_arguments)
keyword_args = list(call_site.keyword_arguments.keys())
Expand Down
17 changes: 17 additions & 0 deletions tests/functional/a/arguments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,17 @@ def wrapper(*args, do_something=True, **kwargs):
return wrapper


def yet_another_mutation_decorator(fun): # pylint: disable=signature-mutator
"""Yet another decorator that changes a function's signature"""
def wrapper(*args, do_something=True, **kwargs):
if do_something:
return fun(*args, **kwargs)

return None

return wrapper


@mutation_decorator
def mutated_function(arg):
return arg
Expand All @@ -250,11 +261,17 @@ def mutated(arg):
return arg


@yet_another_mutation_decorator
def another_mutated_function(arg):
return arg


mutated_function(do_something=False)
mutated_function()

mutated(do_something=True)

another_mutated_function(do_something=False)

def func(one, two, three):
return one + two + three
Expand Down