Skip to content

Fix a crash on psycopg2 for elif used #5369

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

Merged
merged 12 commits into from
Nov 24, 2021
Merged
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ Release date: TBA

* Fix ``install graphiz`` message which isn't needed for puml output format.

* Fix a crash in the ``check_elif`` extensions where an undetected if in a comprehension
with an if statement within a f-string resulted in an out of range error. The checker no
longer relies on counting if statements anymore and uses known if statements locations instead.
It should not crash on badly parsed if statements anymore.

* Fix ``simplify-boolean-expression`` when condition can be inferred as False.

Closes #5200
Expand Down
26 changes: 12 additions & 14 deletions pylint/checkers/classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1848,20 +1848,18 @@ def _check_first_arg_for_type(self, node, metaclass=0):
"bad-mcs-method-argument",
node.name,
)
# regular class
else: # pylint: disable=else-if-used
# class method
if node.type == "classmethod" or node.name == "__class_getitem__":
self._check_first_arg_config(
first,
self.config.valid_classmethod_first_arg,
node,
"bad-classmethod-argument",
node.name,
)
# regular method without self as argument
elif first != "self":
self.add_message("no-self-argument", node=node)
# regular class with class method
elif node.type == "classmethod" or node.name == "__class_getitem__":
self._check_first_arg_config(
first,
self.config.valid_classmethod_first_arg,
node,
"bad-classmethod-argument",
node.name,
)
# regular class with regular method without self as argument
elif first != "self":
self.add_message("no-self-argument", node=node)

def _check_first_arg_config(self, first, config, node, message, method_name):
if first not in config:
Expand Down
38 changes: 14 additions & 24 deletions pylint/extensions/check_elif.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

from pylint.checkers import BaseTokenChecker
from pylint.checkers.utils import check_messages
from pylint.interfaces import IAstroidChecker, ITokenChecker
from pylint.interfaces import HIGH, IAstroidChecker, ITokenChecker


class ElseifUsedChecker(BaseTokenChecker):
Expand All @@ -38,37 +38,27 @@ def __init__(self, linter=None):
self._init()

def _init(self):
self._elifs = []
self._if_counter = 0
self._elifs = {}

def process_tokens(self, tokens):
# Process tokens and look for 'if' or 'elif'
for _, token, _, _, _ in tokens:
if token == "elif":
self._elifs.append(True)
elif token == "if":
self._elifs.append(False)
"""Process tokens and look for 'if' or 'elif'"""
self._elifs = {
begin: token for _, token, begin, _, _ in tokens if token in {"elif", "if"}
}

def leave_module(self, _: nodes.Module) -> None:
self._init()

def visit_ifexp(self, node: nodes.IfExp) -> None:
if isinstance(node.parent, nodes.FormattedValue):
return
self._if_counter += 1

def visit_comprehension(self, node: nodes.Comprehension) -> None:
self._if_counter += len(node.ifs)

@check_messages("else-if-used")
def visit_if(self, node: nodes.If) -> None:
if isinstance(node.parent, nodes.If):
orelse = node.parent.orelse
# current if node must directly follow an "else"
if orelse and orelse == [node]:
if not self._elifs[self._if_counter]:
self.add_message("else-if-used", node=node)
self._if_counter += 1
"""Current if node must directly follow an 'else'"""
if (
isinstance(node.parent, nodes.If)
and node.parent.orelse == [node]
and (node.lineno, node.col_offset) in self._elifs
and self._elifs[(node.lineno, node.col_offset)] == "if"
):
self.add_message("else-if-used", node=node, confidence=HIGH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

πŸ˜„

Copy link
Member Author

Choose a reason for hiding this comment

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

Not having an off by one error when we miscount an If increased the confidence πŸ˜„ But really the other option are:

Confidence = namedtuple("Confidence", ["name", "description"])
# Warning Certainties
HIGH = Confidence("HIGH", "No false positive possible.")
INFERENCE = Confidence("INFERENCE", "Warning based on inference result.")
INFERENCE_FAILURE = Confidence(
    "INFERENCE_FAILURE", "Warning based on inference with failures."
)
UNDEFINED = Confidence("UNDEFINED", "Warning without any associated confidence level.")

CONFIDENCE_LEVELS = [HIGH, INFERENCE, INFERENCE_FAILURE, UNDEFINED]

There's no inference here, so it should probably by HIGH. But also, we should probably refactor this (?) It was not used before so we might as well make it make sense. I'm not confident enough to say there will never be any false positive just because there was no inference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like we could use a NO_INFERENCE level. Maybe add the to the growing list of todo's for 2.13?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated #5317



def register(linter):
Expand Down
23 changes: 22 additions & 1 deletion tests/functional/ext/check_elif/check_elif.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
# pylint: disable=no-else-raise,unsupported-membership-test,using-constant-test

"""Checks use of "else if" triggers a refactor message"""
from typing import Union, Sequence, Any, Mapping


def my_function():
Expand All @@ -7,7 +10,7 @@ def my_function():
if myint > 5:
pass
else:
if myint <= 5: # [else-if-used]
if myint <= 5: # [else-if-used]
pass
else:
myint = 3
Expand All @@ -25,3 +28,21 @@ def my_function():
if myint:
pass
myint = 4


def _if_in_fstring_comprehension_with_elif(
params: Union[Sequence[Any], Mapping[str, Any]]
):
order = {}
if "z" not in "false":
raise TypeError(
f" {', '.join(sorted(i for i in order or () if i not in params))}"
)
elif "z" not in "true":
pass
else:
if "t" not in "false": # [else-if-used]
raise TypeError("d")
else:
if "y" in "life": # [else-if-used]
print("e")
6 changes: 4 additions & 2 deletions tests/functional/ext/check_elif/check_elif.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
else-if-used:10:8:my_function:"Consider using ""elif"" instead of ""else if""":HIGH
else-if-used:22:20:my_function:"Consider using ""elif"" instead of ""else if""":HIGH
else-if-used:13:8:my_function:"Consider using ""elif"" instead of ""else if""":HIGH
else-if-used:25:20:my_function:"Consider using ""elif"" instead of ""else if""":HIGH
else-if-used:44:8:_if_in_fstring_comprehension_with_elif:"Consider using ""elif"" instead of ""else if""":HIGH
else-if-used:47:12:_if_in_fstring_comprehension_with_elif:"Consider using ""elif"" instead of ""else if""":HIGH