linter: only lint on updated lines in the new file (#4409)

openhands/linter/languages/python.py

@@ -1,5 +1,6 @@
 from typing import List
 
+from openhands.core.logger import openhands_logger as logger
 from openhands.linter.base import BaseLinter, LintResult
 from openhands.linter.utils import run_cmd
 
@@ -39,11 +40,31 @@ def flake_lint(filepath: str) -> list[LintResult]:
             _msg = parts[3].strip()
             if len(parts) > 4:
                 _msg += ': ' + parts[4].strip()
+
+            try:
+                line_num = int(parts[1])
+            except ValueError as e:
+                logger.warning(
+                    f'Error parsing flake8 output for line: {e}. Parsed parts: {parts}. Skipping...'
+                )
+                continue
+
+            try:
+                column_num = int(parts[2])
+            except ValueError as e:
+                column_num = 1
+                _msg = (
+                    parts[2].strip() + ' ' + _msg
+                )  # add the unparsed message to the original message
+                logger.warning(
+                    f'Error parsing flake8 output for column: {e}. Parsed parts: {parts}. Using default column 1.'
+                )
+
             results.append(
                 LintResult(
                     file=filepath,
-                    line=int(parts[1]),
-                    column=int(parts[2]),
+                    line=line_num,
+                    column=column_num,
                     message=_msg,
                 )
             )

openhands/linter/linter.py

@@ -1,5 +1,6 @@
 import os
 from collections import defaultdict
+from difflib import SequenceMatcher
 
 from openhands.linter.base import BaseLinter, LinterException, LintResult
 from openhands.linter.languages.python import PythonLinter
@@ -33,3 +34,89 @@ class DefaultLinter(BaseLinter):
             if res:
                 return res
         return []
+
+    def lint_file_diff(
+        self, original_file_path: str, updated_file_path: str
+    ) -> list[LintResult]:
+        """Only return lint errors that are introduced by the diff.
+
+        Args:
+            original_file_path: The original file path.
+            updated_file_path: The updated file path.
+
+        Returns:
+            A list of lint errors that are introduced by the diff.
+        """
+        # 1. Lint the original and updated file
+        original_lint_errors: list[LintResult] = self.lint(original_file_path)
+        updated_lint_errors: list[LintResult] = self.lint(updated_file_path)
+
+        # 2. Load the original and updated file content
+        with open(original_file_path, 'r') as f:
+            old_lines = f.readlines()
+        with open(updated_file_path, 'r') as f:
+            new_lines = f.readlines()
+
+        # 3. Get line numbers that are changed & unchanged
+        # Map the line number of the original file to the updated file
+        # NOTE: this only works for lines that are not changed (i.e., equal)
+        old_to_new_line_no_mapping: dict[int, int] = {}
+        replace_or_inserted_lines: list[int] = []
+        for (
+            tag,
+            old_idx_start,
+            old_idx_end,
+            new_idx_start,
+            new_idx_end,
+        ) in SequenceMatcher(
+            isjunk=None,
+            a=old_lines,
+            b=new_lines,
+        ).get_opcodes():
+            if tag == 'equal':
+                for idx, _ in enumerate(old_lines[old_idx_start:old_idx_end]):
+                    old_to_new_line_no_mapping[old_idx_start + idx + 1] = (
+                        new_idx_start + idx + 1
+                    )
+            elif tag == 'replace' or tag == 'insert':
+                for idx, _ in enumerate(old_lines[old_idx_start:old_idx_end]):
+                    replace_or_inserted_lines.append(new_idx_start + idx + 1)
+            else:
+                # omit the case of delete
+                pass
+
+        # 4. Get pre-existing errors in unchanged lines
+        # increased error elsewhere introduced by the newlines
+        # i.e., we omit errors that are already in original files and report new one
+        new_line_no_to_original_errors: dict[int, list[LintResult]] = defaultdict(list)
+        for error in original_lint_errors:
+            if error.line in old_to_new_line_no_mapping:
+                new_line_no_to_original_errors[
+                    old_to_new_line_no_mapping[error.line]
+                ].append(error)
+
+        # 5. Select errors from lint results in new file to report
+        selected_errors = []
+        for error in updated_lint_errors:
+            # 5.1. Error introduced by replace/insert
+            if error.line in replace_or_inserted_lines:
+                selected_errors.append(error)
+            # 5.2. Error introduced by modified lines that impacted
+            #      the unchanged lines that HAVE pre-existing errors
+            elif error.line in new_line_no_to_original_errors:
+                # skip if the error is already reported
+                # or add if the error is new
+                if not any(
+                    original_error.message == error.message
+                    and original_error.column == error.column
+                    for original_error in new_line_no_to_original_errors[error.line]
+                ):
+                    selected_errors.append(error)
+            # 5.3. Error introduced by modified lines that impacted
+            #      the unchanged lines that have NO pre-existing errors
+            else:
+                selected_errors.append(error)
+
+        # 6. Sort errors by line and column
+        selected_errors.sort(key=lambda x: (x.line, x.column))
+        return selected_errors

openhands/utils/diff.py

@@ -0,0 +1,41 @@
+import difflib
+
+import whatthepatch
+
+
+def get_diff(old_contents: str, new_contents: str, filepath: str = 'file') -> str:
+    diff = list(
+        difflib.unified_diff(
+            old_contents.split('\n'),
+            new_contents.split('\n'),
+            fromfile=filepath,
+            tofile=filepath,
+            # do not output unchange lines
+            # because they can cause `parse_diff` to fail
+            n=0,
+        )
+    )
+    return '\n'.join(map(lambda x: x.rstrip(), diff))
+
+
+def parse_diff(diff_patch: str) -> list[whatthepatch.patch.Change]:
+    # handle empty patch
+    if diff_patch.strip() == '':
+        return []
+
+    patch = whatthepatch.parse_patch(diff_patch)
+    patch_list = list(patch)
+    assert len(patch_list) == 1, (
+        'parse_diff only supports single file diff. But got:\nPATCH:\n'
+        + diff_patch
+        + '\nPATCH LIST:\n'
+        + str(patch_list)
+    )
+    changes = patch_list[0].changes
+
+    # ignore changes that are the same (i.e., old_lineno == new_lineno)
+    output_changes = []
+    for change in changes:
+        if change.old != change.new:
+            output_changes.append(change)
+    return output_changes

poetry.lock

@@ -1,4 +1,4 @@
-# This file is automatically @generated by Poetry 1.8.3 and should not be changed by hand.
+# This file is automatically @generated by Poetry 1.8.2 and should not be changed by hand.
 
 [[package]]
 name = "aenum"
@@ -10001,4 +10001,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"]
 [metadata]
 lock-version = "2.0"
 python-versions = "^3.12"
-content-hash = "f0d6c96fb36fd6ff330f27b0bf8a2051099b1c46f8e3b03d0d530025c87c92af"
+content-hash = "431cd8f4b8a41e6c52c79cd18a55654388c8d15dcdabfa1487dda17cf6caa3e4"

pyproject.toml

@@ -52,6 +52,7 @@ python-pptx = "*"
 pylatexenc = "*"
 tornado = "*"
 python-dotenv = "*"
+whatthepatch = "^1.0.6"
 protobuf = "^4.21.6,<5.0.0" # chromadb currently fails on 5.0+
 opentelemetry-api = "1.25.0"
 opentelemetry-exporter-otlp-proto-grpc = "1.25.0"

tests/unit/linters/test_lint_diff.py

@@ -0,0 +1,417 @@
+from openhands.linter import DefaultLinter, LintResult
+from openhands.utils.diff import get_diff, parse_diff
+
+OLD_CONTENT = """
+def foo():
+    print("Hello, World!")
+    x = UNDEFINED_VARIABLE
+foo()
+"""
+
+NEW_CONTENT_V1 = (
+    OLD_CONTENT
+    + """
+def new_function_that_causes_error():
+    y = ANOTHER_UNDEFINED_VARIABLE
+"""
+)
+
+NEW_CONTENT_V2 = """
+def foo():
+    print("Hello, World!")
+    x = UNDEFINED_VARIABLE
+    y = ANOTHER_UNDEFINED_VARIABLE
+foo()
+"""
+
+
+def test_get_and_parse_diff(tmp_path):
+    diff = get_diff(OLD_CONTENT, NEW_CONTENT_V1, 'test.py')
+    print(diff)
+    assert (
+        diff
+        == """
+--- test.py
++++ test.py
+@@ -6,0 +7,3 @@
++def new_function_that_causes_error():
++    y = ANOTHER_UNDEFINED_VARIABLE
++
+""".strip()
+    )
+
+    print(
+        '\n'.join(
+            [f'{i+1}|{line}' for i, line in enumerate(NEW_CONTENT_V1.splitlines())]
+        )
+    )
+    changes = parse_diff(diff)
+    assert len(changes) == 3
+    assert (
+        changes[0].old is None
+        and changes[0].new == 7
+        and changes[0].line == 'def new_function_that_causes_error():'
+    )
+    assert (
+        changes[1].old is None
+        and changes[1].new == 8
+        and changes[1].line == '    y = ANOTHER_UNDEFINED_VARIABLE'
+    )
+    assert changes[2].old is None and changes[2].new == 9 and changes[2].line == ''
+
+
+def test_lint_with_diff_append(tmp_path):
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(OLD_CONTENT)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(NEW_CONTENT_V1)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    print(result)
+    assert len(result) == 1
+    assert (
+        result[0].line == 8
+        and result[0].column == 9
+        and result[0].message == "F821 undefined name 'ANOTHER_UNDEFINED_VARIABLE'"
+    )
+
+
+def test_lint_with_diff_insert(tmp_path):
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(OLD_CONTENT)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(NEW_CONTENT_V2)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    assert len(result) == 1
+    assert (
+        result[0].line == 5
+        and result[0].column == 9
+        and result[0].message == "F821 undefined name 'ANOTHER_UNDEFINED_VARIABLE'"
+    )
+
+
+def test_lint_with_multiple_changes_and_errors(tmp_path):
+    old_content = """
+def foo():
+    print("Hello, World!")
+    x = 10
+foo()
+"""
+    new_content = """
+def foo():
+    print("Hello, World!")
+    x = UNDEFINED_VARIABLE
+    y = 20
+
+def bar():
+    z = ANOTHER_UNDEFINED_VARIABLE
+    return z + 1
+
+foo()
+bar()
+"""
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(old_content)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    assert len(result) == 2
+    assert (
+        result[0].line == 4
+        and result[0].column == 9
+        and result[0].message == "F821 undefined name 'UNDEFINED_VARIABLE'"
+    )
+    assert (
+        result[1].line == 8
+        and result[1].column == 9
+        and result[1].message == "F821 undefined name 'ANOTHER_UNDEFINED_VARIABLE'"
+    )
+
+
+def test_lint_with_introduced_and_fixed_errors(tmp_path):
+    old_content = """
+x = UNDEFINED_VARIABLE
+y = 10
+"""
+    new_content = """
+x = 5
+y = ANOTHER_UNDEFINED_VARIABLE
+z = UNDEFINED_VARIABLE
+"""
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(old_content)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    assert len(result) == 2
+    assert (
+        result[0].line == 3
+        and result[0].column == 5
+        and result[0].message == "F821 undefined name 'ANOTHER_UNDEFINED_VARIABLE'"
+    )
+    assert (
+        result[1].line == 4
+        and result[1].column == 5
+        and result[1].message == "F821 undefined name 'UNDEFINED_VARIABLE'"
+    )
+
+
+def test_lint_with_multiline_changes(tmp_path):
+    old_content = """
+def complex_function(a, b, c):
+    return (a +
+            b +
+            c)
+"""
+    new_content = """
+def complex_function(a, b, c):
+    return (a +
+            UNDEFINED_VARIABLE +
+            b +
+            c)
+"""
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(old_content)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    assert len(result) == 1
+    assert (
+        result[0].line == 4
+        and result[0].column == 13
+        and result[0].message == "F821 undefined name 'UNDEFINED_VARIABLE'"
+    )
+
+
+def test_lint_with_syntax_error(tmp_path):
+    old_content = """
+def foo():
+    print("Hello, World!")
+"""
+    new_content = """
+def foo():
+    print("Hello, World!"
+"""
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(old_content)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    assert len(result) == 1
+    assert (
+        result[0].line == 3
+        and result[0].column == 11
+        and result[0].message == "E999 SyntaxError: '(' was never closed"
+    )
+
+
+def test_lint_with_docstring_changes(tmp_path):
+    old_content = '''
+def foo():
+    """This is a function."""
+    print("Hello, World!")
+'''
+    new_content = '''
+def foo():
+    """
+    This is a function.
+    It now has a multi-line docstring with an UNDEFINED_VARIABLE.
+    """
+    print("Hello, World!")
+'''
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(old_content)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    assert len(result) == 0  # Linter should ignore changes in docstrings
+
+
+def test_lint_with_multiple_errors_on_same_line(tmp_path):
+    old_content = """
+def foo():
+    print("Hello, World!")
+    x = 10
+foo()
+"""
+    new_content = """
+def foo():
+    print("Hello, World!")
+    x = UNDEFINED_VARIABLE + ANOTHER_UNDEFINED_VARIABLE
+foo()
+"""
+    with open(tmp_path / 'old.py', 'w') as f:
+        f.write(old_content)
+    with open(tmp_path / 'new.py', 'w') as f:
+        f.write(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(tmp_path / 'old.py'),
+        str(tmp_path / 'new.py'),
+    )
+    print(result)
+    assert len(result) == 2
+    assert (
+        result[0].line == 4
+        and result[0].column == 9
+        and result[0].message == "F821 undefined name 'UNDEFINED_VARIABLE'"
+    )
+    assert (
+        result[1].line == 4
+        and result[1].column == 30
+        and result[1].message == "F821 undefined name 'ANOTHER_UNDEFINED_VARIABLE'"
+    )
+
+
+def test_parse_diff_with_empty_patch():
+    diff_patch = ''
+    changes = parse_diff(diff_patch)
+    assert len(changes) == 0
+
+
+def test_lint_file_diff_ignore_existing_errors(tmp_path):
+    """
+    Make sure we allow edits as long as it does not introduce new errors. In other
+    words, we don't care about existing linting errors. Although they might be
+    real syntax issues, sometimes they are just false positives, or errors that
+    we don't care about.
+    """
+    content = """def some_valid_but_weird_function():
+    # this function is legitimate, yet static analysis tools like flake8
+    # reports 'F821 undefined name'
+    if 'variable' in locals():
+        print(variable)
+def some_wrong_but_unused_function():
+    # this function has a linting error, but it is not modified by us, and
+    # who knows, this function might be completely dead code
+    x = 1
+def sum(a, b):
+    return a - b
+"""
+    new_content = content.replace('    return a - b', '    return a + b')
+    temp_file_old_path = tmp_path / 'problematic-file-test.py'
+    temp_file_old_path.write_text(content)
+    temp_file_new_path = tmp_path / 'problematic-file-test-new.py'
+    temp_file_new_path.write_text(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(temp_file_old_path),
+        str(temp_file_new_path),
+    )
+    assert len(result) == 0  # no new errors introduced
+
+
+def test_lint_file_diff_catch_new_errors_in_edits(tmp_path):
+    """
+    Make sure we catch new linting errors in our edit chunk, and at the same
+    time, ignore old linting errors (in this case, the old linting error is
+    a false positive)
+    """
+    content = """def some_valid_but_weird_function():
+    # this function is legitimate, yet static analysis tools like flake8
+    # reports 'F821 undefined name'
+    if 'variable' in locals():
+        print(variable)
+def sum(a, b):
+    return a - b
+"""
+
+    temp_file_old_path = tmp_path / 'problematic-file-test.py'
+    temp_file_old_path.write_text(content)
+    new_content = content.replace('    return a - b', '    return a + variable')
+    temp_file_new_path = tmp_path / 'problematic-file-test-new.py'
+    temp_file_new_path.write_text(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(temp_file_old_path),
+        str(temp_file_new_path),
+    )
+    print(result)
+    assert len(result) == 1
+    assert (
+        result[0].line == 7
+        and result[0].column == 16
+        and result[0].message == "F821 undefined name 'variable'"
+    )
+
+
+def test_lint_file_diff_catch_new_errors_outside_edits(tmp_path):
+    """
+    Make sure we catch new linting errors induced by our edits, even
+    though the error itself is not in the edit chunk
+    """
+    content = """def valid_func1():
+    print(my_sum(1, 2))
+def my_sum(a, b):
+    return a - b
+def valid_func2():
+    print(my_sum(0, 0))
+"""
+    # Add 100 lines of invalid code, which linter shall ignore
+    # because they are not being edited. For testing purpose, we
+    # must add these existing linting errors, otherwise the pre-edit
+    # linting would pass, and thus there won't be any comparison
+    # between pre-edit and post-edit linting.
+    for _ in range(100):
+        content += '\ninvalid_func()'
+
+    temp_file_old_path = tmp_path / 'problematic-file-test.py'
+    temp_file_old_path.write_text(content)
+
+    new_content = content.replace('def my_sum(a, b):', 'def my_sum2(a, b):')
+    temp_file_new_path = tmp_path / 'problematic-file-test-new.py'
+    temp_file_new_path.write_text(new_content)
+
+    linter = DefaultLinter()
+    result: list[LintResult] = linter.lint_file_diff(
+        str(temp_file_old_path),
+        str(temp_file_new_path),
+    )
+    assert len(result) == 2
+    assert (
+        result[0].line == 2
+        and result[0].column == 11
+        and result[0].message == "F821 undefined name 'my_sum'"
+    )
+    assert (
+        result[1].line == 6
+        and result[1].column == 11
+        and result[1].message == "F821 undefined name 'my_sum'"
+    )