Enhance checker "Dangerous default value" detecting callables

[alblasco]

Type of Changes

	Type
✓	✨ New feature

Description

Refs #4659
Closes #4659

Notes:

• This is my first contribution to pylint, and not familiar with AST so please any feedback is welcome
• My approach is something KISS.
  • Detect any callable
  • excluding a small list of builtins immutables as frozenset

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on django:
The following messages are now emitted:

1. dangerous-default-value:
   Dangerous default value Q() Callable as argument
   https://github.com/django/django/blob/2eb1f37260f0e0b71ef3a77eb5522d2bb68d6489/django/db/models/query_utils.py#L399
2. dangerous-default-value:
   Dangerous default value Value(' ') Callable as argument
   https://github.com/django/django/blob/2eb1f37260f0e0b71ef3a77eb5522d2bb68d6489/django/db/models/functions/text.py#L193
3. dangerous-default-value:
   Dangerous default value Value('') Callable as argument
   https://github.com/django/django/blob/2eb1f37260f0e0b71ef3a77eb5522d2bb68d6489/django/db/models/functions/text.py#L248
4. dangerous-default-value:
   Dangerous default value _('This is not a valid IPv6 address.') Callable as argument
   https://github.com/django/django/blob/2eb1f37260f0e0b71ef3a77eb5522d2bb68d6489/django/utils/ipv6.py#L7
5. dangerous-default-value:
   Dangerous default value gettext_lazy('or') Callable as argument
   https://github.com/django/django/blob/2eb1f37260f0e0b71ef3a77eb5522d2bb68d6489/django/utils/text.py#L254

Effect on music21:
The following messages are now emitted:

1. dangerous-default-value:
   Dangerous default value note.Note('C4') Callable as argument
   https://github.com/cuthbertLab/music21/blob/f05ec0af047977eaa8a2c09f7cf99892a5983eb7/music21/stream/base.py#L5792
2. dangerous-default-value:
   Dangerous default value pitch.Pitch('B5') Callable as argument
   https://github.com/cuthbertLab/music21/blob/f05ec0af047977eaa8a2c09f7cf99892a5983eb7/music21/figuredBass/possibility.py#L203

Effect on pandas:
The following messages are now emitted:

1. dangerous-default-value:
   Dangerous default value np.dtype(np.float64) Callable as argument
   https://github.com/pandas-dev/pandas/blob/8e19396012bee39b15b16efad30f85055f32a10b/pandas/core/nanops.py#L846
2. dangerous-default-value:
   Dangerous default value np.dtype(np.float64) Callable as argument
   https://github.com/pandas-dev/pandas/blob/8e19396012bee39b15b16efad30f85055f32a10b/pandas/core/nanops.py#L1451
3. dangerous-default-value:
   Dangerous default value np.dtype('object') Callable as argument
   https://github.com/pandas-dev/pandas/blob/8e19396012bee39b15b16efad30f85055f32a10b/pandas/core/arrays/categorical.py#L2396
4. dangerous-default-value:
   Dangerous default value _compose(_replace_locals, _replace_booleans, _rewrite_assign, clean_backtick_quoted_toks) Callable as argument
   https://github.com/pandas-dev/pandas/blob/8e19396012bee39b15b16efad30f85055f32a10b/pandas/core/computation/expr.py#L135
5. dangerous-default-value:
   Dangerous default value partial(_preparse, f=_compose(_replace_locals, _replace_booleans, clean_backtick_quoted_toks)) Callable as argument
   https://github.com/pandas-dev/pandas/blob/8e19396012bee39b15b16efad30f85055f32a10b/pandas/core/computation/expr.py#L758
6. dangerous-default-value:
   Dangerous default value DataFrame() Callable as argument
   https://github.com/pandas-dev/pandas/blob/8e19396012bee39b15b16efad30f85055f32a10b/pandas/io/formats/style_render.py#L1995

Effect on psycopg:
The following messages are now emitted:

1. dangerous-default-value:
   Dangerous default value cast(AsyncRowFactory[Row], tuple_row) Callable as argument
   https://github.com/psycopg/psycopg/blob/090ee7ec81cbf7eab9ab8d982e623d9c468c7b6f/psycopg/psycopg/connection_async.py#L57
