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

Annotate pylint code with type annotations #2079

Closed
PCManticore opened this issue May 12, 2018 · 19 comments · Fixed by #8402
Closed

Annotate pylint code with type annotations #2079

PCManticore opened this issue May 12, 2018 · 19 comments · Fixed by #8402
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors High effort 🏋 Difficult solution or problem to solve Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
Milestone

Comments

@PCManticore
Copy link
Contributor

Since we're on Python 3, we can use annotations for pylint.

@PCManticore PCManticore added Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors labels May 13, 2018
@sushobhit27
Copy link
Contributor

@PCManticore since the person opted to work on this issue has backed off, I would like to work on this issue.

@PCManticore
Copy link
Contributor Author

Hey @sushobhit27 I'm not sure who opted to work on this issue, but feel free to take it! The rough idea of this change would be:

  • first add mypy in tox but not on Travis
  • try to annotate as much pylint code as possible with type annotations
  • if there are any bugs with the types that we pass from one place to another, fix them
  • Once we can safely run mypy on pylint without any errors, integrate mypy on Travis

Feel free to send multiple smaller PRs for this, since it might require quite a few changes. I suggest starting just with the public APIs for now, e.g methods and functions that are public and that we also use internally. Probably we can skip the visit methods for now, because we already know the type, but if it helps, we can add annotations for them if we can find more bugs in the visit methods.

@sushobhit27
Copy link
Contributor

@PCManticore Agree that multiple smaller PRs will be required for this and starting just with the public APIs for now will be good starting point.
Will analyze more and start working on this.

@AWhetter
Copy link
Contributor

AWhetter commented Jun 6, 2018

Before we add too many annotations to pylint, will we get more value out of adding annotations to astroid first? From what I understand of transitioning to a typed codebase, typing modules that are lower in the dependency tree first is a good idea because it makes it easier to correct mistakes later and you have more types available to you in the modules that have the dependency when it comes to typing those.

@PCManticore
Copy link
Contributor Author

Sounds like a good plan to me, @AWhetter !

@PCManticore
Copy link
Contributor Author

#2338 was merged but I'm not sure if we should close this issue now or after we have annotations for all the pylint code. Probably the latter as it's easier to track progress with an opened issue.

@AWhetter
Copy link
Contributor

Sounds good to me. Perhaps we close this once we start getting useful information from mypy, not necessarily when pylint is 100% annotated?

@PCManticore
Copy link
Contributor Author

@AWhetter Definitely. Since that is not measurable enough, I'd say we can close this once we have annotations where it matters. It's important that we now have the setup for running mypy against pylint, the annotations can be its own goal.

@PCManticore PCManticore changed the title Annotate pylint code and integrate mypy Annotate pylint code with type annotations Aug 22, 2018
@sushobhit27
Copy link
Contributor

@PCManticore @AWhetter I will suggest to do in stages, first annotating functions used across pylint code and then covering checker functions.

@Pierre-Sassoulas Pierre-Sassoulas added the Help wanted 🙏 Outside help would be appreciated, good for new contributors label Mar 2, 2021
@Pierre-Sassoulas Pierre-Sassoulas added the High effort 🏋 Difficult solution or problem to solve label Sep 27, 2021
@Pierre-Sassoulas
Copy link
Member

I agree with @sushobhit27:

  • first annotating functions used across pylint code :

    • That would be pylint.reporters, pylint.messages, pylint.lint, pylint.testutil, pylint.checker.base_checker and maybe pylint.config
  • and then covering checker functions:

    • That would be everything in pylint.checkers, and pylint.extensions

@DanielNoord
Copy link
Collaborator

DanielNoord commented Sep 28, 2021

List up to date as of 21/04/2022.

