Skip to content
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

Report dangerous-default-value for instances of list, dict, set #3183

Closed
scop opened this issue Oct 11, 2019 · 5 comments · Fixed by #3194
Closed

Report dangerous-default-value for instances of list, dict, set #3183

scop opened this issue Oct 11, 2019 · 5 comments · Fixed by #3194
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors

Comments

@scop
Copy link
Contributor

scop commented Oct 11, 2019

It would be useful for dangerous-default-value to warn about more things than just plain list, dict, and set. For example collections.defaultdict is equally dangerous as dict.

Not sure if this should actually report for any values that are instances of list, dict, or set. On the other hand immutable instances of those aren't dangerous, so maybe the best would be if it could be reported just mutable ones.

If the instance of stuff isn't available, I guess extending the hardcoded list to include various known implementations e.g. in the collections module would be an improvement.

@PCManticore PCManticore added Good first issue Friendly and approachable by new contributors Enhancement ✨ Improvement to a component labels Oct 11, 2019
@PCManticore
Copy link
Contributor

Thanks, this sound reasonable.

@scop
Copy link
Contributor Author

scop commented Oct 11, 2019

If you can outline which approach of the suggested ones you'd prefer and give some hints how to implement it (e.g. how to do the needed instanceof-like calls with astroid instances, or other hints whichever way you'd prefer to see it), I could have a crack at it.

@PCManticore
Copy link
Contributor

Hey @scop I think extending the list with a predefined set of mutable collections utilities should suffice for now. Is there something this extension would not include?

Regarding your second question, there are multiple approaches:

  • once an object is inferred, you call .qname() on it if it's an instance of something / function / class, to get its fully qualified name and compare that against a known list
  • you can use astroid.helpers.is_subtype to check if a type is a subtype of another one
  • or you can use pylint.checkers.is_subclass_of to determine if something is a subclass of a different Class node

This is not an exhaustive list, there's probably other ways to check what you need, including type checking against the astroid nodes themselves.

@scop
Copy link
Contributor Author

scop commented Oct 16, 2019

Extending the check with known mutable collections would be an improvement, but it not take unknown mutable collections into account :) I.e. user defined ones.

I believe checking if the value is an instance of collections.abc.MutableSequence, collections.abc.MutableSet, or collections.abc.MutableMapping would implement this best.

@scop
Copy link
Contributor Author

scop commented Oct 16, 2019

Come to think some more of this, it's not just the collections, but any mutable object instance (or an immutable collection that contain one) is dangerous as a default, so this issue is much wider. Anyway I'll submit a PR that adds the mutable collections.*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants