Fix remaining typing issues for `PyLinter` and related functions #5663

DanielNoord · 2022-01-11T08:52:18Z

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

These are the final changes necessary to add typing to the __init__ of PyLinter.
I'll comment on some of the changes as these are a bit all over the place.

DanielNoord · 2022-01-11T08:53:36Z

pylint/constants.py

@@ -24,7 +30,7 @@
 # The line/node distinction does not apply to fatal errors and reports.
 _SCOPE_EXEMPT = "FR"

-MSG_TYPES = {
+MSG_TYPES: Final = {


Although mypy doesn't recognise that because this dict is final its values could be seen as Literal instead of str it does "allow" us to add an ignore later on.
We will be warned if MSG_TYPES is being altered and we can thus be sure that any of the values obtained from it is one of the values already in there.

There isn't really a concept of final dicts that I know of (at this time). However there might be two separate workaround that you could try.

Dict[Literal["I", "C", ...], Literal["info", "convention", ...] That should work but doesn't guarantee a mapping between specific keys and values.

If the mapping is important, I would suggest creating a TypedDict and using that instead of Final for the annotation. That only works for string keys, but in this case it could be exactly what you're looking for. (It might even be worth a suggestion on the mypy / pyright issue tracker?)

class M(TypedDict): I: Literal["info"] C: Literal["converntion"] ...

It could be that you then need a cast for message_definition.msgid[0] though.

@cdce8p Do you know if there is any plans or previous discussion about the concept of final dicts? I saw a similar issue about final frozen sets I think, which would also be beneficial to us.

I can change this to TypedDict, but if there are plans to (at some point) support final dicts I think it makes sense to keep it like this as it "better" represents what this actually is.

@cdce8p Do you know if there is any plans or previous discussion about the concept of final dicts? I saw a similar issue about final frozen sets I think, which would also be beneficial to us.

I can change this to TypedDict, but if there are plans to (at some point) support final dicts I think it makes sense to keep it like this as it "better" represents what this actually is.

I don't know of any plans. If TypedDicts work, they might be the easiest solution here.

DanielNoord · 2022-01-11T08:53:58Z

pylint/lint/pylinter.py

@@ -537,7 +537,7 @@ def make_options() -> Tuple[Tuple[str, OptionDict], ...]:
    def __init__(
        self,
        options=(),
-        reporter=None,
+        reporter: Union[reporters.BaseReporter, reporters.MultiReporter, None] = None,


I wanted to do this in a separate PR but some of the issues fixed here didn't show if I didn't add typing to at least one of these parameters.

DanielNoord · 2022-01-11T08:54:11Z

pylint/lint/pylinter.py

@@ -1496,7 +1496,11 @@ def _add_one_message(
        msg_cat = MSG_TYPES[message_definition.msgid[0]]
        self.msg_status |= MSG_TYPES_STATUS[message_definition.msgid[0]]
        self.stats.increase_single_message_count(msg_cat, 1)
-        self.stats.increase_single_module_message_count(self.current_name, msg_cat, 1)
+        self.stats.increase_single_module_message_count(
+            self.current_name,  # type: ignore[arg-type] # Should be removable after https://github.com/PyCQA/pylint/pull/5580


current_name shouldn't be None...

pylint/lint/pylinter.py

DanielNoord · 2022-01-11T08:55:12Z

pylint/lint/pylinter.py

@@ -1613,6 +1617,9 @@ def _set_one_msg_status(
    ) -> None:
        """Set the status of an individual message"""
        if scope == "module":
+            if not isinstance(line, int):


int is never None if the scope is module. I'm not sure what the best way to do this is. If it is not an int we should indeed raise a ValueError, but in our current code that's impossible...
Perhaps an assert? Let me know!

I think asserts are the canonical way to convey "I'm doing that to appease the mypy god".

Yeah, although both assert and isinstance will actually decrease performance. I wonder if there is a way to do this without impacting performance..

Some typing can be declared beforehand at no cost (I think we used that for the message id store). But it only work when it's actually impossible for line to not be an int. I think the only way is to change the design which might be costly (and/or overkill).

If you know that it's never None, just use cast.
In case it's supposed to always be int, assert isinstance(line, int) is probably the better choice. The assert basically checks that some other code didn't mess up.

if isinstance(...) is more designed to select a code branch in case of true alternatives.

DanielNoord · 2022-01-11T08:55:33Z

pylint/utils/file_state.py

@@ -139,6 +139,8 @@ def handle_ignored_message(
        or globally.
        """
        if state_scope == MSG_STATE_SCOPE_MODULE:
+            if not isinstance(line, int):


Similar problem here: if state_scope == MSG_STATE_SCOPE_MODULE then line is never not an int. How to fix this...?

Isn't it an issue to fix in astroid ?

No this is a pylint issue. If we disable something via pylintrc its disabled on the "package" scope, that's why it doesn't have a line number. If we disable something in a module itself it will always have an assosciated line number

All right, thank you for clearing that up.

Maybe we could create a different structure to handle module and package disable, but I'm not very clear on the state of the code right now. I'll have a look before 2.13.0 is released. (Or I trust you if you find a solution that you think is good enough in the meantime).

Same here. Either cast or possibly assert.

coveralls · 2022-01-11T08:57:59Z

Pull Request Test Coverage Report for Build 1800595762

10 of 10 (100.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 93.932%

Totals
Change from base Build 1791297472:	0.01%
Covered Lines:	14845
Relevant Lines:	15804

💛 - Coveralls

Pierre-Sassoulas

I think we can merge, the isinstance won't be the main contributor to pylint being slow and a refactor of the design would take a long time. We can benefit from the typing before that.

DanielNoord · 2022-02-02T08:01:43Z

Perhaps @cdce8p knows a good trick for this?

cdce8p · 2022-02-02T14:06:20Z

Perhaps @cdce8p knows a good trick for this?

I left some comments. Check the conversations above 🔝

cdce8p · 2022-02-04T18:37:43Z

pylint/constants.py

@@ -24,7 +30,7 @@
 # The line/node distinction does not apply to fatal errors and reports.
 _SCOPE_EXEMPT = "FR"

-MSG_TYPES = {
+MSG_TYPES: Final = {


@cdce8p Do you know if there is any plans or previous discussion about the concept of final dicts? I saw a similar issue about final frozen sets I think, which would also be beneficial to us.

I can change this to TypedDict, but if there are plans to (at some point) support final dicts I think it makes sense to keep it like this as it "better" represents what this actually is.

I don't know of any plans. If TypedDicts work, they might be the easiest solution here.

cdce8p · 2022-02-04T18:39:05Z

pylint/lint/pylinter.py

@@ -1631,6 +1635,8 @@ def _set_one_msg_status(
    ) -> None:
        """Set the status of an individual message"""
        if scope == "module":
+            assert isinstance(line, int)


I was wondering, would it make sense to add a comment along the lines

assert ... # line should always be int inside module scope

cdce8p · 2022-02-04T18:39:15Z

pylint/utils/file_state.py

@@ -139,6 +139,8 @@ def handle_ignored_message(
        or globally.
        """
        if state_scope == MSG_STATE_SCOPE_MODULE:
+            assert isinstance(line, int)


DanielNoord · 2022-02-05T21:19:15Z

TypedDict creates a lot of problems because you can only use the correct keys and its difficult to assert that the first letter of each message id is in the keys of the TypedDict. I have opted to leave it as is.

Fix remaining typing issues for PyLinter and related functions

f82dd22

DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Jan 11, 2022

DanielNoord commented Jan 11, 2022

View reviewed changes

Pierre-Sassoulas added this to the 2.13.0 milestone Jan 11, 2022

Pierre-Sassoulas approved these changes Feb 2, 2022

View reviewed changes

DanielNoord added 2 commits February 3, 2022 21:26

Merge main

8afbb78

Code review

01dcf70

cdce8p reviewed Feb 4, 2022

View reviewed changes

Code review

f5b05e5

Fix?

1d644a1

DanielNoord merged commit 9989444 into pylint-dev:main Feb 8, 2022

DanielNoord deleted the typing-pylinter branch February 8, 2022 21:00

Uh oh!

Fix remaining typing issues for PyLinter and related functions #5663

Fix remaining typing issues for PyLinter and related functions #5663

Uh oh!

Conversation

DanielNoord commented Jan 11, 2022

Type of Changes

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1800595762

💛 - Coveralls

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Feb 2, 2022

Uh oh!

cdce8p commented Feb 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Feb 5, 2022

Uh oh!

Uh oh!

Fix remaining typing issues for `PyLinter` and related functions #5663

Fix remaining typing issues for `PyLinter` and related functions #5663

coveralls commented Jan 11, 2022 •

edited

Loading