Done:

  • pylint/*.py
  • pylint/config/*.py
  • pylint/lint/*.py
  • pylint/message/*.py
  • pylint/reporters/*.py
  • pylint/testutils/*.py
  • pylint/utils/*.py

--

  • pylint/pyreverse/plantuml_printer.py
  • pylint/pyreverse/printer_factory.py
  • pylint/pyreverse/inspector.py
  • pylint/pyreverse/__init__.py
  • pylint/pyreverse/utils.py
  • pylint/pyreverse/diadefslib.py
  • pylint/pyreverse/vcg_printer.py
  • pylint/pyreverse/writer.py
  • pylint/pyreverse/diagrams.py
  • pylint/pyreverse/main.py
  • pylint/pyreverse/printer.py
  • pylint/pyreverse/dot_printer.py

--

  • pylint/checkers/logging.py
  • pylint/checkers/spelling.py
  • pylint/checkers/misc.py
  • pylint/checkers/typecheck.py
  • pylint/checkers/variables.py
  • pylint/checkers/deprecated.py
  • pylint/checkers/base_checker.py
  • pylint/checkers/__init__.py
  • pylint/checkers/format.py
  • pylint/checkers/mapreduce_checker.py
  • pylint/checkers/imports.py
  • pylint/checkers/utils.py
  • pylint/checkers/raw_metrics.py
  • pylint/checkers/newstyle.py
  • pylint/checkers/exceptions.py
  • pylint/checkers/classes.py
  • pylint/checkers/stdlib.py
  • pylint/checkers/async.py
  • pylint/checkers/refactoring/recommendation_checker.py
  • pylint/checkers/refactoring/__init__.py
  • pylint/checkers/refactoring/refactoring_checker.py
  • pylint/checkers/refactoring/not_checker.py
  • pylint/checkers/refactoring/len_checker.py
  • pylint/checkers/similar.py
  • pylint/checkers/design_analysis.py
  • pylint/checkers/base.py
  • pylint/checkers/strings.py

--

  • pylint/extensions/empty_comment.py
  • pylint/extensions/code_style.py
  • pylint/extensions/check_docs.py
  • pylint/extensions/broad_try_clause.py
  • pylint/extensions/docparams.py
  • pylint/extensions/__init__.py
  • pylint/extensions/mccabe.py
  • pylint/extensions/emptystring.py
  • pylint/extensions/comparetozero.py
  • pylint/extensions/confusing_elif.py
  • pylint/extensions/_check_docs_utils.py
  • pylint/extensions/consider_ternary_expression.py
  • pylint/extensions/while_used.py
  • pylint/extensions/typing.py
  • pylint/extensions/redefined_variable_type.py
  • pylint/extensions/docstyle.py
  • pylint/extensions/check_elif.py
  • pylint/extensions/set_membership.py
  • pylint/extensions/overlapping_exceptions.py
  • pylint/extensions/bad_builtin.py

@DanielNoord
Copy link
Collaborator

Just to give an update here, the typing of most base files is now done. What's left is the typing of all checkers, extensions and pyreverse. Most checkers should be relatively straightforward, but it will still take considerable time and effort!

@jacobtylerwalls jacobtylerwalls moved this from To do to In progress in Pylint 3.0 Jul 3, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jul 8, 2022
@DanielNoord
Copy link
Collaborator

With #7448 we have now turned on mypy strict mode on our codebase 🎉

The only thing left to do is #7115, which can close this issue.
Sadly it is blocked by an astroid issue, but we'll get to that at some point.

Over the coming days/weeks I'll try and see if there is anything in our code that needs to be updated, but for now the typing effort is almost done!

@ollie-iterators
Copy link
Contributor

ollie-iterators commented Mar 3, 2023

pylint/extensions/check_docs.py should be renamed, because there is no "pylint/extensions/check_docs.py
" in the pylint/extensions file.

@ollie-iterators
Copy link
Contributor

I read an article that said that adding types was important for improving performance in python code. It is number 3 in the article: https://aglowiditsolutions.com/blog/python-optimization/

@DudeNr33
Copy link
Collaborator

DudeNr33 commented Mar 6, 2023

I fear that you misinterpreted this paragraph.
Statically typed languages don't have the computational overhead dynamically typed languages like Python do.
Adding type annotations to Python code however does not make this overhead vanish: these type annotations are not evaluated at runtime.
You can try that for yourself by running this example:

a: str = "Hello"
b: str = "World"

a = 1
b = 2

print(a + b)

You will see that although we annotated the types for a and b as str, nothing is stopping us from assigning integers to them.

Type annotations have a lot of benefits, but (at least for pure Python, there may be other tools like numba that might help here) increased runtime performance is not one of them.

@nickdrozd
Copy link
Collaborator

If everything were typed correctly it would be possible to compile with Mypyc to get a substantial speed improvement. Unfortunately Astroid is thoroughly type-incorrect, and getting it right would take some work. That effort is complicated by the fact that the existing type-incorrect behavior is considered part of the guaranteed public API. So there are both technical and political obstacles to getting things right.

@Pierre-Sassoulas
Copy link
Member

the existing type-incorrect behavior is considered part of the guaranteed public API

Huh ? If something is wrongly typed in astroid we should fix the typing, I don't think there's anyone saying the contrary here. But if we want to change the way it's used for example by replacing an Optional[x] type typing to simply x then we need to make some kind of breaking changes and there's going to be a lot of code to modify to actually be able to do that in astroid as well as in pylint.

@nickdrozd
Copy link
Collaborator

Here's an example:

    if isinstance(stmt, nodes.For):
        try:
            inferred_iterable = next(stmt.iter.infer(context=context))
        except (InferenceError, StopIteration):
            yield util.Uninferable
            return

stmt is a For node. stmt.iter.infer is accessed. But the declared type of For.iter is NodeNG | None. That's a type error. Astroid is full of cases like this.

Pylint 3.0 automation moved this from In progress to Done Mar 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0, 3.0.0a6 Mar 9, 2023
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 Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors High effort 🏋 Difficult solution or problem to solve Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
Development

Successfully merging a pull request may close this issue.

9 participants