Refactor various typing related issues #4940

DanielNoord · 2021-08-31T16:33:30Z

Add yourself to CONTRIBUTORS if you are a new contributor.
Write a good description on what the PR does.

Type of Changes

	Type
✓	🔨 Refactoring

Description

This PR has two related aims:

Add typing to all visit_... and leave_... calls in the codebase
Fix some outstanding mypy issues
As a result of doing the former the number of mypy issues increased so it seemed logical to take another look at some of the open issues (based on comments meant to ignore mypy). I did not add a changelog contribution.

Before merging there are a small number of calls for which I did not find documentation and therefore I could not add the type annotations. I think it would be best to add those before merging this, to make sure this PR catches all calls.
The missing annotations are for:
~~visit_exec~~
~~visit_default~~
visit_print
visit_repr
visit_package
visit_instance
~~visit_project~~
Does any of the maintainers know the nodes passed into these calls? And if so, can you give me a list or add them yourself?

This adds typing to most calls that visit nodes. All other changes are due to mypy errors resulting from introduction of typing.

This removes some of the `type: ignore` comments in favour of solving the mypy issues these comments were surpressing.

Pierre-Sassoulas

Amazing work, there are a lot of removed type ignore ! Maybe we can even add the mypy option for unnecessary type ignore now ? Regarding the missing type I need to check on a computer later.

pylint/checkers/typecheck.py

pylint/extensions/broad_try_clause.py

pylint/checkers/stdlib.py

Except for two references to node_classes in the changelog this should be the last of them

DanielNoord · 2021-08-31T19:28:11Z

I have updated the mypy configuration.

Outstanding issues:

visit_print comes from Python3Checker. Is that checker even relevant after having dropped support for Python < 3.6? I saw a number of disabled messages in our own codebase from this checker which currently do nothing.
Are we going to keep the checker in the code? And if so, should we remove disabled messages that do nothing currently?
visit_repr, same as above.
visit_package, docstring refers to astroid.Package node but I don't know what this is.
visit_instance, this is a astroid.objects.ExceptionInstance. However, as astroid.objects is not imported into the main astroid file we can't access it before runtime and are therefore losing out on IntelliSense. Should we update astroid to include this in its main imports and therefore make it accessible to our typing?

Pierre-Sassoulas · 2021-08-31T20:02:33Z

You're right about the python3 checker, I'm going to clean it up, if someone really need to port to python 3 in 2021 that someone can probably suffer to use an older pylint.

cdce8p

Awesome work you already did here @DanielNoord 🚀

A few points:

I don't really like the added pylint: disable=unused-argument comments. We should use _ instead.
I believe that in the strict mode for mypy even unused arguments should be typed. Those are by far most of my comments.
visit_print, visit_repr, and visit_exec should get called anymore. Those could be removed. I haven't looked at Remove the python3 porting mode from the codebase #4942 just yet, but will do so next.
We should probably avoid typing generic list, dict, ... . Better to use List[Any], ... instead.

doc/how_tos/custom_checkers.rst

pylint/checkers/base.py

pylint/extensions/redefined_variable_type.py

pylint/pyreverse/diadefslib.py

pylint/pyreverse/inspector.py

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

for more information, see https://pre-commit.ci

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

for more information, see https://pre-commit.ci

DanielNoord · 2021-08-31T22:41:46Z

Besides open questions from code review, following is needed for this to go forward:
~~1. Merge of #4942~~
2. Answer to: visit_instance, this is a astroid.objects.ExceptionInstance. However, as astroid.objects is not imported into the main astroid file we can't access it before runtime and are therefore losing out on IntelliSense. Should we update astroid to include this in its main imports and therefore make it accessible to our typing?
3. Typing for visit_package in pylint/pyreverse/inspector.py

pylint/pyreverse/inspector.py

coveralls · 2021-09-01T01:04:19Z

Pull Request Test Coverage Report for Build 1193364184

311 of 312 (99.68%) changed or added relevant lines in 40 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 93.098%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/pyreverse/inspector.py	26	27	96.3%

Totals
Change from base Build 1191709213:	0.01%
Covered Lines:	13233
Relevant Lines:	14214

💛 - Coveralls

Pierre-Sassoulas

Amazing work done here. I'm pretty sure being able to use proper typing will prevent crashes in the future.

doc/how_tos/custom_checkers.rst

pylint/checkers/exceptions.py

Pierre-Sassoulas · 2021-09-01T06:59:12Z

pylint/checkers/logging.py