2. dangerous-default-value:
   Dangerous default value cast(RowFactory[Row], tuple_row) Callable as argument
   https://github.com/psycopg/psycopg/blob/090ee7ec81cbf7eab9ab8d982e623d9c468c7b6f/psycopg/psycopg/connection.py#L663
3. dangerous-default-value:
   Dangerous default value re.compile(b'\\(.)') Callable as argument
   https://github.com/psycopg/psycopg/blob/090ee7ec81cbf7eab9ab8d982e623d9c468c7b6f/psycopg/psycopg/types/array.py#L366

This comment was generated for commit 5146772

[jacobtylerwalls]

Thanks for jumping on this. Just a quick note to say I'm not sure about adding this into dangerous-default-value. I personally wouldn't want to use this check, but I'd want to keep dangerous-default-value. There's nothing dangerous about creating a constant with a callable. It's dangerous if you're creating an instance and manipulating that instance, or if the constant is time-sensitive, but the check is going to flag all kinds of safe cases, like the primer results show.

[Pierre-Sassoulas]

I agree with Jacob here, I think the check needs a list of problematic calls (optimally a configurable list) instead of a list of non problematic calls. The primer's result shows that too imo.

[alblasco]

Thanks for the feedback:

Just a quick note to say I'm not sure about adding this into dangerous-default-value. I personally wouldn't want to use this check, but I'd want to keep dangerous-default-value. There's nothing dangerous about creating a constant with a callable. It's dangerous if you're creating an instance and manipulating that instance, or if the constant is time-sensitive, but the check is going to flag all kinds of safe cases, like the primer results show.

Agreed. Using a new different check isolates both results (dangerous-default-value is a basic check working since a long time).
And we can also disable the new check by default.

I agree with Jacob here, I think the check needs a list of problematic calls (optimally a configurable list) instead of a list of non problematic calls. The primer's result shows that too imo.

Not so sure. In theory any callable is potentially dangerous, so a configurable list seems not enough imo. Even if it's a callable returning a constant, you have to assure that this callable is not calling to any external method to retrieve this constant or that the cyclomatic complexity of that callable is not more than one (i.e. not ifs, for, ... or any loop or conditional programming is used).

I just implemented a kiss approach (every callable is potentially unsafe, except some immutable cases). With previous conditions most callables will be identified as potentially unsafe. So probably too much work for so little benefit.

Any way I am willing to keep contributing. But for any other approach than kiss, I would need additional help about ast, to learn how to dig inside the callable to inspect it. E.g: a commit example in my fork would be very much appreciated.

[Pierre-Sassoulas]

Reading the primer in more details, I'm not entirely sure all of those numerous new messages are false positives... but I think there are indeed false positives between them, because if the left padding in django had a real problem with consequences, it would have been fixed a long time ago. Let me know what you think :)

The pylint team favor false negative over false positives (see our fundamental tenets). I think the proposed check would make sense as an external plugin, but even if we create a new message (Maybe call-as-default-value or dangerous-call-as-default-value ?), we should avoid raising when what is returned from the call is an immutable that still make sense after 10k calls or a year of uptime. Checking that a call return something safe is a hard problem to solve generically and in a reasonable time (both dev and runtime) which is why I was proposing a set of known dangerous calls where a problem can be expected (like the example given in #4659 with "today").

[jacobtylerwalls]

which is why I was proposing a set of known dangerous calls where a problem can be expected (like the example given in #4659 with "today").

Agree that to move this forward we should only check for known dangerous calls that are time-sensitive or expensive to compute or misleading (e.g. random.random(), which will be anything but random).

[jacobtylerwalls]

Re: the case from #8826, we could detect if the callable is a class (and not a frozen dataclass). That's easy to distinguish from a method that returns a constant.

[jacobtylerwalls]

Don't forget to update the dangerous-default-value message definition to avoid referring to "dicts and lists" only.

[alblasco]

FYI I'm done
What I was looking for is already available at https://github.com/PyCQA/flake8-bugbear

B008: Do not perform function calls in argument defaults. The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call. If this is intended, assign the function call to a module-level variable and use that variable as a default value.