Implement reusable Method Deprecation checker.

[matusvalo]

This PR is moving deprecated check logic to separate mixin class DeprecatedMixin. This enables plugins to implement their own deprecation checks. The most visible use case can be pylint-django. Here is the example of simple plugin:

"""
Simple DeprecatedChecker marking test.deprecated_func as deprecated.
"""
import astroid

from pylint.checkers import BaseChecker, DeprecatedMixin, utils
from pylint.interfaces import IAstroidChecker


class DeprecatedChecker(BaseChecker, DeprecatedMixin):
    __implements__ = (IAstroidChecker,)
    name = "deprecated"

    msgs = {
        "W1505": (
            "Using deprecated method %s()",
            "deprecated-method",
            "The method is marked as deprecated and will be removed in "
            "a future version of Python. Consider looking for an "
            "alternative in the documentation.",
        )
      }

    @utils.check_messages(
        "deprecated-method",
    )
    def visit_call(self, node):
        """Visit a Call node."""
        try:
            for inferred in node.func.infer():
                self.check_deprecated_method(node, inferred)
        except astroid.InferenceError:
            return

    def deprecated_methods(self):
        return {'test.deprecated_func'}


def register(linter):
    linter.register_checker(DeprecatedChecker(linter))

Current desing has multiple design choices which can be discussed:

1. Implementing Deprecated checker requires writing new checker instead of configuring existing one.
2. Using DeprecatedMixin requires to define deprecated-method msg in Checker class. This can be cumbersome, but it is backwards compatible and it conforms rule that each checker has it's own isolated set of IDs.

Current state of PR is mainly a starting point for final implementation. The following parts needs to be done:

• Implementing/adjusting unittests
• Documentation

Steps

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Description

Type of Changes

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

Related Issue

Closes #4049

[matusvalo]

I am not able to reproduce failing test in CI. Locally, all tests are passing.

[hippo91]

@matusvalo do not care about failing CI. The reason of such failure is my last commit on astroid master branch. I am currently working on a fix.

[Pierre-Sassoulas]

Thank you for the merge request, this seems reasonable. I left a minor comment, but we could merge once astroid is fixed.

[matusvalo]

Thank you @Pierre-Sassoulas for your comments. I will Implement your proposals.

[matusvalo]

A have pushed new version of PR. I have implemented the feedback, cleaned the code (mainly docstrings) and added unittests. In the next step, I would like to add documentation to How To guide:

http://pylint.pycqa.org/en/latest/how_tos/custom_checkers.html

Should it go to this PR or, should I create new PR?

[Pierre-Sassoulas]

Thanks a lot ! Documentation is much needed and very appreciated. Usually I would say to open another MR but the pipeline for next release is getting crowded so we could do with fewer pipelines to run and rebase to do especially for such a clear and straigthforward merge request :)

[matusvalo]

I was thinking about documentation. Where do you think is the best place documenting it? I was thinking about two places:

1. How To guide about writing a checker: http://pylint.pycqa.org/en/latest/how_tos/custom_checkers.html
2. Technical reference about checker: http://pylint.pycqa.org/en/latest/technical_reference/checkers.html