-        if is_method_call(func, types, methods) and not is_complex_format_str(
-            func.bound
+        if (
+            isinstance(func, astroid.BoundMethod)


Is this equivalent to what we had before ? Maybe this prevent crash we never saw yet ? Thank you, mypy ! 😄

Yeah, it is actually redundant and is_method_call makes sure that func is a BoundMethod. However, this doesn't get recognised by mypy or any IDE so we need to add this redundant check here.
Perhaps removing the checks from the return statement in is_method_call and doing if ... return True might also work. That would remove a duplicate check. However, I am not sure.

Pierre-Sassoulas · 2021-09-01T07:08:53Z

Regarding visit_package in pylint/pyreverse/inspector.py, this is never executed in the whole test suite, it miight be safe to remove the function (?)

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

doc/how_tos/custom_checkers.rst

pylint/pyreverse/inspector.py

pylint/checkers/typecheck.py

pylint/checkers/utils.py

DanielNoord · 2021-09-01T14:21:03Z

I have removed visit_package.

Left to-do:

Three open questions which have not been resolved.
Answer to: visit_instance, this is a astroid.objects.ExceptionInstance. However, as astroid.objects is not imported into the main astroid file we can't access it before runtime and are therefore losing out on IntelliSense. Should we update astroid to include this in its main imports and therefore make it accessible to our typing?

pylint/checkers/utils.py

pylint/pyreverse/inspector.py

Resolved

With typing added to parts of the code in ``pylint`` it has become useful to import this class so ``pylint`` can access it. See discussion in: pylint-dev/pylint#4940

DanielNoord · 2021-09-02T07:40:38Z

I have opened pylint-dev/astroid#1160 so that the issue with the typing of visit_instance can be resolved.
After that merge all that's left is the outstanding question of Pierre and two from Marc, although I think those have been answered and can be resolved!
Rest is good to go I think!

…yping

cdce8p · 2021-09-02T12:53:31Z

I did take a look at

# old
ops = list(itertools.chain(*ops))

# new
iter_ops: Iterable[Any] = iter(ops)
ops = list(itertools.chain(*iter_ops))

Tbh I think this might be a mypy issue. We can probably leave the fix as you have it now.

--
Due to the large size of this PR, I was wondering if we should merge it sooner rather than later. Can the visit_instance issue be resolved in a followup as well? @Pierre-Sassoulas What do you think?

DanielNoord · 2021-09-02T15:38:47Z

@Pierre-Sassoulas already approved my PR for astroid so visit_instance has been taken care of. I think we can merge this before astroid has been released as it does not affect functionality.

Pierre-Sassoulas · 2021-09-02T19:34:16Z

Yeah, this is a huge change and need to be merged relatively quickly or we'll get conflict on a lot of other MR. May as well put it in 2.11.0 as I did not finish #4900 and we can't release it yet 😄

DanielNoord · 2021-09-03T10:49:23Z

Is there anything blocking the merge of this PR? Or do you want to wait on the astroid PR?

With typing added to parts of the code in ``pylint`` it has become useful to import this class so ``pylint`` can access it. See discussion in: pylint-dev/pylint#4940

DanielNoord added 2 commits August 31, 2021 18:07

Add type annotations to visit & leave calls

5a1e21d

This adds typing to most calls that visit nodes. All other changes are due to mypy errors resulting from introduction of typing.

Fix outstanding mypy issues

c81f02f

This removes some of the `type: ignore` comments in favour of solving the mypy issues these comments were surpressing.

Pierre-Sassoulas reviewed Aug 31, 2021

View reviewed changes

pylint/checkers/typecheck.py Outdated Show resolved Hide resolved

pylint/extensions/broad_try_clause.py Show resolved Hide resolved

pylint/checkers/stdlib.py Outdated Show resolved Hide resolved

Fix remaining references to node_classes

8ea6f70

Except for two references to node_classes in the changelog this should be the last of them

DanielNoord mentioned this pull request Aug 31, 2021

False positive for super-init-not-called #4941

Closed

Update typing and mypy configuration

7d2062c

Fix cyclic import in deprecated.py

3fe7bc4

cdce8p self-requested a review August 31, 2021 20:47

Pierre-Sassoulas mentioned this pull request Aug 31, 2021

Remove the python3 porting mode from the codebase #4942

Merged

cdce8p previously requested changes Aug 31, 2021

View reviewed changes

DanielNoord and others added 5 commits September 1, 2021 00:00

Apply suggestions from code review

6bbf2b3

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

c004d75

for more information, see https://pre-commit.ci

Apply suggestions from code review

596fc28

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

c45ac73

for more information, see https://pre-commit.ci

Add changes from code review

1f2d7a7

DanielNoord commented Aug 31, 2021

View reviewed changes

pylint/pyreverse/inspector.py Show resolved Hide resolved

cdce8p self-requested a review August 31, 2021 22:49

Additional typing

3c31ed0

Merge branch 'main' of https://github.com/PyCQA/pylint into typing

8dbb8fb

Pierre-Sassoulas reviewed Sep 1, 2021

View reviewed changes

Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 1, 2021

Pierre-Sassoulas added this to the 2.11.x milestone Sep 1, 2021

DanielNoord and others added 2 commits September 1, 2021 09:37

Apply suggestions from code review

8dd90cd

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

Merge branch 'main' into typing

4bf1c59

cdce8p reviewed Sep 1, 2021

View reviewed changes

doc/how_tos/custom_checkers.rst Show resolved Hide resolved

cdce8p reviewed Sep 1, 2021

View reviewed changes

doc/how_tos/custom_checkers.rst Show resolved Hide resolved

pylint/pyreverse/inspector.py Show resolved Hide resolved

pylint/checkers/typecheck.py Show resolved Hide resolved

pylint/checkers/utils.py Outdated Show resolved Hide resolved

Implement code review

3a47b64

Fix test

49aa00c

cdce8p reviewed Sep 1, 2021

View reviewed changes

pylint/checkers/utils.py Show resolved Hide resolved

pylint/checkers/utils.py Show resolved Hide resolved

pylint/pyreverse/inspector.py Outdated Show resolved Hide resolved

Reverse removal of visit_package

ce1aa3d

DanielNoord mentioned this pull request Sep 1, 2021

Remove unused visit_package function #4952

Merged

Merge branch 'main' into typing

c2299df

DanielNoord mentioned this pull request Sep 2, 2021

Add astroid.objects.ExceptionInstance import pylint-dev/astroid#1160

Merged

1 task

DanielNoord added 3 commits September 2, 2021 10:06

Update import of objects.ExceptionInstance

1bfe966

Merge branch 'main' of https://github.com/PyCQA/pylint into typing

4e99175

Merge branch 'typing' of https://github.com/DanielNoord/pylint into t…

7aeb8c4

…yping

cdce8p mentioned this pull request Sep 2, 2021

Typing for ./pylint, batch 1 #4954

Merged

4 tasks

Pierre-Sassoulas modified the milestones: 2.11.x, 2.11.0 Sep 2, 2021

Pierre-Sassoulas merged commit baaa81a into pylint-dev:main Sep 3, 2021

DanielNoord deleted the typing branch September 3, 2021 11:49

DanielNoord mentioned this pull request Sep 20, 2021

Type add_message and add MessageLocationTuple #5050

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor various typing related issues #4940

Refactor various typing related issues #4940

DanielNoord commented Aug 31, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

DanielNoord commented Aug 31, 2021

Pierre-Sassoulas commented Aug 31, 2021

cdce8p left a comment

DanielNoord commented Aug 31, 2021 •

edited

Loading

coveralls commented Sep 1, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

Pierre-Sassoulas Sep 1, 2021

DanielNoord Sep 1, 2021

Pierre-Sassoulas commented Sep 1, 2021

DanielNoord commented Sep 1, 2021 •

edited

Loading

DanielNoord commented Sep 2, 2021

cdce8p commented Sep 2, 2021

DanielNoord commented Sep 2, 2021

Pierre-Sassoulas commented Sep 2, 2021

DanielNoord commented Sep 3, 2021

Refactor various typing related issues #4940

Refactor various typing related issues #4940

Conversation

DanielNoord commented Aug 31, 2021 • edited Loading

Type of Changes

Description

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Aug 31, 2021

Pierre-Sassoulas commented Aug 31, 2021

cdce8p left a comment

Choose a reason for hiding this comment

DanielNoord commented Aug 31, 2021 • edited Loading

coveralls commented Sep 1, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1193364184

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas Sep 1, 2021

Choose a reason for hiding this comment

DanielNoord Sep 1, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas commented Sep 1, 2021

DanielNoord commented Sep 1, 2021 • edited Loading

DanielNoord commented Sep 2, 2021

cdce8p commented Sep 2, 2021

DanielNoord commented Sep 2, 2021

Pierre-Sassoulas commented Sep 2, 2021

DanielNoord commented Sep 3, 2021

DanielNoord commented Aug 31, 2021 •

edited

Loading

DanielNoord commented Aug 31, 2021 •

edited

Loading

coveralls commented Sep 1, 2021 •

edited

Loading

DanielNoord commented Sep 1, 2021 •

edited

Loading