Pytest: Investigate how we can warn of/prevent redefinition of built-ins

[gnikonorov]

Building off of https://github.com/pytest-dev/pytest/pull/7428 we should investigate if there's an elegant way we can detect and prevent redefinition of built-ins. pylint supports this under W0622, but we do not use pylint ( rationale as to why can be found here )

The high level steps of what we'd want to accomplish would then be:

• determine if we want to prevent redefinition of built ins, or stop short of blocking with some sort of warning, log message, etc. ( or if we even want to block this at all )
• determine which tool we could use to accomplish this
• perform a one time sweep to remove any overridden built ins
• hook it into our CI pipeline

[asottile]

flake8-builtins is probably the easiest way to do this

[gnikonorov]

@asottile agreed
Do we have any concerns that it's owned and managed by only one person? ( meaning, any concerns it may end up unmaintained )

[asottile]

we're already using a lot of one-person software so I highly doubt it

[gnikonorov]

I'm +1 for flake8-builtins then

I suggest we run it on ci/cd and also as part of the linting target

[asottile]

no need to do any special setup for it, just add it to the list of flake8 plugins in .pre-commit-config.yaml

[gnikonorov]

Thanks @asottile ~- let me get a PR up for this tonight ( assuming no surprises )~

Wrapping up the PR now. We have a good amount of overridden keywords in the codebase. I should have it up ~tomorrow~ in a few days

[bluetech]

@gnikonorov Can you describe what problem did this cause? I followed the link to #7428 but couldn't quite figure it out.

[gnikonorov]

@bluetech - in my case I just split a function, and it redefined type. When I ran mypy, mypy complained about how I was using type in my new function.

There is no real problem going on with mypy or the code. It is dangerous to override builtins though, and for developer santity/peace of mind we shouldn't do it IMO. I remember it really confused me at first that we were overriding type ( to use this example ) when I first started looking at the file. If it were called ini_type I would have not been confused, since the overriding of type looked intentional when in reality it wasn't

[bluetech]

Ah thanks for explaining. So I think it's a tradeoff. Using the name of a builtin can cause confusion as you describe. But some arguments against it:

• I still do it sometimes because builtins occupy a lot of useful names like id, type, min, max, dir, input, compile and coming up with alternative names can be annoying.
• mypy should catch most misuses.
• Adding a flake8 plugin causes tox -e linting to take a little longer presumably.
• From looking at #7449 it seems like we'd need a lot of noqa's for it.

So overall I am -0.5 on enforcing this, i.e. I prefer the current situation but I won't completely block it if others think it's useful.

(In any case some changes in #7449 do look good like changing object -> obj for example).

[gnikonorov]

I agree it's a trade off, and definitely requires broader buyer than just myself

Some responses to your concerns:

• The names are useful, but overriding them at best is confusing to people unfamiliar with the code and at worst a future bug
• The plugin doesn't slow down the linting target that much, maybe a few seconds
• We'd only need to noqa the existing code, any new code added should never be noqa'ed since you shouldn't override language keywords in 99% of cases. I also want to go and clean those up. They're just riskier changes and could break the API/end users. They at the very least should be tackled in separate PR's

[nicoddemus]

I'm also leaning a bit against it, mostly because of the huge number of noqa's needed even for those cases that IMHO don't cause any problems, such as methods and attribute names.

An argument which I didn't consider but do agree with is that mypy should catch most/all of such errors.

@asottile @RonnyPfannschmidt what are your takes on this?

Also @The-Compiler might chime in, as I take he uses flake8 a lot and might weight in his experience its usefulness.

[asottile]

personally I think mypy is sufficient to catch these flavor of issues -- I didn't consider it before agreeing with the plugin approach but think it's best to leave it alone (or only enable the local variable flavor of check)

[The-Compiler]

Disclaimer: I love linters. I run pylint, flake8 + plugins, mypy and others (like vulture) myself.

From a quick look at #7449, the reason for most (or at least some) of the "noqa" comments is that we have such names exposed via our API. We definitely should not use built-in Python names as argument names, because it e.g. leads to confusing syntax highlighting in editors:

I guess the choice is less clear for methods. I suppose I'm ±0 about disabling A003 (methods/class attributes shadowing built ins) as they are clearly namespaced and it's something Python's API is doing as well (e.g. logging.LogFilter.filter). I suppose that'd eliminate the need for most noqa comments.

[gnikonorov]

It looks like the plugin exposes the following based on the source:

    assign_msg = 'A001 variable "{0}" is shadowing a python builtin'
    argument_msg = 'A002 argument "{0}" is shadowing a python builtin'
    class_attribute_msg = 'A003 class attribute "{0}" is shadowing a python builtin'

I've no qa'ed as follows:

• A001 5 times
• A002 8 times
• A003 29 times

I didn't realize mypy can catch most of these errors. @bluetech is there a setting we have to enable for this?

[The-Compiler]

As far as I'm aware, mypy can catch "hey, you did print = True but now used print("Hello World")". I don't think it can catch "hey, your argument happens to be named the same as a built-in, this is probably a bad idea".

[bluetech]

@The-Compiler is correct, mypy doesn't warn about the shadowing itself, only if you then try to use it as the builtin you'd (probably) get a type error. So it catches the misuses themselves.

[gnikonorov]

Thanks @bluetech and @The-Compiler

[nicoddemus]

So as per our discussion here, what should we do regarding #7449?

I vote 👎 as is, but change my vote to -0.5 if we ignore A003. 😁

[gnikonorov]

I think it would make sense to disable A003

I’ll defer to @bluetech, @The-Compiler, @asottile

[gnikonorov]

What do we think? Do we want to close this, or add in the checks so long as we ignore A003

[The-Compiler]

FWIW I still think it's worthwhile to add the checks and fixes with A003 ignored. That means we'd have 13 "noqa" comment (which seems quite reasonable across the entire codebase) and we'd avoid introducing new arguments in our API which shadow builtins.

[bluetech]

and we'd avoid introducing new arguments in our API which shadow builtins.

I'm not sure about this one either. For example, from argparse's docs:

import argparse

parser = argparse.ArgumentParser(description='Process some integers.')
parser.add_argument('integers', metavar='N', type=int, nargs='+',
                    help='an integer for the accumulator')

I don't consider type and help to be bad API. I think they are the most obvious names for what they do, even though they happen to shadow builtins (which is an internal detail).

[gnikonorov]

I’m in agreement with @The-Compiler but will defer to someone else regarding @bluetech’s comment

[nicoddemus]

FWIW I'm still -0.5, for the reasons mentioned by @bluetech, specially given mypy will catch any errors.

[nicoddemus]

@The-Compiler and @gnikonorov do you still think this is worthwhile? If you don't have a strong conviction I think we can close this then (I count -2.5 from @asottile, @bluetech and myself vs +2 from @gnikonorov and @The-Compiler).

[gnikonorov]

I’d be ok with whatever wins out. I’m +1

[The-Compiler]

I suppose let's close this then.

[nicoddemus]

Thanks everyone.

Sorry @gnikonorov that this didn't pan out in the end, I'm sure it was a lot of work, but thanks anyway! We definitely appreciate the effort.

[gnikonorov]

No worries @nicoddemus! Glad we discussed this and came to a consensus