From 7d9b6b98352ea17f03c2e6b02b43fd762e037574 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Fri, 25 Nov 2022 09:17:46 -0300 Subject: [PATCH 1/5] Fix ``logging-fstring-interpolation`` --- doc/whatsnew/fragments/4984.false_positive | 3 +++ pylint/checkers/logging.py | 20 +++++++++++++------ .../logging_fstring_interpolation_py37.py | 5 +++++ .../logging_fstring_interpolation_py37.txt | 2 ++ 4 files changed, 24 insertions(+), 6 deletions(-) create mode 100644 doc/whatsnew/fragments/4984.false_positive diff --git a/doc/whatsnew/fragments/4984.false_positive b/doc/whatsnew/fragments/4984.false_positive new file mode 100644 index 0000000000..b319205641 --- /dev/null +++ b/doc/whatsnew/fragments/4984.false_positive @@ -0,0 +1,3 @@ +Fix ``logging-fstring-interpolation`` false positive raised when logging and f-string with ``%s`` formatting. + +Closes #4984 diff --git a/pylint/checkers/logging.py b/pylint/checkers/logging.py index 53ac24f603..8060d43978 100644 --- a/pylint/checkers/logging.py +++ b/pylint/checkers/logging.py @@ -240,8 +240,9 @@ def _check_log_method(self, node: nodes.Call, name: str) -> None: else: return - if isinstance(node.args[format_pos], nodes.BinOp): - binop = node.args[format_pos] + format_arg = node.args[format_pos] + if isinstance(format_arg, nodes.BinOp): + binop = format_arg emit = binop.op == "%" if binop.op == "+": total_number_of_strings = sum( @@ -256,11 +257,18 @@ def _check_log_method(self, node: nodes.Call, name: str) -> None: node=node, args=(self._helper_string(node),), ) - elif isinstance(node.args[format_pos], nodes.Call): - self._check_call_func(node.args[format_pos]) - elif isinstance(node.args[format_pos], nodes.Const): + elif isinstance(format_arg, nodes.Call): + self._check_call_func(format_arg) + elif isinstance(format_arg, nodes.Const): self._check_format_string(node, format_pos) - elif isinstance(node.args[format_pos], nodes.JoinedStr): + elif isinstance(format_arg, nodes.JoinedStr): + if any( + "%s" in val.value + for val in format_arg.values + if isinstance(val, nodes.Const) + ): + # Detect % formatting inside an f-string, such as `f'Hello %s'` + return self.add_message( "logging-fstring-interpolation", node=node, diff --git a/tests/functional/l/logging/logging_fstring_interpolation_py37.py b/tests/functional/l/logging/logging_fstring_interpolation_py37.py index 963b2ce8ce..2319dbf355 100644 --- a/tests/functional/l/logging/logging_fstring_interpolation_py37.py +++ b/tests/functional/l/logging/logging_fstring_interpolation_py37.py @@ -3,3 +3,8 @@ VAR = "string" logging.error(f"{VAR}") # [logging-fstring-interpolation] + +WORLD = "world" +logging.error(f'Hello {WORLD}') # [logging-fstring-interpolation] + +logging.error(f'Hello %s', 'World!') # [f-string-without-interpolation] diff --git a/tests/functional/l/logging/logging_fstring_interpolation_py37.txt b/tests/functional/l/logging/logging_fstring_interpolation_py37.txt index a41c3e1450..f8cfcd9c0f 100644 --- a/tests/functional/l/logging/logging_fstring_interpolation_py37.txt +++ b/tests/functional/l/logging/logging_fstring_interpolation_py37.txt @@ -1 +1,3 @@ logging-fstring-interpolation:5:0:5:23::Use lazy % formatting in logging functions:UNDEFINED +logging-fstring-interpolation:8:0:8:31::Use lazy % formatting in logging functions:UNDEFINED +f-string-without-interpolation:10:14:10:25::Using an f-string that does not have any interpolated variables:UNDEFINED From 676a4bb6f3f7c1a2ed0f4275f5be21e57259d96e Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Fri, 25 Nov 2022 17:42:57 -0300 Subject: [PATCH 2/5] check if s formatting in f str --- pylint/checkers/logging.py | 28 +++++++++++++++---- .../logging_fstring_interpolation_py37.py | 1 + .../logging_fstring_interpolation_py37.txt | 1 + 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/pylint/checkers/logging.py b/pylint/checkers/logging.py index 8060d43978..e3488abe83 100644 --- a/pylint/checkers/logging.py +++ b/pylint/checkers/logging.py @@ -105,6 +105,13 @@ "warning", } +MOST_COMMON_FORMATTING = { + "%s", + "%d", + "%f", + "%r", +} + def is_method_call( func: bases.BoundMethod, types: tuple[str, ...] = (), methods: tuple[str, ...] = () @@ -262,12 +269,7 @@ def _check_log_method(self, node: nodes.Call, name: str) -> None: elif isinstance(format_arg, nodes.Const): self._check_format_string(node, format_pos) elif isinstance(format_arg, nodes.JoinedStr): - if any( - "%s" in val.value - for val in format_arg.values - if isinstance(val, nodes.Const) - ): - # Detect % formatting inside an f-string, such as `f'Hello %s'` + if str_formatting_in_f_string(format_arg): return self.add_message( "logging-fstring-interpolation", @@ -401,5 +403,19 @@ def _count_supplied_tokens(args: list[nodes.NodeNG]) -> int: return sum(1 for arg in args if not isinstance(arg, nodes.Keyword)) +def str_formatting_in_f_string(node: nodes.NodeNG) -> bool: + """Determine whether the node represents an f-string with string formatting. + + For example: `f'Hello %s'` + """ + # Check "%" presence first for performance. + return any( + "%" in val.value and any(x in val.value for x in MOST_COMMON_FORMATTING) + for val in node.values + if isinstance(val, nodes.Const) + ) + + +# Detect % formatting inside an f-string, such as `f'Hello %s'` def register(linter: PyLinter) -> None: linter.register_checker(LoggingChecker(linter)) diff --git a/tests/functional/l/logging/logging_fstring_interpolation_py37.py b/tests/functional/l/logging/logging_fstring_interpolation_py37.py index 2319dbf355..d08424eb04 100644 --- a/tests/functional/l/logging/logging_fstring_interpolation_py37.py +++ b/tests/functional/l/logging/logging_fstring_interpolation_py37.py @@ -8,3 +8,4 @@ logging.error(f'Hello {WORLD}') # [logging-fstring-interpolation] logging.error(f'Hello %s', 'World!') # [f-string-without-interpolation] +logging.error(f'Hello %d', 1) # [f-string-without-interpolation] diff --git a/tests/functional/l/logging/logging_fstring_interpolation_py37.txt b/tests/functional/l/logging/logging_fstring_interpolation_py37.txt index f8cfcd9c0f..d8b84d674d 100644 --- a/tests/functional/l/logging/logging_fstring_interpolation_py37.txt +++ b/tests/functional/l/logging/logging_fstring_interpolation_py37.txt @@ -1,3 +1,4 @@ logging-fstring-interpolation:5:0:5:23::Use lazy % formatting in logging functions:UNDEFINED logging-fstring-interpolation:8:0:8:31::Use lazy % formatting in logging functions:UNDEFINED f-string-without-interpolation:10:14:10:25::Using an f-string that does not have any interpolated variables:UNDEFINED +f-string-without-interpolation:11:14:11:25::Using an f-string that does not have any interpolated variables:UNDEFINED From d483c835d6b862c6572d23fd5db0a34c48a6af0c Mon Sep 17 00:00:00 2001 From: Dani Alcala <112832187+clavedeluna@users.noreply.github.com> Date: Fri, 25 Nov 2022 20:30:48 -0300 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com> --- pylint/checkers/logging.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pylint/checkers/logging.py b/pylint/checkers/logging.py index e3488abe83..a1fc1099c0 100644 --- a/pylint/checkers/logging.py +++ b/pylint/checkers/logging.py @@ -403,7 +403,7 @@ def _count_supplied_tokens(args: list[nodes.NodeNG]) -> int: return sum(1 for arg in args if not isinstance(arg, nodes.Keyword)) -def str_formatting_in_f_string(node: nodes.NodeNG) -> bool: +def str_formatting_in_f_string(node: nodes.JoinedStr) -> bool: """Determine whether the node represents an f-string with string formatting. For example: `f'Hello %s'` @@ -416,6 +416,5 @@ def str_formatting_in_f_string(node: nodes.NodeNG) -> bool: ) -# Detect % formatting inside an f-string, such as `f'Hello %s'` def register(linter: PyLinter) -> None: linter.register_checker(LoggingChecker(linter)) From b958d0a681a0d2cada5e374f2b3dc02fb4bf3a03 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sat, 26 Nov 2022 14:40:05 +0100 Subject: [PATCH 4/5] Update pylint/checkers/logging.py --- pylint/checkers/logging.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pylint/checkers/logging.py b/pylint/checkers/logging.py index a1fc1099c0..b3a6845b2f 100644 --- a/pylint/checkers/logging.py +++ b/pylint/checkers/logging.py @@ -105,12 +105,7 @@ "warning", } -MOST_COMMON_FORMATTING = { - "%s", - "%d", - "%f", - "%r", -} +MOST_COMMON_FORMATTING = frozenset("%s", "%d", "%f", "%r") def is_method_call( From d4403c3b077be9a2c935a79210dc9a1205347c54 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sat, 26 Nov 2022 14:41:20 +0100 Subject: [PATCH 5/5] Update pylint/checkers/logging.py --- pylint/checkers/logging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/checkers/logging.py b/pylint/checkers/logging.py index b3a6845b2f..1cfa33c0f6 100644 --- a/pylint/checkers/logging.py +++ b/pylint/checkers/logging.py @@ -105,7 +105,7 @@ "warning", } -MOST_COMMON_FORMATTING = frozenset("%s", "%d", "%f", "%r") +MOST_COMMON_FORMATTING = frozenset(["%s", "%d", "%f", "%r"]) def is_method_call(