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

False positive for implicit-str-concat when some but not all strings are raw #6663

Open
jacobtylerwalls opened this issue May 22, 2022 · 9 comments
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Minor 💅 Polishing pylint is always nice

Comments

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented May 22, 2022

Bug description

I don't think implicit-str-concat is a useful warning when the reason for breaking and restarting the string is to start or stop treating a portion of a string as a raw string. (As I understand it, we're trying to catch someone forgetting a comma between arguments.)

For instance, this string needs to escape \o but not \n:

import unittest

class MyTest(unittest.TestCase):
    def testColors(self):
        self.assertEqual(
            self.string_output(),
            r'\override Stem.color = "darkgreen"' '\n'
        )

Configuration

No response

Command used

pylint a.py

Pylint output

************* Module a
a.py:7: [W1404(implicit-str-concat), ] Implicit string concatenation found in call

---------------------------------------------------------------------
Your code has been rated at 7.50/10 (previous run: -40.00/10, +47.50)

Expected behavior

no msg

Pylint version

pylint 2.14.0-b1
astroid 2.12.0-dev0
Python 3.10.1 (v3.10.1:2cd268a3a9, Dec  6 2021, 14:28:59) [Clang 13.0.0 (clang-1300.0.29.3)]

OS / Environment

No response

Additional dependencies

No response

@jacobtylerwalls jacobtylerwalls added Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling Unreleased labels May 22, 2022
@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 22, 2022
@Pierre-Sassoulas
Copy link
Member

It make sense. I think the problem existed before we extended this check to call so it was already released just not as likely to be reported.

@jacobtylerwalls
Copy link
Member Author

When working on this, we could check for f-strings mixed with normal strings also. You might terminate an f-string so that it's easier to use { without escaping.

@jacobtylerwalls jacobtylerwalls added the Minor 💅 Polishing pylint is always nice label May 22, 2022
@DanielNoord
Copy link
Collaborator

This is hard to solve as ast doesn't store "rawness". See pylint-dev/astroid#1156.

@jhbuhrman
Copy link

Shouldn't we be heading for another direction in solving this? I believe the main intention of this particular check is to prevent accidental concatenation while adding members to a list or tuple, or perhaps indeed also when passing parameters in a function call. For example:

COLORS = [
    "red"
    "yellow",
    "blue",
]

(note the missing comma after "red").

I frequently use implicit string concatenation when breaking up too long strings (to prevent or fix line-too-long warnings). In doing so, the string parts that I want to be concatenated end up between parentheses, ref. the following example:

SENTENCES = [
    "This is a rather short sentence.",
    (
        "This is quite a long sentence, having all kinds of additional"
        " subordinate clauses, however."
    ),
    "This is another short sentence here.",
]

(shown as how black would reformat this).

So my plea is to only warn for string concatenation when the parts are not collected together in parentheses.

For now, I have to generally disable this check, to my regrets, since the COLORS example is a good one to be warned about.

@DanielNoord
Copy link
Collaborator

I think you should open a separate issue for this. This is mainly about concatenating different types.

@jhbuhrman
Copy link

I think you should open a separate issue for this. This is mainly about concatenating different types.

Yeah you're right, I discovered that my issue is substantially different (is more about that I've activated check-str-concat-over-line-jumps). Thanks.

(Although my remark could provide some sort of work-around for this issue as well? ...)

@jhbuhrman
Copy link

I don't want to spoil things, but I can't refrain myself from popping up the following philosophical question: how does pylint know that the string concatenation was intentional?

Suppose you're building up a list, where each digit (as) text is preceded by a backslash and you rely on your knowledge that you know what backslash/character combinations are interpreted as a escape sequences and what are not (perhaps not the best coding practice, but anyways):

my_digits = ["\zero", "\one", r"\two", r"\three", r"\four", r"\five" "\six", "\seven", "\eight", r"\nine"]

With this issue's proposed solution, this erroneous code would pass without a complaint, since it will not complain about the missing comma between r"\five" and "\six".

@Pierre-Sassoulas
Copy link
Member

We favor false negatives, so this is a trade off I think we should make. There's two problem in r"\five" "\six", , the implicit string concat and the missing r before six.

@jhbuhrman
Copy link

jhbuhrman commented Dec 13, 2022

Yes, I should have come up with a better example, but I think we can come up with examples where it is completely logical to enter one value within a list of strings as a raw string, and another as a plain string, and perhaps a third as an f-string. A missing comma in those situations doesn't automatically imply that the concatenation is intentional.

[[[

Although technically there's nothing wrong (yet) with unrecognized escape sequences (as in "\six") I can imagine that it is worthwhile to have a check that doesn't allow for it (especially now I'm reading in https://docs.python.org/3.10/reference/lexical_analysis.html?highlight=escape#escape-sequences that these unrecognized escape sequences are triggering a DeprecationWarning since py36).

]]]

Anyhow, that's another issue. As stated earlier, given the example at the top of this issue, I would propose to "solve" it by allowing implicit string concatenation if and only if you surround it with a set of parentheses (as a signal to your linter that the concatenation is intentional), as in:

import unittest

class MyTest(unittest.TestCase):
    def testColors(self):
        self.assertEqual(
            self.string_output(),
            (r'\override Stem.color = "darkgreen"' '\n')
        )

(ref. #7929)

If I'm not mistaken, the black formatter leaves this (sort of redundant) pair of parentheses alone...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Minor 💅 Polishing pylint is always nice
Projects
None yet
Development

No branches or pull requests

4 participants