-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance checker "Dangerous default value" detecting callables #8553
Conversation
Thanks for jumping on this. Just a quick note to say I'm not sure about adding this into |
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. |
Thanks for the feedback:
Agreed. Using a new different check isolates both results (dangerous-default-value is a basic check working since a long time).
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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").
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). |
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. |
Don't forget to update the |
FYI I'm done
|
Closing as the original author indicated they won't work on this. |
Type of Changes
Description
Refs #4659
Closes #4659
Notes: