Adding implicit-str-concat-in-sequence check #1655

Lucas-C · 2017-09-11T17:01:22Z

Fixes / new features

Adds a new warning. Example of code that would raise it:

TEST_LIST = ('a', 'b' 'c')

Message:

String literals are implicitly concatenated in a comma-separated list : maybe a comma is missing ?

rogalski · 2017-09-12T07:07:53Z

Hello,

Thank you for you PR!

Can we have some test case along lines of:

def my_func(arg, arg2, msg):
    pass

my_func("s1", "s2", "really long message that I'd like to "
                    "split across multiple lines")

Case above (IMHO) shouldn't emit a warning but it looks like it would be emitted.

Lucas-C · 2017-09-12T07:55:38Z

Well spotted, thanks
I'm going to add it and fix the checker

Lucas-C · 2017-09-12T09:02:51Z

Ok, so i'm facing a dead-end here :(

First, this problem has a bit of history:

Now, I see 2 approaches in pylint:

use astroid AST visitor-pattern. Sadly it is limited by Python builtin compile method (cf. usage in code) which does not allow to detect string concatenation : on this level, for astroid, it's just a single string node
analyze tokens generated with tokenize. This time, the problem is that I cannot detect if I'm in a list/tuple context, or if I'm looking at function arguments as in your example

The only idea I've got would be to retrieve from the source the code snippet corresponding to each list/tuple element, to check if there is string concatenation.

Did any checker had to implement such thing already ? I.e retrieving source code from node.lineno / node.col_offset ?

Or does anyone has a better idea ?

asottile · 2017-09-13T21:07:57Z

I'm not sure if pylint gives you an option to but one way would be to cross reference the ast nodes with the tokenized output -- this is roughly how pyupgrade finds list / tuple nodes.

Otherwise disregard me, just drive by browsing issues (while I wait for pylint to clone) and noticed a familiar name :)

Lucas-C · 2017-09-14T08:30:14Z

Hey, thanks for the advice @asottile !

I contemplated this idea, but it's awesome to see you already implemented it quite concisely:
https://github.com/asottile/pyupgrade/blob/master/pyupgrade.py#L370-L383

Before implementing something like that in pylint though, it'd be great to know:

is there already something like that in pylint ? I.e cross-referencing tokens & ast
else, in which module would it be best to put this ? pylint/utils.py ?

@PCManticore, @rogalski ?

rogalski · 2017-09-17T08:40:16Z

@Lucas-C You'll need a checker that inherits from BaseTokenChecker. From what I see in code, processing tokens occurs before processing AST nodes, so potential implementation may look like:

Collect all "suspiciously" joined strings in process_tokens (which is already implemented), store information about them on some class attribute
For each const node visited:
If constant node position (row / col) matches suspicious string found earlier - emit a message

Lucas-C · 2017-09-17T14:58:32Z

Thanks @rogalski !

I updated the PR, I think I nailed it ;)

ChangeLog

pylint/checkers/strings.py

rogalski · 2017-09-20T16:39:32Z

pylint/test/functional/str_literals_concatenation_and_comma.py

@@ -0,0 +1,19 @@
+#pylint: disable=bad-continuation,invalid-name,missing-docstring


your tests does not pass.

rogalski · 2017-09-20T16:40:53Z

pylint/checkers/strings.py

@@ -541,15 +562,21 @@ class StringConstantChecker(BaseTokenChecker):
    # Unicode strings.
    UNICODE_ESCAPE_CHARACTERS = 'uUN'

+    string_tokens = {}


this really should be instance attribute, no class attribute.
We also need to be sure that data gets thrown away before switching to new module for analysis (simple reinitialization of dict on the beginning of process tokens should work fine).

I tested it manually, it is thrown away.
I'm going to try to make this an instance attribute, but it's bit annoying that you precisely recommended a class attribute in one of your earlier comment :(

Do you want to add a unit test ?
If so, could you please then point me to an existing test I could use as an example to perform such "reinitialization" ?

pylint/checkers/strings.py

rogalski · 2017-09-20T16:43:51Z

pylint/checkers/strings.py

+        # This check relies on the asumption that StringConstantChecker is run beforehand
+        for elt in iterable_node.elts:
+            if isinstance(elt, astroid.node_classes.Const) and elt.pytype() == 'builtins.str':
+                if StringConstantChecker.string_tokens[(elt.lineno, elt.col_offset)] != elt.value:


Also, checker classes really should be independent, consider doing your process tokens inside StringFormatChecker

Fixed by introducing IAstroidAndTokensChecker interface

Lucas-C · 2017-09-21T19:25:54Z

Tests are failing with Python 2 interpreter.
I'm going to install one on my machine to figure out why (probably either the AST walker or the tokenizer behave differently in regard to string literals concatenation)

Lucas-C · 2017-09-24T12:55:51Z

I fixed this hook for Python 2

Lucas-C · 2017-09-25T14:21:53Z

I don't quite get why the pylint tox environment is failing.
There are many [R]efactor messages but they all seem to have been preexisting

rogalski · 2017-09-26T20:35:57Z

pylint/checkers/strings.py

@@ -545,11 +588,11 @@ def process_module(self, module):
        self._unicode_literals = 'unicode_literals' in module.future_imports

    def process_tokens(self, tokens):
-        for (tok_type, token, (start_row, _), _, _) in tokens:
+        for (tok_type, token, start, _, _) in tokens:


this change is redundant

rogalski · 2017-09-26T20:36:01Z

pylint/checkers/strings.py

            if tok_type == tokenize.STRING:
                # 'token' is the whole un-parsed token; we can look at the start
                # of it to see whether it's a raw or unicode string etc.
-                self.process_string_token(token, start_row)
+                self.process_string_token(token, start[0])


this change is redundant

rogalski · 2017-09-26T20:37:20Z

pylint/checkers/strings.py

@@ -527,7 +570,7 @@ class StringConstantChecker(BaseTokenChecker):
                  'String constant might be missing an r or u prefix.',
                  'anomalous-unicode-escape-in-string',
                  'Used when an escape like \\u is encountered in a byte '
-                  'string where it has no effect.'),
+                  'string where it has no effect.')


this change is redundant

rogalski · 2017-09-26T20:39:53Z

pylint/interfaces.py

@@ -79,6 +79,18 @@ class IAstroidChecker(IChecker):
    """


+class IAstroidAndTokensChecker(IAstroidChecker):


Isn't BaseTokenChecker exact same thing? From what I see BaseTokenChecker is a checker, which means it can involve walking ASTs and visiting nodes... Or am I missing something?

There is a side effect however:
making StringFormatChecker inherit from BaseTokenChecker and implement ITokenChecker forces to change all the .add_message call sites in this class, because it now requires a line parameter:
https://github.com/PyCQA/pylint/blob/master/pylint/utils.py#L374
https://github.com/PyCQA/pylint/blob/master/pylint/utils.py#L153

rogalski · 2017-09-26T20:42:41Z

pylint/lint.py

@@ -844,10 +844,12 @@ def _do_check(self, files_or_modules):
                         and c is not self]
        rawcheckers = [c for c in _checkers
                       if interfaces.implements(c, interfaces.IRawChecker)]
+        astcheckers = [c for c in _checkers
