Consistent 0-based line tracking for linters (#1855)

Author: asnare

src/databricks/labs/ucx/source_code/base.py

@@ -5,6 +5,9 @@
 from dataclasses import dataclass
 from pathlib import Path
 
+from astroid import NodeNG  # type: ignore
+from typing_extensions import Self
+
 
 # Code mapping between LSP, PyLint, and our own diagnostics:
 # | LSP                       | PyLint     | Our            |
@@ -21,21 +24,22 @@
 class Advice:
     code: str
     message: str
+    # Lines and columns are both 0-based: the first line is line 0.
     start_line: int
     start_col: int
     end_line: int
     end_col: int
 
     def replace(
-        self,
+        self: Self,
         code: str | None = None,
         message: str | None = None,
         start_line: int | None = None,
         start_col: int | None = None,
         end_line: int | None = None,
         end_col: int | None = None,
-    ) -> 'Advice':
-        return self.__class__(
+    ) -> Self:
+        return type(self)(
             code=code if code is not None else self.code,
             message=message if message is not None else self.message,
             start_line=start_line if start_line is not None else self.start_line,
@@ -59,6 +63,27 @@ def as_convention(self) -> 'Convention':
     def for_path(self, path: Path) -> LocatedAdvice:
         return LocatedAdvice(self, path)
 
+    @classmethod
+    def from_node(cls, code: str, message: str, node: NodeNG) -> Self:
+        # Astroid lines are 1-based.
+        return cls(
+            code=code,
+            message=message,
+            start_line=(node.lineno or 1) - 1,
+            start_col=node.col_offset or 0,
+            end_line=(node.end_lineno or 1) - 1,
+            end_col=node.end_col_offset,
+        )
+
+    def replace_from_node(self, node: NodeNG) -> Self:
+        # Astroid lines are 1-based.
+        return self.replace(
+            start_line=(node.lineno or 1) - 1,
+            start_col=node.col_offset,
+            end_line=(node.end_lineno or 1) - 1,
+            end_col=node.end_col_offset,
+        )
+
 
 @dataclass
 class LocatedAdvice:

src/databricks/labs/ucx/source_code/graph.py

@@ -4,8 +4,8 @@
 from dataclasses import dataclass
 from pathlib import Path
 from collections.abc import Callable
-from typing import cast
-from astroid import ImportFrom  # type: ignore
+
+from astroid import ImportFrom, NodeNG  # type: ignore
 
 from databricks.labs.ucx.source_code.base import Advisory
 from databricks.labs.ucx.source_code.linters.imports import (
@@ -161,20 +161,28 @@ def visit(self, visit_node: Callable[[DependencyGraph], bool | None], visited: s
         return False
 
     def build_graph_from_python_source(self, python_code: str) -> list[DependencyProblem]:
+        """Check python code for dependency-related problems.
+
+        Returns:
+            A list of dependency problems; position information is relative to the python code itself.
+        """
         problems: list[DependencyProblem] = []
         tree = Tree.parse(python_code)
         syspath_changes = SysPathChange.extract_from_tree(tree)
         run_calls = DbutilsLinter.list_dbutils_notebook_run_calls(tree)
-        import_sources, import_problems = ImportSource.extract_from_tree(tree, DependencyProblem)
-        problems.extend(cast(list[DependencyProblem], import_problems))
+        import_sources: list[ImportSource]
+        import_problems: list[DependencyProblem]
+        import_sources, import_problems = ImportSource.extract_from_tree(tree, DependencyProblem.from_node)
+        problems.extend(import_problems)
         nodes = syspath_changes + run_calls + import_sources
         # need to execute things in intertwined sequence so concat and sort
-        for base_node in sorted(nodes, key=lambda node: node.node.lineno * 10000 + node.node.col_offset):
+        for base_node in sorted(nodes, key=lambda node: (node.node.lineno, node.node.col_offset)):
             for problem in self._process_node(base_node):
+                # Astroid line numbers are 1-based.
                 problem = problem.replace(
-                    start_line=base_node.node.lineno,
+                    start_line=base_node.node.lineno - 1,
                     start_col=base_node.node.col_offset,
-                    end_line=base_node.node.end_lineno or 0,
+                    end_line=(base_node.node.end_lineno or 1) - 1,
                     end_col=base_node.node.end_col_offset or 0,
                 )
                 problems.append(problem)
@@ -386,6 +394,7 @@ class DependencyProblem:
     code: str
     message: str
     source_path: Path = Path(MISSING_SOURCE_PATH)
+    # Lines and colums are both 0-based: the first line is line 0.
     start_line: int = -1
     start_col: int = -1
     end_line: int = -1
@@ -424,6 +433,18 @@ def as_advisory(self) -> 'Advisory':
             end_col=self.end_col,
         )
 
+    @staticmethod
+    def from_node(code: str, message: str, node: NodeNG) -> DependencyProblem:
+        # Astroid line numbers are 1-based.
+        return DependencyProblem(
+            code=code,
+            message=message,
+            start_line=(node.lineno or 1) - 1,
+            start_col=node.col_offset,
+            end_line=(node.end_lineno or 1) - 1,
+            end_col=(node.end_col_offset),
+        )
+
 
 @dataclass
 class MaybeGraph:

src/databricks/labs/ucx/source_code/linters/dbfs.py

@@ -17,23 +17,17 @@ class DetectDbfsVisitor(TreeVisitor):
     def __init__(self):
         self._advices: list[Advice] = []
         self._fs_prefixes = ["/dbfs/mnt", "dbfs:/", "/mnt/"]
-        self._reported_locations = set()  # Set to store reported locations
+        self._reported_locations = set()  # Set to store reported locations; astroid coordinates!
 
     def visit_call(self, node: Call):
         for arg in node.args:
             if isinstance(arg, Const) and isinstance(arg.value, str):
                 value = arg.value
                 if any(value.startswith(prefix) for prefix in self._fs_prefixes):
-                    self._advices.append(
-                        Deprecation(
-                            code='dbfs-usage',
-                            message=f"Deprecated file system path in call to: {value}",
-                            start_line=arg.lineno,
-                            start_col=arg.col_offset,
-                            end_line=arg.lineno,
-                            end_col=arg.col_offset + len(value),
-                        )
+                    deprecation = Deprecation.from_node(
+                        code='dbfs-usage', message=f"Deprecated file system path in call to: {value}", node=arg
                     )
+                    self._advices.append(deprecation.replace(end_col=arg.col_offset + len(value)))
                     # Record the location of the reported constant, so we do not double report
                     self._reported_locations.add((arg.lineno, arg.col_offset))
 
@@ -47,16 +41,10 @@ def _check_str_constant(self, node: Const):
         if (node.lineno, node.col_offset) not in self._reported_locations:
             value = node.value
             if any(value.startswith(prefix) for prefix in self._fs_prefixes):
-                self._advices.append(
-                    Advisory(
-                        code='dbfs-usage',
-                        message=f"Possible deprecated file system path: {value}",
-                        start_line=node.lineno,
-                        start_col=node.col_offset,
-                        end_line=node.lineno,
-                        end_col=node.col_offset + len(value),
-                    )
+                advisory = Advisory.from_node(
+                    code='dbfs-usage', message=f"Possible deprecated file system path: {value}", node=node
                 )
+                self._advices.append(advisory.replace(end_col=node.col_offset + len(value)))
 
     def get_advices(self) -> Iterable[Advice]:
         yield from self._advices

src/databricks/labs/ucx/source_code/linters/imports.py

@@ -2,7 +2,7 @@
 
 import abc
 import logging
-from collections.abc import Iterable, Callable
+from collections.abc import Callable, Iterable
 from typing import TypeVar, cast
 
 from astroid import (  # type: ignore
@@ -20,12 +20,15 @@
 
 logger = logging.getLogger(__name__)
 
+P = TypeVar("P")
+ProblemFactory = Callable[[str, str, NodeNG], P]
+
 
 class ImportSource(NodeBase):
 
     @classmethod
-    def extract_from_tree(cls, tree: Tree, problem_type: T) -> tuple[list[ImportSource], list[T]]:
-        problems: list[T] = []
+    def extract_from_tree(cls, tree: Tree, problem_factory: ProblemFactory) -> tuple[list[ImportSource], list[P]]:
+        problems: list[P] = []
         sources: list[ImportSource] = []
         try:  # pylint: disable=too-many-try-statements
             nodes = tree.locate(Import, [])
@@ -36,11 +39,11 @@ def extract_from_tree(cls, tree: Tree, problem_type: T) -> tuple[list[ImportSour
                 sources.append(source)
             nodes = tree.locate(Call, [("import_module", Attribute), ("importlib", Name)])
             nodes.extend(tree.locate(Call, [("__import__", Attribute), ("importlib", Name)]))
-            for source in cls._make_sources_for_import_call_nodes(nodes, problem_type, problems):
+            for source in cls._make_sources_for_import_call_nodes(nodes, problem_factory, problems):
                 sources.append(source)
             return sources, problems
         except Exception as e:  # pylint: disable=broad-except
-            problem = problem_type('internal-error', f"While checking imports: {e}")
+            problem = problem_factory('internal-error', f"While checking imports: {e}", tree.root)
             problems.append(problem)
             return [], problems
 
@@ -57,19 +60,14 @@ def _make_sources_for_import_from_nodes(cls, nodes: list[ImportFrom]) -> Iterabl
             yield ImportSource(node, node.modname)
 
     @classmethod
-    def _make_sources_for_import_call_nodes(cls, nodes: list[Call], problem_type: T, problems: list[T]):
+    def _make_sources_for_import_call_nodes(cls, nodes: list[Call], problem_factory: ProblemFactory, problems: list[P]):
         for node in nodes:
             arg = node.args[0]
             if isinstance(arg, Const):
                 yield ImportSource(node, arg.value)
                 continue
-            problem = problem_type(
-                'dependency-not-constant',
-                "Can't check dependency not provided as a constant",
-                start_line=node.lineno,
-                start_col=node.col_offset,
-                end_line=node.end_lineno or 0,
-                end_col=node.end_col_offset or 0,
+            problem = problem_factory(
+                'dependency-not-constant', "Can't check dependency not provided as a constant", node
             )
             problems.append(problem)
 
@@ -91,9 +89,6 @@ def get_notebook_path(self) -> str | None:
         return None
 
 
-T = TypeVar("T", bound=Callable)
-
-
 class DbutilsLinter(Linter):
 
     def lint(self, code: str) -> Iterable[Advice]:
@@ -106,21 +101,15 @@ def _convert_dbutils_notebook_run_to_advice(cls, node: NodeNG) -> Advisory:
         assert isinstance(node, Call)
         path = cls.get_dbutils_notebook_run_path_arg(node)
         if isinstance(path, Const):
-            return Advisory(
+            return Advisory.from_node(
                 'dbutils-notebook-run-literal',
                 "Call to 'dbutils.notebook.run' will be migrated automatically",
-                node.lineno,
-                node.col_offset,
-                node.end_lineno or 0,
-                node.end_col_offset or 0,
+                node=node,
             )
-        return Advisory(
+        return Advisory.from_node(
             'dbutils-notebook-run-dynamic',
             "Path for 'dbutils.notebook.run' is not a constant and requires adjusting the notebook path",
-            node.lineno,
-            node.col_offset,
-            node.end_lineno or 0,
-            node.end_col_offset or 0,
+            node=node,
         )
 
     @staticmethod

src/databricks/labs/ucx/source_code/linters/pyspark.py

@@ -71,20 +71,12 @@ def lint(self, from_table: FromTable, index: MigrationIndex, node: Call) -> Iter
         table_arg = self._get_table_arg(node)
         if isinstance(table_arg, Const):
             for advice in from_table.lint(table_arg.value):
-                yield advice.replace(
-                    start_line=node.lineno,
-                    start_col=node.col_offset,
-                    end_line=node.end_lineno,
-                    end_col=node.end_col_offset,
-                )
+                yield advice.replace_from_node(node)
         else:
-            yield Advisory(
+            yield Advisory.from_node(
                 code='table-migrate',
                 message=f"Can't migrate '{node}' because its table name argument is not a constant",
-                start_line=node.lineno,
-                start_col=node.col_offset,
-                end_line=node.end_lineno or 0,
-                end_col=node.end_col_offset or 0,
+                node=node,
             )
 
     def apply(self, from_table: FromTable, index: MigrationIndex, node: Call) -> None:
@@ -102,28 +94,22 @@ def lint(self, from_table: FromTable, index: MigrationIndex, node: Call) -> Iter
 
         if not isinstance(table_arg, Const):
             assert isinstance(node.func, Attribute)  # always true, avoids a pylint warning
-            yield Advisory(
+            yield Advisory.from_node(
                 code='table-migrate',
                 message=f"Can't migrate '{node.func.attrname}' because its table name argument is not a constant",
-                start_line=node.lineno,
-                start_col=node.col_offset,
-                end_line=node.end_lineno or 0,
-                end_col=node.end_col_offset or 0,
+                node=node,
             )
             return
 
         dst = self._find_dest(index, table_arg.value, from_table.schema)
         if dst is None:
             return
 
-        yield Deprecation(
+        yield Deprecation.from_node(
             code='table-migrate',
             message=f"Table {table_arg.value} is migrated to {dst.destination()} in Unity Catalog",
             # SQLGlot does not propagate tokens yet. See https://github.com/tobymao/sqlglot/issues/3159
-            start_line=node.lineno,
-            start_col=node.col_offset,
-            end_line=node.end_lineno or 0,
-            end_col=node.end_col_offset or 0,
+            node=node,
         )
 
     def apply(self, from_table: FromTable, index: MigrationIndex, node: Call) -> None:
@@ -150,13 +136,10 @@ def matches(self, node: NodeNG):
 
     def lint(self, from_table: FromTable, index: MigrationIndex, node: Call) -> Iterator[Advice]:
         assert isinstance(node.func, Attribute)  # always true, avoids a pylint warning
-        yield Advisory(
+        yield Advisory.from_node(
             code='table-migrate',
             message=f"Call to '{node.func.attrname}' will return a list of <catalog>.<database>.<table> instead of <database>.<table>.",
-            start_line=node.lineno,
-            start_col=node.col_offset,
-            end_line=node.end_lineno or 0,
-            end_col=node.end_col_offset or 0,
+            node=node,
         )
 
     def apply(self, from_table: FromTable, index: MigrationIndex, node: Call) -> None:
@@ -191,23 +174,17 @@ def lint(self, from_table: FromTable, index: MigrationIndex, node: NodeNG) -> It
         if not isinstance(table_arg.value, str):
             return
         if any(table_arg.value.startswith(prefix) for prefix in self._DIRECT_FS_REFS):
-            yield Deprecation(
+            yield Deprecation.from_node(
                 code='direct-filesystem-access',
                 message=f"The use of direct filesystem references is deprecated: {table_arg.value}",
-                start_line=node.lineno,
-                start_col=node.col_offset,
-                end_line=node.end_lineno or 0,
-                end_col=node.end_col_offset or 0,
+                node=node,
             )
             return
         if table_arg.value.startswith("/") and self._check_call_context(node):
-            yield Deprecation(
+            yield Deprecation.from_node(
                 code='direct-filesystem-access',
                 message=f"The use of default dbfs: references is deprecated: {table_arg.value}",
-                start_line=node.lineno,
-                start_col=node.col_offset,
-                end_line=node.end_lineno or 0,
-                end_col=node.end_col_offset or 0,
+                node=node,
             )
 
     def apply(self, from_table: FromTable, index: MigrationIndex, node: Call) -> None: