pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.29k stars 1.13k forks source link

proposal: warn when trying to catch KeyError in defaultdict lookup #8666

Open jgehrcke opened 1 year ago

jgehrcke commented 1 year ago

Current problem

I just debugged a funny situation where I did

    try:
        matching_results = bmrt_cache["by_benchmark_name"][bname]
    except KeyError:
        return f"benchmark name not known: `{bname}`"

In this snippet, bmrt_cache["by_benchmark_name"] is a defaultdict(list). That is, this never actually raises a KeyError, but instead inserts the key. Which was an unintended side effect in this case.

Of course this was a little stupid and such, but yeah that's how bugs happen. Just for completeness: we use mypy and pylint and flake8 on this code base, there was no warning.

Desired solution

If that's not possible to detect with reasonable confidence then that's OK of course, this might be pretty tricky. But I do understand little about the feasibility here, and before dropping the ball prematurely, I thought that it's worth asking:

Can a tool like pylint detect this situation? With certainty? Can we even emit an error in this case?

If not with certainty: maybe we can emit a warning for the except KeyError line, stating that it might never be reachable?

Additional context

No response

Pierre-Sassoulas commented 1 year ago

Thank you for opening the issue, it seems reasonable.

Detecting with certainty or not depend on the scope where the default dict is declared.

This will be uninferable for example:

    def function(bmrt_cache, bname):
        try:
            matching_results = bmrt_cache["by_benchmark_name"][bname]
        except KeyError:
            return f"benchmark name not known: `{bname}`"

But if we have more info we're going to be able to infer the value (modulo conditional and hard to understand code construct, or code that is "too dynamic").

jgehrcke commented 1 year ago

Thank you for the initial feedback on this @Pierre-Sassoulas and a huge thank you for your work on pylint. I am glad I asked!

jgehrcke commented 1 month ago

@Pierre-Sassoulas I wonder if this is worth exploring further. I myself feel rather clueless with respect to contributing here however. Is this easy? :)

Pierre-Sassoulas commented 1 month ago

Modifying inference in astroid is not very easy, I wouldn't start my first contribution with this. But there's some documentation for it here: https://pylint.readthedocs.io/projects/astroid/en/latest/inference.html