Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

logging-not-lazy and logging-too-many-args not detected #3176

Closed
mhooreman opened this issue Oct 9, 2019 · 4 comments
Closed

logging-not-lazy and logging-too-many-args not detected #3176

mhooreman opened this issue Oct 9, 2019 · 4 comments
Labels
Enhancement ✨ Improvement to a component inference

Comments

@mhooreman
Copy link

mhooreman commented Oct 9, 2019

Hello,

When we use a logger which is cached, the logging-not-lazy and logging-too-many-args issues are not detected.

Steps to reproduce

Run pylint on the following code

"""Demo of non detected logging issues"""

import logging

_loggers = {}


class _Demo:
    @property
    def logger(self):
        """The logger

        If we replace the body of this method by
        "return logging.getLogger(__name__)", the issues are correclty
        detected by pylint
        """
        cls = type(self)
        if cls not in _loggers:
            _loggers[cls] = logging.getLogger(cls)
        return _loggers[cls]

    def __call__(self):
        """Demonstration

        Case 2:
            expected: logging-not-lazy
            got     : no issue

        Case 3:
            expected: logging-too-many-args
            got     : no issue
        """
        self.logger.warning("Case 1 %s", "lorem ipsum")
        self.logger.warning("Case 2 %s" % "lorem ipsum")
        self.logger.warning("Case 3", "lorem ipsum")


if __name__ == "__main__":
    _Demo()

Current behavior

No detected issue

Expected behavior

  • logging-not-lazy on case 2
  • logging-too-many-args on case 3

Notes

  • Correct behavior if we don't use the cache mechanism, e.g. if logger returns immediately logging.getLogger(...)
  • I use this kind of approach because I have several loggers which are created on-the-fly when needed (cls usage is a simplified hook for the example)

pylint --version output

  • pylint 2.4.2
  • astroid 2.3.1
  • Python 3.7.4 (default, Jul 17 2019, 14:07:45)
  • [GCC 7.4.0]
@PCManticore PCManticore added Enhancement ✨ Improvement to a component inference labels Oct 9, 2019
@PCManticore
Copy link
Contributor

I believe there's not much we can do here. The issue is that pylint cannot infer that the code returns a logger given that pylint does not have control flow inference and is not smart enough to figure out the value comes from a mutation of a dictionary that happened earlier.

You can adapt the code to help pylint figure out the return value of logger with something along the lines of:

    @property
    def logger(self):
        """The logger

        If we replace the body of this method by
        "return logging.getLogger(__name__)", the issues are correclty
        detected by pylint
        """
        cls = type(self)
        d = _loggers.get(cls)
        if cls not in _loggers:
            d = logging.getLogger(cls)
            _loggers[cls] = d
        return d

A future alternative would be support for type annotations in pylint, so that your logger property could have a return type that is set to a logger. Unfortunately we won't support annotations any time soon.

Hope this helps.

@FreyGeospatial
Copy link

It's been a few years... any change in status for this?

@Pierre-Sassoulas
Copy link
Member

It's probably still too dynamic for pylint, taking type into consideration is traced in #4813 / #8230

@FreyGeospatial
Copy link

understood... thanks for responding so quickly and for linking this to other issues.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Enhancement ✨ Improvement to a component inference
Projects
None yet
Development

No branches or pull requests

4 participants