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

Move message adding functions from MessagesHandlerMixIn #5129

Merged
merged 1 commit into from
Oct 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
222 changes: 218 additions & 4 deletions pylint/lint/pylinter.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,20 @@
import traceback
import warnings
from io import TextIOWrapper
from typing import Iterable, Iterator, List, Optional, Sequence, Union
from typing import Any, Dict, Iterable, Iterator, List, Optional, Sequence, Union

import astroid
from astroid import AstroidError, nodes

from pylint import checkers, config, exceptions, interfaces, reporters
from pylint.constants import MAIN_CHECKER_NAME, MSG_TYPES
from pylint.constants import (
MAIN_CHECKER_NAME,
MSG_STATE_CONFIDENCE,
MSG_STATE_SCOPE_CONFIG,
MSG_STATE_SCOPE_MODULE,
MSG_TYPES,
MSG_TYPES_STATUS,
)
from pylint.lint.expand_modules import expand_modules
from pylint.lint.parallel import check_parallel
from pylint.lint.report_functions import (
Expand All @@ -30,7 +37,12 @@
get_fatal_error_message,
prepare_crash_report,
)
from pylint.message import MessageDefinitionStore, MessagesHandlerMixIn
from pylint.message import (
Message,
MessageDefinition,
MessageDefinitionStore,
MessagesHandlerMixIn,
)
from pylint.reporters.ureports import nodes as report_nodes
from pylint.typing import FileItem, ModuleDescriptionDict
from pylint.utils import ASTWalker, FileState, LinterStats, ModuleStats, utils
Expand All @@ -41,6 +53,11 @@
parse_pragma,
)

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

MANAGER = astroid.MANAGER


Expand Down Expand Up @@ -519,7 +536,11 @@ def __init__(self, options=(), reporter=None, option_groups=(), pylintrc=None):
"disable-msg": self._options_methods["disable"],
"enable-msg": self._options_methods["enable"],
}
MessagesHandlerMixIn.__init__(self)

# Attributes related to message (state) handling
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These come from MessagesHandlerMixIn.init(). They could be their own PR but I thought this fitted.

self.msg_status = 0
self._msgs_state: Dict[str, bool] = {}

