Fix AstroidError in similarity checker with imports/signatures ignored

[jacobtylerwalls]

Type of Changes

	Type
✓	🐛 Bug fix

Description

The array of lines being filtered by append_stream() needs to be parsable as valid python code if imports/signatures are being ignored, see: https://github.com/PyCQA/pylint/blob/adf660a7300148a31cdb8bc25301bfd21af0602b/pylint/checkers/similar.py#L565-L566.

Solution now is to re-parse the AST before trying to filter out lines that were disabled.

Closes #6301

[Pierre-Sassoulas]

Why do we need to parse code with astroid in the duplicate-code checker ? Isn't it a check where we can work directly on text ?

[coveralls]

Pull Request Test Coverage Report for Build 2180193525

• 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
• 57 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.003%) to 95.113%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/checkers/similar.py	5	6	83.33%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/similar.py	1	96.28%
pylint/checkers/logging.py	8	94.29%
pylint/checkers/imports.py	10	94.37%
pylint/checkers/strings.py	13	93.95%
pylint/checkers/typecheck.py	25	95.19%

Totals
Change from base Build 2177698716:	-0.003%
Covered Lines:	15707
Relevant Lines:	16514

---

💛 - Coveralls

[jacobtylerwalls]

Why do we need to parse code with astroid in the duplicate-code checker ?

Multi-line imports: 76ce7c1

[DanielNoord]

Thanks @jacobtylerwalls!

[jacobtylerwalls]

Need a solution for TestSimilarCodeChecker.test_duplicate_code_raw_strings_disable_line_two, which fails on 2.13.x

[jacobtylerwalls]

@DanielNoord, well I gave this a shot, and I think it's probably mergeable, but there are a whole host of problems you can create beyond the reported one, because anywhere you can put a disable comment, you can wipe out a line required for the code to be interpreted.

E.g., this will still crash:

def a():
    x = [
        None, None
    ]  # pylint: disable=duplicate-code

I'll open a separate PR to just catch AstroidError and let there be a lack of "ignore-imports" and "ignore-signatures" functionality for line disables. (Can add to the docs something saying use block disables instead, assuming that works.)

[jacobtylerwalls]

Triple-quoted strings will mess with this also. Ultimately it's a losing battle. I'll open that follow-up PR.

[DanielNoord]

@jacobtylerwalls You have probably explored this, but can we move the disable checking to L382? So after the LineSet has been created? In the final line set we still have access to the line number of each individual line so could we create a DisableCheckedLineSet afterwards?

[jacobtylerwalls]

No, it had not occurred to me! I was boiling in the water as I saw the problem scope get bigger. 🎈

I like that a lot. Should be simpler.

[jacobtylerwalls]

I fought with this locally and finally just no-verified it. Why is this syntax okay with our minimum python version of 3.7.2? (Well, 3.6.2, since we are talking about backporting, potentially.)

[jacobtylerwalls]

oh, did we future import annotations everywhere? ah. but still a problem with 3.6, then.

[jacobtylerwalls]

I guess we don't have to backport this if only one user reported it and 2.14 is coming soon.

[jacobtylerwalls]

Cool. LMK if I should move the release note to 2.14. If we backport I guess we can just use different type annotation syntax.

[Pierre-Sassoulas]

This commit is hard to cherry-pick. I have the following non trivial conflicts to fix:

diff --cc ChangeLog
index 126e82e2b,74d3d8005..000000000
--- a/ChangeLog
+++ b/ChangeLog
diff --cc pylint/checkers/similar.py
index 69b401fa4,11b12995d..000000000
--- a/pylint/checkers/similar.py
+++ b/pylint/checkers/similar.py
@@@ -359,28 -366,25 +360,47 @@@ class Similar
              readlines = decoding_stream(stream, encoding).readlines
          else:
              readlines = stream.readlines  # type: ignore[assignment] # hint parameter is incorrectly typed as non-optional
+ 
          try:
++<<<<<<< HEAD
 +            active_lines: List[str] = []
 +            if hasattr(self, "linter"):
 +                # Remove those lines that should be ignored because of disables
 +                for index, line in enumerate(readlines()):
 +                    if self.linter._is_one_message_enabled("R0801", index + 1):  # type: ignore[attr-defined]
 +                        active_lines.append(line)
 +            else:
 +                active_lines = readlines()
 +
 +            self.linesets.append(
 +                LineSet(
 +                    streamid,
 +                    active_lines,
 +                    self.ignore_comments,
 +                    self.ignore_docstrings,
 +                    self.ignore_imports,
 +                    self.ignore_signatures,
 +                )
 +            )
++=======
+             lines = readlines()
++>>>>>>> 1664202ba... Fix `AstroidError` in similarity checker with imports/signatures ignored (#6357)
          except UnicodeDecodeError:
-             pass
+             lines = []
+ 
+         self.linesets.append(
+             LineSet(
+                 streamid,
+                 lines,
+                 self.namespace.ignore_comments,
+                 self.namespace.ignore_docstrings,
+                 self.namespace.ignore_imports,
+                 self.namespace.ignore_signatures,
+                 line_enabled_callback=self.linter._is_one_message_enabled  # type: ignore[attr-defined]
+                 if hasattr(self, "linter")
+                 else None,
+             )
+         )
  
      def run(self) -> None:
          """Start looking for similarities and display results on stdout."""
@@@ -552,7 -560,8 +572,12 @@@ def stripped_lines
      ignore_docstrings: bool,
      ignore_imports: bool,
      ignore_signatures: bool,
++<<<<<<< HEAD
 +) -> List[LineSpecifs]:
++=======
+     line_enabled_callback: Callable[[str, int], bool] | None = None,
+ ) -> list[LineSpecifs]:
++>>>>>>> 1664202ba... Fix `AstroidError` in similarity checker with imports/signatures ignored (#6357)
      """Return tuples of line/line number/line type with leading/trailing whitespace and any ignored code features removed.
  
      :param lines: a collection of lines

[DanielNoord]

I think incoming is fine. LineSet has gained a new argument which should be a Callable[[str, int], bool] | None. I think the incoming change has that covered.

[Pierre-Sassoulas]

Because of that the code is not compatible with python 3.6 the fix to the similarity checker is mixed with it.

pylint/checkers/similar.py:663: in LineSet
    line_enabled_callback: Callable[[str, int], bool] | None = None,
E   TypeError: 'ABCMeta' object is not subscriptable

[DanielNoord]

Oh, that should be fixable by importing Callable from typing instead of collections.

[Pierre-Sassoulas]

There were other problem with typing, I removed the typing, but the tests suite now fail with 'SimilarChecker' object has no attribute 'namespace'. Would you mind checking if the cherry-pick is reasonably doable @DanielNoord ? On the current maintenance branch : git cherry-pick 1664202ba. I'm half inclined to move this to 2.14.0, because if we have to change too much during the cherry-pick we might have two incompatible branch to maintain (and the merge back in main will be painful too)

[DanielNoord]

Cherry-picked to maintenance!