From 7ab3bf729f0c6fa525e7cc2bebe2f3566996bff2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Rogalski?= Date: Sun, 17 Oct 2021 22:05:14 +0300 Subject: [PATCH 1/6] ASTVerifier Improved performance by caching intermediate data during bisect. --- src/darker/__main__.py | 16 +++++------ src/darker/tests/test_main.py | 24 ++++++++-------- src/darker/tests/test_verification.py | 30 +++++++++++++++++++- src/darker/verification.py | 40 +++++++++++++++++++++++++-- 4 files changed, 86 insertions(+), 24 deletions(-) diff --git a/src/darker/__main__.py b/src/darker/__main__.py index 0b0f2cf05..20dd3375f 100644 --- a/src/darker/__main__.py +++ b/src/darker/__main__.py @@ -32,8 +32,8 @@ from darker.help import ISORT_INSTRUCTION from darker.import_sorting import apply_isort, isort from darker.linting import run_linters -from darker.utils import GIT_DATEFORMAT, TextDocument, get_common_root -from darker.verification import BinarySearch, NotEquivalentError, verify_ast_unchanged +from darker.utils import GIT_DATEFORMAT, TextDocument, debug_dump, get_common_root +from darker.verification import ASTVerifier, BinarySearch, NotEquivalentError logger = logging.getLogger(__name__) @@ -143,6 +143,9 @@ def _reformat_single_file( # pylint: disable=too-many-arguments,too-many-locals max_context_lines = len(rev2_isorted.lines) minimum_context_lines = BinarySearch(0, max_context_lines + 1) last_successful_reformat = None + + verifier = ASTVerifier(baseline=rev2_isorted) + while not minimum_context_lines.found: context_lines = minimum_context_lines.get_next() if context_lines > 0: @@ -176,13 +179,8 @@ def _reformat_single_file( # pylint: disable=too-many-arguments,too-many-locals len(rev2_isorted.lines), len(chosen.lines), ) - try: - verify_ast_unchanged(rev2_isorted, chosen, black_chunks, edited_linenums) - except NotEquivalentError: - # Diff produced misaligned chunks which couldn't be reconstructed into - # a partially re-formatted Python file which produces an identical AST. - # Try again with a larger `-U` option for `git diff`, - # or give up if `context_lines` is already very large. + if not verifier.is_equivalent_to_baseline(chosen): + debug_dump(black_chunks, edited_linenums) logger.debug( "AST verification of %s with %s lines of context failed", src, diff --git a/src/darker/tests/test_main.py b/src/darker/tests/test_main.py index 009865dff..d6b97b904 100644 --- a/src/darker/tests/test_main.py +++ b/src/darker/tests/test_main.py @@ -8,7 +8,7 @@ from pathlib import Path from textwrap import dedent from types import SimpleNamespace -from unittest.mock import Mock, patch +from unittest.mock import patch import pytest from black import find_project_root @@ -44,12 +44,13 @@ def run_isort(git_repo, monkeypatch, caplog, request): paths = git_repo.add({"test1.py": "original"}, commit="Initial commit") paths["test1.py"].write_bytes(b"changed") args = getattr(request, "param", ()) - with patch.multiple( - darker.__main__, - run_black=Mock(return_value=TextDocument()), - verify_ast_unchanged=Mock(), - ), patch("darker.import_sorting.isort_code") as isort_code: - isort_code.return_value = "dummy isort output" + isorted_code = "import os; import sys;" + blacken_code = "import os\nimport sys\n" + patch_run_black_ctx = patch.object( + darker.__main__, "run_black", return_value=TextDocument(blacken_code) + ) + with patch_run_black_ctx, patch("darker.import_sorting.isort_code") as isort_code: + isort_code.return_value = isorted_code darker.__main__.main(["--isort", "./test1.py", *args]) return SimpleNamespace( isort_code=darker.import_sorting.isort_code, caplog=caplog @@ -213,11 +214,10 @@ def test_format_edited_parts_ast_changed(git_repo, caplog): caplog.set_level(logging.DEBUG, logger="darker.__main__") paths = git_repo.add({"a.py": "1\n2\n3\n4\n5\n6\n7\n8\n"}, commit="Initial commit") paths["a.py"].write_bytes(b"8\n7\n6\n5\n4\n3\n2\n1\n") - with patch.object( - darker.__main__, "verify_ast_unchanged" - ) as verify_ast_unchanged, pytest.raises(NotEquivalentError): - verify_ast_unchanged.side_effect = NotEquivalentError - + mock_ctx = patch.object( + darker.verification.ASTVerifier, "is_equivalent_to_baseline", return_value=False + ) + with mock_ctx, pytest.raises(NotEquivalentError): _ = list( darker.__main__.format_edited_parts( git_repo.root, diff --git a/src/darker/tests/test_verification.py b/src/darker/tests/test_verification.py index c8aef6542..96680156a 100644 --- a/src/darker/tests/test_verification.py +++ b/src/darker/tests/test_verification.py @@ -5,7 +5,12 @@ import pytest from darker.utils import DiffChunk, TextDocument -from darker.verification import BinarySearch, NotEquivalentError, verify_ast_unchanged +from darker.verification import ( + ASTVerifier, + BinarySearch, + NotEquivalentError, + verify_ast_unchanged, +) @pytest.mark.kwparametrize( @@ -28,6 +33,29 @@ def test_verify_ast_unchanged(dst_content, expect): assert expect is None +def test_ast_verifier_is_equivalent(): + """``darker.verification.ASTVerifier.is_equivalent_to_baseline``""" + verifier = ASTVerifier(baseline=TextDocument.from_lines(["if True: pass"])) + assert verifier.is_equivalent_to_baseline( + TextDocument.from_lines(["if True:", " pass"]) + ) + assert not verifier.is_equivalent_to_baseline( + TextDocument.from_lines(["if False: pass"]) + ) + + +def test_ast_verifier_assert_equivalent(): + """``darker.verification.ASTVerifier.assert_equivalent_to_baseline``""" + verifier = ASTVerifier(baseline=TextDocument.from_lines(["if True: pass"])) + verifier.assert_equivalent_to_baseline( + TextDocument.from_lines(["if True:", " pass"]) + ) + with pytest.raises(NotEquivalentError): + verifier.assert_equivalent_to_baseline( + TextDocument.from_lines(["if False: pass"]) + ) + + def test_binary_search_premature_result(): """``darker.verification.BinarySearch``""" with pytest.raises(RuntimeError): diff --git a/src/darker/verification.py b/src/darker/verification.py index 65c1eda23..f70ff4f04 100644 --- a/src/darker/verification.py +++ b/src/darker/verification.py @@ -1,8 +1,8 @@ """Verification for unchanged AST before and after reformatting""" -from typing import List +from typing import Dict, List -from black import assert_equivalent +from black import assert_equivalent, parse_ast, stringify_ast from darker.utils import DiffChunk, TextDocument, debug_dump @@ -67,3 +67,39 @@ def verify_ast_unchanged( except AssertionError as exc_info: debug_dump(black_chunks, edited_linenums) raise NotEquivalentError(str(exc_info)) + + +class ASTVerifier: + """ + Verifies if reformatted TextDocument is AST-equivalent to baseline. + + Keeps in-memory data about previous comparisons to improve performance. + """ + + def __init__(self, baseline: TextDocument) -> None: + self._baseline_ast_str = self._to_ast_str(baseline) + self._comparisons: Dict[str, bool] = {baseline.string: True} + + @staticmethod + def _to_ast_str(document: TextDocument) -> str: + return "\n".join(stringify_ast(parse_ast(document.string))) + + def is_equivalent_to_baseline(self, document: TextDocument) -> bool: + """Returns true if document is AST-equivalent to baseline""" + if document.string in self._comparisons: + return self._comparisons[document.string] + + try: + document_ast_str = self._to_ast_str(document) + except SyntaxError: + comparison = False + else: + comparison = self._baseline_ast_str == document_ast_str + + self._comparisons[document.string] = comparison + return self._comparisons[document.string] + + def assert_equivalent_to_baseline(self, document: TextDocument) -> None: + """Raises NotEquivalentError if document is not AST-equivalent to baseline""" + if not self.is_equivalent_to_baseline(document): + raise NotEquivalentError From 5bc8829b8db706c545cd3708a1acb686dcf44809 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Rogalski?= Date: Sun, 17 Oct 2021 22:05:14 +0300 Subject: [PATCH 2/6] Fix docstring code style according to review --- src/darker/verification.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/darker/verification.py b/src/darker/verification.py index f70ff4f04..4dc67df99 100644 --- a/src/darker/verification.py +++ b/src/darker/verification.py @@ -70,8 +70,7 @@ def verify_ast_unchanged( class ASTVerifier: - """ - Verifies if reformatted TextDocument is AST-equivalent to baseline. + """Verifies if reformatted TextDocument is AST-equivalent to baseline. Keeps in-memory data about previous comparisons to improve performance. """ From 9b5d4a935407c51230d58ced1d068fc57931dd19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Rogalski?= Date: Sun, 17 Oct 2021 22:05:14 +0300 Subject: [PATCH 3/6] Add missing tests for SyntaxError --- src/darker/tests/test_verification.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/darker/tests/test_verification.py b/src/darker/tests/test_verification.py index 96680156a..fe21a2871 100644 --- a/src/darker/tests/test_verification.py +++ b/src/darker/tests/test_verification.py @@ -42,6 +42,9 @@ def test_ast_verifier_is_equivalent(): assert not verifier.is_equivalent_to_baseline( TextDocument.from_lines(["if False: pass"]) ) + assert not verifier.is_equivalent_to_baseline( + TextDocument.from_lines(["if False:"]) + ) def test_ast_verifier_assert_equivalent(): @@ -54,6 +57,8 @@ def test_ast_verifier_assert_equivalent(): verifier.assert_equivalent_to_baseline( TextDocument.from_lines(["if False: pass"]) ) + with pytest.raises(NotEquivalentError): + verifier.assert_equivalent_to_baseline(TextDocument.from_lines(["if False:"])) def test_binary_search_premature_result(): From 313ddc761a716b5e374b8138779bf808a018869c Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 18 Oct 2021 10:37:44 +0300 Subject: [PATCH 4/6] Remove unused verification assertion method --- src/darker/tests/test_verification.py | 14 -------------- src/darker/verification.py | 5 ----- 2 files changed, 19 deletions(-) diff --git a/src/darker/tests/test_verification.py b/src/darker/tests/test_verification.py index fe21a2871..ae4df5fe4 100644 --- a/src/darker/tests/test_verification.py +++ b/src/darker/tests/test_verification.py @@ -47,20 +47,6 @@ def test_ast_verifier_is_equivalent(): ) -def test_ast_verifier_assert_equivalent(): - """``darker.verification.ASTVerifier.assert_equivalent_to_baseline``""" - verifier = ASTVerifier(baseline=TextDocument.from_lines(["if True: pass"])) - verifier.assert_equivalent_to_baseline( - TextDocument.from_lines(["if True:", " pass"]) - ) - with pytest.raises(NotEquivalentError): - verifier.assert_equivalent_to_baseline( - TextDocument.from_lines(["if False: pass"]) - ) - with pytest.raises(NotEquivalentError): - verifier.assert_equivalent_to_baseline(TextDocument.from_lines(["if False:"])) - - def test_binary_search_premature_result(): """``darker.verification.BinarySearch``""" with pytest.raises(RuntimeError): diff --git a/src/darker/verification.py b/src/darker/verification.py index 4dc67df99..f99ae2ad0 100644 --- a/src/darker/verification.py +++ b/src/darker/verification.py @@ -97,8 +97,3 @@ def is_equivalent_to_baseline(self, document: TextDocument) -> bool: self._comparisons[document.string] = comparison return self._comparisons[document.string] - - def assert_equivalent_to_baseline(self, document: TextDocument) -> None: - """Raises NotEquivalentError if document is not AST-equivalent to baseline""" - if not self.is_equivalent_to_baseline(document): - raise NotEquivalentError From 45dafbae25f426df69896f31c40dd77f511826d8 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 18 Oct 2021 10:38:29 +0300 Subject: [PATCH 5/6] Consistent docstring formatting --- src/darker/verification.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/darker/verification.py b/src/darker/verification.py index f99ae2ad0..26347dcb1 100644 --- a/src/darker/verification.py +++ b/src/darker/verification.py @@ -70,9 +70,10 @@ def verify_ast_unchanged( class ASTVerifier: - """Verifies if reformatted TextDocument is AST-equivalent to baseline. + """Verify if reformatted TextDocument is AST-equivalent to baseline Keeps in-memory data about previous comparisons to improve performance. + """ def __init__(self, baseline: TextDocument) -> None: From e899fccf1861459b7a23c4d375e4a9f60ba1e697 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Thu, 21 Oct 2021 23:49:25 +0300 Subject: [PATCH 6/6] Satisfy Pylint --- src/darker/verification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darker/verification.py b/src/darker/verification.py index 26347dcb1..5522c1cc8 100644 --- a/src/darker/verification.py +++ b/src/darker/verification.py @@ -69,7 +69,7 @@ def verify_ast_unchanged( raise NotEquivalentError(str(exc_info)) -class ASTVerifier: +class ASTVerifier: # pylint: disable=too-few-public-methods """Verify if reformatted TextDocument is AST-equivalent to baseline Keeps in-memory data about previous comparisons to improve performance.