reporters.ReportsHandlerMixIn.__init__(self)
super().__init__(
usage=__doc__,
Expand Down Expand Up @@ -1300,3 +1321,196 @@ def _report_evaluation(self):
sect = report_nodes.EvaluationSection(msg)
self.reporter.display_reports(sect)
return note

# Adding (ignored) messages to the Message Reporter
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 couldn't find a better way to indicate a section of functions without running into https://www.flake8rules.com/rules/E266.html

I really dislike how # IAstroidChecker interface ################################################# looks on L1248.

Copy link
Member

Choose a reason for hiding this comment

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

We need separators because we have a badly designed god class with 1500+ lines, I would classify having a nicer looking separator as "polishing a turd". Granted we're probably not re-designing pylinter any time soon but... well I think this separator is good enough.

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 agree, although I don't immediately know how to redesign PyLinter without passing self as a parameter to functions that are now methods. A lot of its methods require access to self...

This separator as in the one I added right now? Or do you want me to add the additional #?

Copy link
Member

Choose a reason for hiding this comment

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

The separator are good enough let's leave them all as is :)


def _get_message_state_scope(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now a private method. The # type: ignore's are necessary because mypy doesn't infer that the return values are constants and complains that the return value should be Optional[int].

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Oct 9, 2021

Choose a reason for hiding this comment

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

It seems we're testing this function in the tests. Generally, I would say that if a function is private you don't need to test it directly and can test it through the public function. Could it be a subclass with message related code instead (MessageHandler ?) ? Then we'd be using that class in Pylinter using composition instead of inheritance and we'd be able to test it without accessing private function (and pylinter would be more manageable).

I tried to do that at some point and there was a problem because some function are called with self being a MessageHandlerMixin at some point and then a Pylinter at another point. Which as you said is bad design, (but also hard to refactor). What do you think ?

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 looked at this but we need self.file_state._module_msgs_state[msgid]: at L1339 here. As well as self.config on L1336. We could pass self as parameter, but I think that would be part of a bigger redesign (see previous comment). For now I think removing MessagesHandlerMixIn is a good first step.

I don't necessarily agree with the private method testing, but I also don't know what is standard for other python projects. I feel like it is always good to test a function directly as you can diagnose potential problems directly. If we test this by testing the functions that call it and those tests fail we will need an additional step to diagnose the problem. Testing private methods directly makes it easier to diagnose regressions.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Oct 10, 2021

Choose a reason for hiding this comment

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

It's not a Python standard, it's a general standard. It means you can modify private function without changing tests or code. Ie implementation details are hidden (for example move from a polar representation to cartesian in a coordinate class without changing the interface). Or if it should be public and his hidden to simplify the class api it forces to put the code in another code unit that is tested itself, smaller and simpler. (I suppose you made the function private to simplify pylinter's api but i'd like to know your rationale if that's not it)

Anyway, seeing the complexity of pylinter, it's not the time to aim for theorical purity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Understood!

Is this good to go then?

self,
msgid: str,
line: Optional[int] = None,
confidence: Optional[interfaces.Confidence] = None,
) -> Optional[Literal[0, 1, 2]]:
"""Returns the scope at which a message was enabled/disabled."""
if confidence is None:
confidence = interfaces.UNDEFINED
if self.config.confidence and confidence.name not in self.config.confidence:
return MSG_STATE_CONFIDENCE # type: ignore # mypy does not infer Literal correctly
try:
if line in self.file_state._module_msgs_state[msgid]:
return MSG_STATE_SCOPE_MODULE # type: ignore
except (KeyError, TypeError):
return MSG_STATE_SCOPE_CONFIG # type: ignore
return None

def _is_one_message_enabled(self, msgid: str, line: Optional[int]) -> bool:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now a private method

"""Checks state of a single message"""
if line is None:
return self._msgs_state.get(msgid, True)
try:
return self.file_state._module_msgs_state[msgid][line]
except KeyError:
# Check if the message's line is after the maximum line existing in ast tree.
# This line won't appear in the ast tree and won't be referred in
# self.file_state._module_msgs_state
# This happens for example with a commented line at the end of a module.
max_line_number = self.file_state.get_effective_max_line_number()
if max_line_number and line > max_line_number:
fallback = True
lines = self.file_state._raw_module_msgs_state.get(msgid, {})

# Doesn't consider scopes, as a disable can be in a different scope
# than that of the current line.
closest_lines = reversed(
[
(message_line, enable)
for message_line, enable in lines.items()
if message_line <= line
]
)
_, fallback_iter = next(closest_lines, (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 changed how this works because mypy was unable to follow what was happening here. The results remains the same.

if fallback_iter is not None:
fallback = fallback_iter

return self._msgs_state.get(msgid, fallback)
return self._msgs_state.get(msgid, True)

def is_message_enabled(
self,
msg_descr: str,
line: Optional[int] = None,
confidence: Optional[interfaces.Confidence] = None,
) -> bool:
"""return true if the message associated to the given message id is
enabled

msgid may be either a numeric or symbolic message id.
"""
if self.config.confidence and confidence:
if confidence.name not in self.config.confidence:
return False
try:
message_definitions = self.msgs_store.get_message_definitions(msg_descr)
msgids = [md.msgid for md in message_definitions]
except exceptions.UnknownMessageError:
# The linter checks for messages that are not registered
# due to version mismatch, just treat them as message IDs
# for now.
msgids = [msg_descr]
for msgid in msgids:
if self._is_one_message_enabled(msgid, line):
return True
return False

def _add_one_message(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now a private method. Note that args remains Any, to be tackled at a different time.

self,
message_definition: MessageDefinition,
line: Optional[int],
node: Optional[nodes.NodeNG],
args: Optional[Any],
confidence: Optional[interfaces.Confidence],
col_offset: Optional[int],
) -> None:
"""After various checks have passed a single Message is
passed to the reporter and added to stats"""
message_definition.check_message_definition(line, node)
if line is None and node is not None:
line = node.fromlineno
if col_offset is None and hasattr(node, "col_offset"):
col_offset = node.col_offset # type: ignore

# should this message be displayed
if not self.is_message_enabled(message_definition.msgid, line, confidence):
self.file_state.handle_ignored_message(
self._get_message_state_scope(
message_definition.msgid, line, confidence
),
message_definition.msgid,
line,
)
return

# update stats
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)
try:
self.stats.by_msg[message_definition.symbol] += 1
except KeyError:
self.stats.by_msg[message_definition.symbol] = 1
# Interpolate arguments into message string
msg = message_definition.msg
if args:
msg %= args
# get module and object
if node is None:
module, obj = self.current_name, ""
abspath = self.current_file
else:
module, obj = utils.get_module_and_frameid(node)
abspath = node.root().file
if abspath is not None:
path = abspath.replace(self.reporter.path_strip_prefix, "", 1)
else:
path = "configuration"
# add the message
self.reporter.handle_message(
Message(
message_definition.msgid,
message_definition.symbol,
(abspath, path, module, obj, line or 1, col_offset or 0), # type: ignore
msg,
confidence,
)
)

def add_message(
self,
msgid: str,
line: Optional[int] = None,
node: Optional[nodes.NodeNG] = None,
args: Optional[Any] = None,
confidence: Optional[interfaces.Confidence] = None,
col_offset: Optional[int] = None,
) -> None:
"""Adds a message given by ID or name.

If provided, the message string is expanded using args.

AST checkers must provide the node argument (but may optionally
provide line if the line number is different), raw and token checkers
must provide the line argument.
"""
if confidence is None:
confidence = interfaces.UNDEFINED
message_definitions = self.msgs_store.get_message_definitions(msgid)
for message_definition in message_definitions:
self._add_one_message(
message_definition, line, node, args, confidence, col_offset
)

def add_ignored_message(
self,
msgid: str,
line: int,
node: Optional[nodes.NodeNG] = None,
confidence: Optional[interfaces.Confidence] = interfaces.UNDEFINED,
) -> None:
"""Prepares a message to be added to the ignored message storage

Some checks return early in special cases and never reach add_message(),
even though they would normally issue a message.
This creates false positives for useless-suppression.
This function avoids this by adding those message to the ignored msgs attribute
"""
message_definitions = self.msgs_store.get_message_definitions(msgid)
for message_definition in message_definitions:
message_definition.check_message_definition(line, node)
self.file_state.handle_ignored_message(
self._get_message_state_scope(
message_definition.msgid, line, confidence
),
message_definition.msgid,
line,
)
Loading