Enhance the protocol checker #3259

craig-sh · 2019-11-18T19:24:13Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

Checks return values of protocol functions to see if they are valid types

TODO

Type of Changes

	Type
✓	✨ New feature
✓	🔨 Refactoring

Related Issue

Closes #560

PCManticore

Thank you for the PR @craig-sh ! This is going in the right direction, but I left a comment with an improvement we can make to type detection.

PCManticore · 2019-11-18T19:31:12Z

pylint/checkers/classes.py

+        if not inferred or inferred is astroid.Uninferable:
+            return
+
+        if not isinstance(inferred, astroid.Instance) or inferred.name != "bool":


You can use a Const node to check if it returns bools or not:

Suggested change

if not isinstance(inferred, astroid.Instance) or inferred.name != "bool":

if not isinstance(inferred, astroid.Const) or not isinstance(inferred.value, bool):

....

Thanks! I'll update my changes!

coveralls · 2019-11-18T19:39:26Z

Coverage decreased (-0.001%) to 89.824% when pulling bd6cd26 on craig-sh:enhance_protocol_checker into 01dfa52 on PyCQA:master.

PCManticore · 2019-11-21T09:00:48Z

Hey @craig-sh Let me know if this is ready for review. 😄 You can also switch it from the draft mode.

PCManticore · 2019-11-21T09:01:16Z

Or are you planning to write all the checks from the TODO? No worries if that's the case, was just wondering.

craig-sh · 2019-11-21T12:15:26Z

Hey, yeah Im just finishing the rest of the checks in the TODO

craig-sh · 2019-11-22T00:23:21Z

I've implemented all the protocol functions mentioned in #560 . Just a couple of notes

I followed the behavior of the existing __len__() non-negative integer checks where a bool is treated as a valid integer. While it's techically correct should this be flagged as a seperate warning?

I was not sure how to go about checking the elements of a tuple returned by a function invoking tuple(...) .

For example this PR will raise an error for def __getnewargs_ex__(self): return ("x", "y") but not for def __getnewargs_ex__(self): return tuple("x", "y").

Is this behavior fine or could you help point me in the right direction to fix it?

Thanks!

PCManticore

Hey @craig-sh This is awesome work, thank you! Left a bunch of comments, overall looks great. Once those are fixed, we can pull this in.

PCManticore · 2019-11-23T09:07:29Z

ChangeLog

@@ -7,6 +7,11 @@ What's New in Pylint 2.5.0?

 Release date: TBA

+* Added errors for some protocol functions (`__hash__`, `__index__`, etc) when


Would be useful to list all the new errors that were added in the description part of this entry.

PCManticore · 2019-11-23T09:09:23Z

doc/whatsnew/2.5.rst

@@ -53,3 +53,6 @@ separated list of regexes, that if a name matches will be always marked as a bla
 * Mutable ``collections.*`` are now flagged as dangerous defaults.

 * Add new --fail-under flag for setting the threshold for the score to fail overall tests. If the score is over the fail-under threshold, pylint will complete SystemExit with value 0 to indicate no errors.
+
+* Add errors for invalid return types from functions ``__index__``, ``__repr__``, ``__str__``, ``__bytes__``,


Let's list the new checks as well. Also it should go under the New Checkers section.

PCManticore · 2019-11-26T08:35:20Z

pylint/checkers/classes.py

+        inferred = _safe_infer_call_result(node, node)
+        # Only want to check types that we are able to infer
+        if inferred and inferred is not astroid.Uninferable:
+            call_map = {


Let's move this dict construction at __init__ level since otherwise we'll create it over and over for every function we visit (which might be a lot for large projects)

PCManticore · 2019-11-26T08:35:47Z

pylint/checkers/classes.py

+
+        inferred = _safe_infer_call_result(node, node)
+        # Only want to check types that we are able to infer
+        if inferred and inferred is not astroid.Uninferable:


The second part is superfluous, astroid.Uninferable is falsey in nature.

PCManticore · 2019-11-26T08:37:49Z

pylint/checkers/classes.py

+        if SpecialMethodsChecker._is_wrapped_type(node, "int"):
+            return True
+
+        if isinstance(node, astroid.Const) and isinstance(node.value, int):


Can simply these three lines to:

Suggested change

if isinstance(node, astroid.Const) and isinstance(node.value, int):

return isinstance(node, astroid.Const) and isinstance(node.value, int)

PCManticore · 2019-11-26T08:39:26Z

pylint/checkers/classes.py

+            "__bool__ does not return bool",
+            "invalid-bool-returned",
+            "Used when a __bool__ method returns something which is not a bool",
+            {},


The fourth dict argument should not be needed, let's try removing it.

craig-sh · 2019-11-27T13:36:14Z

Hey @PCManticore , thanks for the review! I added the fixes you mentioned, please let me know if there is anything else I can change :)