I am not sure whether 1. is the best place since how to should be light. Good place can be 2. but here I would document it together with other classes (and hence it's better in separate MR).

[Pierre-Sassoulas]

Maybe you could add another example next to the example checker in the repository for those wanting more details ?

You can find some simple examples in the distribution (custom.py and custom_raw.py).

=>
You can find some simple examples in the distribution:

• custom.py
• custom_raw.py
• deprecation_checker.py

[matusvalo]

I have implemented the example as discussed. The changes are pushed to MR. Moreover, I have one though about the MR:

The mixin class is raising deprecated-method Message. This is enforcing that the implementing class in any case must define:

"W1505": (
            "Using deprecated method %s()",
            "deprecated-method",
            "The method is marked as deprecated and will be removed in "
            "a future version of Python. Consider looking for an "
            "alternative in the documentation.",
        )

Otherwise following error is raised:

pylint.exceptions.InvalidMessageError: Message symbol 'deprecated-method' cannot be used for 'W0002' and 'W1505' at the same time. If you're creating an 'old_names' use 'old-deprecated-method' as the old symbol.

Hence we should somehow

1. build the message definition to mixin class or
2. provide the way how user can define the message symbol to be raised.

I am not sure what is the best way how to do it.

[Pierre-Sassoulas]

I don't think I have a very elegant way to fix this, the faster would be to rename the msgid/symbol to something like "W1234", "django-deprecated-method". The other one would be to find a way for the MessageStore to know that a message can be shared between Checker as long as the msgid/symbol is the same. (I thought it would be the case ? Do you have the problem if you use the original "W0002" and "deprecated-message" ?) It could be a constant DEPRECATION_MSG then. What do you think ?

[matusvalo]

Yes, the problem is that if deprecated-method/W1505 is already used in stdlib hence one cannot define again deprecated-method with different msgid. Hence in current MR, user is forced to define message with msg symbol deprecated-method and msgid W1505.

I am not sure whether sharing msgid and msg symbol between multiple checker is the right approach - in documentation it is explicitly said that each checker should have it's own set of errors:

The message-id should be a 4-digit number, prefixed with a message category. There are multiple message categories, these being C, W, E, F, R, standing for Convention, Warning, Error, Fatal and Refactoring. The rest of the 5 digits should not conflict with existing checkers and they should be consistent across the checker. For instance, the first two digits should not be different across the checker.

The 15XX errors are already used by stdchecker so using W1505 will violate this rule and (maybe) confuse users.

I was thinking about simple passing of msg symbol as attribute. Does it make sense to let user define its own checker as class attribute:

1. The DeprecatedMixin could look like this:

class DeprecatedMixin(metaclass=abc.ABCMeta):

    # Message Symbol to be issued for deprecated method
    deprecated_method_msg_symbol = None


    @abc.abstractmethod
    def deprecated_methods(self):
        pass

    def check_deprecated_method(self, node, inferred):
        # Code iterating over methods/functions omited

        if any(name in self.deprecated_methods() for name in (qname, func_name)):
            # We are raising message defined in deprecated_method_msg_symbol
            self.add_message(self.deprecated_method_msg_symbol, node=node, args=(func_name,))

2. the plugin would look like this:

import astroid

from pylint.checkers import BaseChecker, DeprecatedMixin, utils
from pylint.interfaces import IAstroidChecker


class DeprecationChecker(BaseChecker, DeprecatedMixin):
    __implements__ = (IAstroidChecker,)
    name = "deprecated"
    msgs = {
        "W0002": (
            "Using deprecated method %s()",
            "mymethod-deprecated-method",
            "The method is marked as deprecated and will be removed in "
            "the future.",
        ),
    }
    deprecated_method_msg_symbol = 'mymethod-deprecated-method'

    def visit_call(self, node):
        try:
            for inferred in node.func.infer():
                # Calling entry point for deprecation check logic.
                self.check_deprecated_method(node, inferred)
        except astroid.InferenceError:
            return

    def deprecated_methods(self):
        return {'mymodule.deprecated_function', 'mymodule.MyClass.deprecated_method'}


def register(linter):
    linter.register_checker(DeprecationChecker(linter))

3. the run of linter as follows:

$ pylint --load-plugins=deprecation_checker mymain.py
************* Module mymain
mymain.py:3:0: W0002: Using deprecated method deprecated_function() (mymethod-deprecated-method)
mymain.py:4:0: W0002: Using deprecated method deprecated_method() (mymethod-deprecated-method)

[Pierre-Sassoulas]

Yeap, it would makes sense with what is said in the documentation. Or we could define the deprecated-method message in DeprecatedMixin (or wherever it makes the most sense) and change the documentation. I think the msgid value should be decoupled from the checker emitting it, so we can refactor checkers as we see fit. Recently a change to disallowed name (foo, bar, etc.) was made very difficult and convoluted because it was embedded in another checker and we lost a possible contributor over it. I mean, this was just a check if a string is in a list of disallowed name, it should not be convoluted. I'd like to be able to create independent checkers class or mixin so the code can stay clean without having to worry about the naming of the msgid apart from the fact that we should not change them overtime without keeping track of the changes with old_names. I refactored the MessageStore so it could take multiple new message for the same old names, so it's still relatively fresh in my mind and I'm not afraid to change it, if necessary. What do you think ?

[matusvalo]

I am not 100% familiar with pylint internals so it is difficult to say for me, but it make sense what you are proposing. Could you please guide what changes needs to be done in this MR in order to finish it?

[Pierre-Sassoulas]

Can you try putting the message for deprecation in DeprecatedMixin, and see what goes wrong, first ? There is probably some change to do in the way the message are registered for it to work, I could find time to do these.

[coveralls]

Coverage increased (+0.02%) to 91.451% when pulling b9ad228 on matusvalo:deprecated_methods into 45c8245 on PyCQA:master.

[matusvalo]

I have put the Message definition to the DeprecationMixin. Implementation was straightforward. There is only one important point to mention: Now the order of base classes is significant - the DeprecationMixin must be before BaseChecker. But this is recommended anyways [1].

[1] https://www.ianlewis.org/en/mixins-and-python

[Pierre-Sassoulas]

Having to inherit in a specific order is not ideal but the code is a lot cleaner than first version and I think pretty elegant now. Also it beats having to touch the internal of pylint to make the feature. It's well documented too and we even made a small optimization... I'm glad it's ready for 2.7 :) !