Declare function mutators with inline comment

[rmorshea]

Preface: There may be a better way to do this, but this is my first contribution and I'm unfamiliar with the code base. I'm mostly hoping this can be a starting point for discussion on how this should actually be achieved. I'll update docs if/when this approach is given the go-ahead.

This addresses this comment requesting a way to mark decorators that mutate function signatures via a comment. This is vital for package authors since, if they create a mutative decorator, all their users would have to manually populate their function_mutators config.

I was able to allow mutative decorators to be marked with inline comments by creating an "error" called decorator-preserves-signature which, when disabled via a comment on a decorator definition, will disable function argument checks on calls to functions that have that decorator. I used an error to do this in order to consume the inline comment state via the linter.file_state._supression_mapping.

So for example, all functions decorated with the following decorator would not have their arguments checked

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

        return None

    return wrapper

@mutative_decorator
def my_func(): pass

my_func(do_something='a-thing')  # OK

Type of Changes

	Type
maybe	🐛 Bug fix
✓	✨ New feature
	🔨 Refactoring
	📜 Docs

Description

Refs #2926 (comment)
Maybe closes this issue #5784

[antonymilne]

I am not at all familiar with pylint's source code so can't comment on the implementation but this feature would be extremely helpful I think, so thank you very much @rmorshea for working on it!

Just two questions:

1. Are there other examples in pylint where you can disable warnings at the point of function definition rather than point of call?
2. If this effectively adds a function to signature-mutators, should the name of the "error" decorator-preserves-signature match that somehow? e.g. signature-mutated or signature-unmutated depending on which way round you want to call it.

[rmorshea]

@AntonyMilneQB

1. As far as I'm aware, the indirect effects of disabling this error are unique in PyLint. If it's not, I'd definitely want to copy whatever approach was taken previously.
2. I renamed the error. The comment is now pylint: disable=signature-mutator.

[DanielNoord]

@Pierre-Sassoulas What do you think about this? I don't know if I prefer this approach but also don't have an immediate better solution yet..

[rmorshea]

@DanielNoord, I'm not sure if your hesitant about using errors in this way, or the level of indirection, but given that the source of the problem arises from the decorator definition, rather than from the call-site for functions that use that decorator, I think some level of indirection is required. As for this, admittedly somewhat hacky use of error statuses, I'd love some recommendations on a better way to implement this.

[Archmonger]

All potentially related issues:

• Support for Overload Decorators #7643
• Add support for PEP 681: dataclass_transform #7437
• decorators not processed successfully with alpha_vantage #8303
• False positive 'undefined-variable' with assignment expression in decorator #5735
• Attributes added by a decorator are raised as missing members errors #3957
• When I use the Decorators to change the function's return , pylint doesn't recognize it #3954
• kwargs used by decorator being flagged as unused. #3662
• False negative undefined-variable for decorators in multiple scenario #3641
• False positive for unexpected-keyword-arg for decorators with inner inner function #5784
• Type changes in decorators are not detected #2578
• Decorator false positive: unsubscriptable-object, no-member, unsupported-membership-test, not-an-iterable, too-many-function-args #1694

[rmorshea]

Wow, that's quite a list! Thanks for collecting that.

[rmorshea]

• Support for Overload Decorators #7643 - ✔️ related, but seems like this could be useful patch until a true solution is found, but would require adding the ability to add an ignore comment at the decorator usage rather then decorator definition since overload is built-in.
• Add support for PEP 681: dataclass_transform #7437 - seems unrelated
• decorators not processed successfully with alpha_vantage #8303 - can't tell - too little information in the issue
• False positive 'undefined-variable' with assignment expression in decorator #5735 - seems unrelated
• Attributes added by a decorator are raised as missing members errors #3957 - duplicate of Type changes in decorators are not detected #2578
• When I use the Decorators to change the function's return , pylint doesn't recognize it #3954 - duplicate of Type changes in decorators are not detected #2578
• kwargs used by decorator being flagged as unused. #3662 - unrelated
• False negative undefined-variable for decorators in multiple scenario #3641 - unrelated
• False positive for unexpected-keyword-arg for decorators with inner inner function #5784 - ✔️ related
• Type changes in decorators are not detected #2578 - related, but I don't think this would fix it unless we could tell PyLint to ignore type checks on the return type of decorators marked as signature mutators.
• Decorator false positive: unsubscriptable-object, no-member, unsupported-membership-test, not-an-iterable, too-many-function-args #1694 duplicate of Type changes in decorators are not detected #2578

[Pierre-Sassoulas]

Thank you for the design proposal, this is thought provoking.

[Pierre-Sassoulas]

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.

[rmorshea]

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.

[antonymilne]

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.

[Pierre-Sassoulas]

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).

[Archmonger]

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.

[Pierre-Sassoulas]

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.

[Archmonger]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.46%. Comparing base (f54dfd1) to head (63f0747).
Report is 956 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8332   +/-   ##
=======================================
  Coverage   95.46%   95.46%           
=======================================
  Files         177      177           
  Lines       18703    18708    +5     
=======================================
+ Hits        17855    17860    +5     
  Misses        848      848           

Files with missing lines	Coverage Δ
pylint/checkers/typecheck.py	96.37% <100.00%> (+0.01%)	⬆️

... and 1 file with indirect coverage changes

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 63f0747

[DanielNoord]

I don't think this proposal ever went anywhere. Personally I still don't really like using comments as it quickly allows for more complexity to seep into pylint. I'd suggest closing this as a nice thought provoking experiment but not something we will act on now.

[rmorshea]

Happy to close if maintainers don't see it going anywhere.

It might be better for users to just disable too-many-function-args, unexpected-keyword-arg, and no-value-for-parameter - I'd argue that other tools do a better job of performing those checks and that maintaining a function_mutators list is quite burdensome for users. Give all that, the complexity this adds to PyLint's internals doesn't seem worth it.

[Pierre-Sassoulas]

I think we can close, but we should create a related issue because a lot of info are here only and would be lost otherwise. It feel like pylint need to understand function mutators better (easier said than done) and prefer false negative over false positive as long as it does not understand them.

[Pierre-Sassoulas]

They were two existing "generic" issues about decorators, #3954 and #2578, closing after I added a comment there.