Thanks!

PCManticore · 2019-11-27T14:03:23Z

pylint/checkers/classes.py

+        if SpecialMethodsChecker._is_wrapped_type(node, "str"):
+            return True
+
+        if isinstance(node, astroid.Const) and isinstance(node.value, str):


These can all be changed to return isinstance(....)

PCManticore · 2019-11-27T15:00:24Z

Hey @craig-sh Thank you for sending this PR! 🎉

Add protocol checks for __bool__

43040c5

PCManticore reviewed Nov 18, 2019

View reviewed changes

Code review feedback for __bool__ changes

20e079c

craig-sh added 3 commits November 21, 2019 18:41

Implement rest of protocol checkers

94b2331

Merge branch 'master' into enhance_protocol_checker

0d85a31

Document changes

81d838e

craig-sh marked this pull request as ready for review November 22, 2019 00:28

craig-sh changed the title ~~(WIP) Enhance the protocol checker~~ Enhance the protocol checker Nov 22, 2019

PCManticore approved these changes Nov 26, 2019

View reviewed changes

craig-sh added 2 commits November 27, 2019 08:25

Fixes from code review

b8e2b58

Merge branch 'master' into enhance_protocol_checker

d46d3d3

PCManticore reviewed Nov 27, 2019

View reviewed changes

Simplify return statements

bd6cd26

PCManticore merged commit c632201 into pylint-dev:master Nov 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance the protocol checker #3259

Enhance the protocol checker #3259

craig-sh commented Nov 18, 2019 •

edited

Loading

PCManticore left a comment

PCManticore Nov 18, 2019

craig-sh Nov 18, 2019

coveralls commented Nov 18, 2019 •

edited

Loading

PCManticore commented Nov 21, 2019

PCManticore commented Nov 21, 2019

craig-sh commented Nov 21, 2019

craig-sh commented Nov 22, 2019

PCManticore left a comment

PCManticore Nov 23, 2019

PCManticore Nov 23, 2019

PCManticore Nov 26, 2019

PCManticore Nov 26, 2019

PCManticore Nov 26, 2019

PCManticore Nov 26, 2019

craig-sh commented Nov 27, 2019 •

edited

Loading

PCManticore Nov 27, 2019

PCManticore commented Nov 27, 2019

	if not isinstance(inferred, astroid.Instance) or inferred.name != "bool":
	if not isinstance(inferred, astroid.Const) or not isinstance(inferred.value, bool):
	....

		@@ -7,6 +7,11 @@ What's New in Pylint 2.5.0?

		Release date: TBA

		* Added errors for some protocol functions (`__hash__`, `__index__`, etc) when

	if isinstance(node, astroid.Const) and isinstance(node.value, int):
	return isinstance(node, astroid.Const) and isinstance(node.value, int)

Enhance the protocol checker #3259

Enhance the protocol checker #3259

Conversation

craig-sh commented Nov 18, 2019 • edited Loading

Steps

Description

TODO

Type of Changes

Related Issue

PCManticore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 18, 2019 • edited Loading

PCManticore commented Nov 21, 2019

PCManticore commented Nov 21, 2019

craig-sh commented Nov 21, 2019

craig-sh commented Nov 22, 2019

PCManticore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craig-sh commented Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

PCManticore commented Nov 27, 2019

craig-sh commented Nov 18, 2019 •

edited

Loading

coveralls commented Nov 18, 2019 •

edited

Loading

craig-sh commented Nov 27, 2019 •

edited

Loading