Skip to content

Fix remaining typing issues for PyLinter and related functions #5663

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 8, 2022
Merged

Fix remaining typing issues for PyLinter and related functions #5663

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f82dd22
Fix remaining typing issues for ``PyLinter`` and related functions
DanielNoord Jan 11, 2022
8afbb78
Merge ``main``
DanielNoord Feb 3, 2022
01dcf70
Code review
DanielNoord Feb 3, 2022
f5b05e5
Code review
DanielNoord Feb 5, 2022
1d644a1
Fix?
DanielNoord Feb 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pylint/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@

from pylint.__pkginfo__ import __version__

if sys.version_info >= (3, 8):
from typing import Final
else:
from typing_extensions import Final


PY37_PLUS = sys.version_info[:2] >= (3, 7)
PY38_PLUS = sys.version_info[:2] >= (3, 8)
PY39_PLUS = sys.version_info[:2] >= (3, 9)
Expand All @@ -24,7 +30,7 @@
# The line/node distinction does not apply to fatal errors and reports.
_SCOPE_EXEMPT = "FR"

MSG_TYPES = {
MSG_TYPES: Final = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Dict[Literal["I", "C", ...], Literal["info", "convention", ...] That should work but doesn't guarantee a mapping between specific keys and values.
  2. 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"]
    ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

"I": "info",
"C": "convention",
"R": "refactor",
Expand Down
15 changes: 12 additions & 3 deletions pylint/lint/pylinter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

option_groups=(),
pylintrc=None,
):
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current_name shouldn't be None...

msg_cat, # type: ignore[arg-type] # Mypy doesn't see a Final dict as dict of literals
1,
)
try:
self.stats.by_msg[message_definition.symbol] += 1
except KeyError:
Expand Down Expand Up @@ -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):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

raise ValueError

self.file_state.set_msg_status(msg, line, enable)
if not enable and msg.symbol != "locally-disabled":
self.add_message(
Expand Down Expand Up @@ -1641,7 +1648,9 @@ def _get_messages_to_set(
else:
category_id_formatted = category_id
if category_id_formatted is not None:
for _msgid in self.msgs_store._msgs_by_category.get(category_id_formatted):
for _msgid in self.msgs_store._msgs_by_category.get(
category_id_formatted, []
):
message_definitions.extend(
self._get_messages_to_set(_msgid, enable, ignore_unknown)
)
Expand Down
4 changes: 3 additions & 1 deletion pylint/utils/file_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def set_msg_status(self, msg: "MessageDefinition", line: int, status: bool) -> N
self._module_msgs_state[msg.msgid] = {line: status}

def handle_ignored_message(
self, state_scope: Optional[Literal[0, 1, 2]], msgid: str, line: int
self, state_scope: Optional[Literal[0, 1, 2]], msgid: str, line: Optional[int]
) -> None:
"""Report an ignored message.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it an issue to fix in astroid ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Either cast or possibly assert.

raise ValueError
try:
orig_line = self._suppression_mapping[(msgid, line)]
self._ignored_msgs[(msgid, orig_line)].add(line)
Expand Down