[TECH DEBT] Let LocalFileLinter reuse the Dependency instead falling back on its path (#3641)

## Changes

Let `LocalFileLinter` reuse the `Dependency` instead falling back on its
path as this introduce duplicate ways of loading files.

### Linked issues

Progresses #3514
Breaks up #3520
Stacked on:
- [x] #3640
- [x] #3639
- [x] #3638

### Functionality

- [x] modified existing command: `databricks labs ucx
migrate-local-code`

### Tests

- [ ] manually tested
- [x] modified and added unit tests
- [x] modified and added integration tests

Author: JCZuurmond

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

@@ -32,30 +32,30 @@ class LocalFile(SourceContainer):
     def __init__(self, path: Path, source: str, language: Language):
         self._path = path
         self._original_code = source
-        self._language = language
+        self.language = language
 
     @property
     def content(self) -> str:
-        """The file content"""
+        """The local file content"""
         return self._original_code
 
     def build_dependency_graph(self, parent: DependencyGraph) -> list[DependencyProblem]:
         """The dependency graph for the local file."""
-        if self._language == Language.PYTHON:
+        if self.language == Language.PYTHON:
             context = parent.new_dependency_graph_context()
             analyzer = PythonCodeAnalyzer(context, self._original_code)
             problems = analyzer.build_graph()
             for idx, problem in enumerate(problems):
                 if problem.has_missing_path():
                     problems[idx] = dataclasses.replace(problem, source_path=self._path)
             return problems
-        if self._language == Language.SQL:  # SQL cannot refer other dependencies
+        if self.language == Language.SQL:  # SQL cannot refer other dependencies
             return []
-        logger.warning(f"Unsupported language: {self._language}")
+        logger.warning(f"Unsupported language: {self.language}")
         return []
 
     def build_inherited_context(self, graph: DependencyGraph, child_path: Path) -> InheritedContext:
-        if self._language == Language.PYTHON:
+        if self.language == Language.PYTHON:
             context = graph.new_dependency_graph_context()
             analyzer = PythonCodeAnalyzer(context, self._original_code)
             inherited = analyzer.build_inherited_context(child_path)

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

@@ -16,7 +16,7 @@
 
 from databricks.labs.ucx.hive_metastore.table_migration_status import TableMigrationIndex
 from databricks.labs.ucx.source_code.base import CurrentSessionState
-from databricks.labs.ucx.source_code.graph import DependencyProblem
+from databricks.labs.ucx.source_code.graph import Dependency, DependencyProblem
 from databricks.labs.ucx.source_code.path_lookup import PathLookup
 
 logger = logging.getLogger(__name__)
@@ -175,11 +175,14 @@ def _analyze_file(cls, known_distributions, library_root, dist_info, module_path
         # exist, while KnownList needs FileLinter to analyze the source code. Given that building KnownList falls
         # outside the normal execution path, we can safely import FileLinter here.
         # pylint: disable-next=import-outside-toplevel,cyclic-import
-        from databricks.labs.ucx.source_code.linters.files import FileLinter
+        from databricks.labs.ucx.source_code.files import FileLoader
 
         # pylint: disable-next=import-outside-toplevel,cyclic-import
         from databricks.labs.ucx.source_code.linters.context import LinterContext
 
+        # pylint: disable-next=import-outside-toplevel,cyclic-import
+        from databricks.labs.ucx.source_code.linters.files import FileLinter
+
         empty_index = TableMigrationIndex([])
         relative_path = module_path.relative_to(library_root)
         module_ref = relative_path.as_posix().replace('/', '.')
@@ -189,7 +192,8 @@ def _analyze_file(cls, known_distributions, library_root, dist_info, module_path
         logger.info(f"Processing module: {module_ref}")
         session_state = CurrentSessionState()
         ctx = LinterContext(empty_index, session_state)
-        linter = FileLinter(ctx, PathLookup.from_sys_path(module_path.parent), module_path)
+        dependency = Dependency(FileLoader(), module_path, inherits_context=False)
+        linter = FileLinter(dependency, PathLookup.from_sys_path(module_path.parent), ctx)
         known_problems = set()
         for problem in linter.lint():
             known_problems.add(KnownProblem(problem.code, problem.message))

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

@@ -1,7 +1,6 @@
 from __future__ import annotations  # for type hints
 
 import dataclasses
-import locale
 import logging
 from collections.abc import Iterable
 from pathlib import Path
@@ -11,12 +10,11 @@
 
 from databricks.labs.ucx.source_code.base import (
     file_language,
-    is_a_notebook,
     Advice,
     Failure,
     safe_read_text,
-    read_text,
 )
+from databricks.labs.ucx.source_code.files import LocalFile
 from databricks.labs.ucx.source_code.graph import Dependency
 from databricks.labs.ucx.source_code.linters.base import PythonLinter
 from databricks.labs.ucx.source_code.linters.context import LinterContext
@@ -43,7 +41,7 @@ class NotebookLinter:
     """
 
     def __init__(
-        self, context: LinterContext, path_lookup: PathLookup, notebook: Notebook, parent_tree: Tree | None = None
+        self, notebook: Notebook, path_lookup: PathLookup, context: LinterContext, parent_tree: Tree | None = None
     ):
         self._context: LinterContext = context
         self._path_lookup = path_lookup
@@ -260,76 +258,46 @@ class FileLinter:
 
     def __init__(
         self,
-        context: LinterContext,
+        dependency: Dependency,
         path_lookup: PathLookup,
-        path: Path,
+        context: LinterContext,
         inherited_tree: Tree | None = None,
-        content: str | None = None,
     ):
-        self._context = context
+        self._dependency = dependency
         self._path_lookup = path_lookup
-        self._path = path
+        self._context = context
         self._inherited_tree = inherited_tree
-        self._content = content
 
     def lint(self) -> Iterable[Advice]:
-        encoding = locale.getpreferredencoding(False)
-        try:
-            # Not using `safe_read_text` here to surface read errors
-            self._content = self._content or read_text(self._path)
-        except FileNotFoundError:
-            failure_message = f"File not found: {self._path}"
-            yield Failure("file-not-found", failure_message, 0, 0, 1, 1)
+        """Lint the file."""
+        if self._dependency.path.suffix.lower() in self._IGNORED_SUFFIXES:
             return
-        except UnicodeDecodeError:
-            failure_message = f"File without {encoding} encoding is not supported {self._path}"
-            yield Failure("unsupported-file-encoding", failure_message, 0, 0, 1, 1)
+        if self._dependency.path.name.lower() in self._IGNORED_NAMES:
             return
-        except PermissionError:
-            failure_message = f"Missing read permission for {self._path}"
-            yield Failure("file-permission", failure_message, 0, 0, 1, 1)
+        if self._dependency.path.suffix.lower() in self._NOT_YET_SUPPORTED_SUFFIXES:
+            message = f"Unsupported language for suffix: {self._dependency.path.suffix}"
+            yield Failure("unsupported-language", message, -1, -1, -1, -1)
             return
-
-        if self._is_notebook():
-            yield from self._lint_notebook()
-        else:
-            yield from self._lint_file()
-
-    def _is_notebook(self) -> bool:
-        assert self._content is not None, "Content should be read from path before calling this method"
-        # pre-check to avoid loading unsupported content
-        language = file_language(self._path)
-        if not language:
-            return False
-        return is_a_notebook(self._path, self._content)
-
-    def _lint_file(self) -> Iterable[Advice]:
-        assert self._content is not None, "Content should be read from path before calling this method"
-        language = file_language(self._path)
-        if not language:
-            suffix = self._path.suffix.lower()
-            if suffix in self._IGNORED_SUFFIXES or self._path.name.lower() in self._IGNORED_NAMES:
-                yield from []
-            elif suffix in self._NOT_YET_SUPPORTED_SUFFIXES:
-                yield Failure("unsupported-language", f"Language not supported yet for {self._path}", 0, 0, 1, 1)
-            else:
-                yield Failure("unknown-language", f"Cannot detect language for {self._path}", 0, 0, 1, 1)
+        source_container = self._dependency.load(self._path_lookup)
+        if isinstance(source_container, Notebook):
+            yield from self._lint_notebook(source_container)
+        elif isinstance(source_container, LocalFile):
+            yield from self._lint_file(source_container)
         else:
-            try:
-                linter = self._context.linter(language)
-                yield from linter.lint(self._content)
-            except ValueError as err:
-                failure_message = f"Error while parsing content of {self._path.as_posix()}: {err}"
-                yield Failure("unsupported-content", failure_message, 0, 0, 1, 1)
+            yield Failure("unsupported-file", "Unsupported file", -1, -1, -1, -1)
 
-    def _lint_notebook(self) -> Iterable[Advice]:
-        assert self._content is not None, "Content should be read from path before calling this method"
-        language = file_language(self._path)
-        if not language:
-            yield Failure("unknown-language", f"Cannot detect language for {self._path}", 0, 0, 1, 1)
-            return
-        notebook = Notebook.parse(self._path, self._content, language)
-        notebook_linter = NotebookLinter(self._context, self._path_lookup, notebook, self._inherited_tree)
+    def _lint_file(self, local_file: LocalFile) -> Iterable[Advice]:
+        """Lint a local file."""
+        try:
+            linter = self._context.linter(local_file.language)
+            yield from linter.lint(local_file.content)
+        except ValueError:
+            # TODO: Remove when implementing: https://github.com/databrickslabs/ucx/issues/3544
+            yield Failure("unsupported-language", f"Unsupported language: {local_file.language}", -1, -1, -1, -1)
+
+    def _lint_notebook(self, notebook: Notebook) -> Iterable[Advice]:
+        """Lint a notebook."""
+        notebook_linter = NotebookLinter(notebook, self._path_lookup, self._context, self._inherited_tree)
         yield from notebook_linter.lint()
 
 

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

@@ -109,7 +109,7 @@ def _process_dependency(
         inherited_tree: Tree | None,
     ) -> Iterable[LocatedAdvice]:
         # FileLinter determines which file/notebook linter to use
-        linter = FileLinter(self._context_factory(), path_lookup, dependency.path, inherited_tree)
+        linter = FileLinter(dependency, path_lookup, self._context_factory(), inherited_tree)
         for advice in linter.lint():
             yield LocatedAdvice(advice, dependency.path)
 

tests/unit/source_code/notebooks/test_sources.py

@@ -1,53 +1,42 @@
-import codecs
-import locale
 from pathlib import Path
-from unittest.mock import create_autospec
 
 import pytest
 from databricks.sdk.service.workspace import Language
 
 from databricks.labs.ucx.hive_metastore.table_migration_status import TableMigrationIndex
 from databricks.labs.ucx.source_code.base import Deprecation, Failure
+from databricks.labs.ucx.source_code.files import FileLoader
+from databricks.labs.ucx.source_code.graph import Dependency
 from databricks.labs.ucx.source_code.linters.context import LinterContext
 from databricks.labs.ucx.source_code.linters.files import FileLinter, NotebookLinter
 from databricks.labs.ucx.source_code.notebooks.sources import Notebook
 from databricks.labs.ucx.source_code.path_lookup import PathLookup
 
 
-@pytest.mark.parametrize("path, content", [("xyz.py", "a = 3"), ("xyz.sql", "select * from dual")])
-def test_file_linter_lints_supported_language(path, content, migration_index, mock_path_lookup) -> None:
-    linter = FileLinter(LinterContext(migration_index), mock_path_lookup, Path(path), None, content)
+def test_file_linter_lints_python(tmp_path, migration_index, mock_path_lookup) -> None:
+    path = tmp_path / "xyz.py"
+    path.write_text("a = 3")
+    dependency = Dependency(FileLoader(), path)
+    linter = FileLinter(dependency, mock_path_lookup, LinterContext(migration_index))
     advices = list(linter.lint())
     assert not advices
 
 
-@pytest.mark.parametrize(
-    "bom, encoding",
-    [
-        (codecs.BOM_UTF8, "utf-8"),
-        (codecs.BOM_UTF16_LE, "utf-16-le"),
-        (codecs.BOM_UTF16_BE, "utf-16-be"),
-        (codecs.BOM_UTF32_LE, "utf-32-le"),
-        (codecs.BOM_UTF32_BE, "utf-32-be"),
-    ],
-)
-def test_file_linter_lints_supported_language_encoded_file_with_bom(
-    tmp_path, migration_index, mock_path_lookup, bom, encoding
-) -> None:
-    path = tmp_path / "file.py"
-    path.write_bytes(bom + "a = 12".encode(encoding))
-    linter = FileLinter(LinterContext(migration_index), mock_path_lookup, path, None)
-
+def test_file_linter_lints_sql(tmp_path, migration_index, mock_path_lookup) -> None:
+    path = tmp_path / "xyz.sql"
+    path.write_text("SELECT * FROM dual")
+    dependency = Dependency(FileLoader(), path)
+    linter = FileLinter(dependency, mock_path_lookup, LinterContext(migration_index))
     advices = list(linter.lint())
-
     assert not advices
 
 
 @pytest.mark.parametrize("path", ["xyz.scala", "xyz.r", "xyz.sh"])
 def test_file_linter_lints_not_yet_supported_language(tmp_path, path, migration_index, mock_path_lookup) -> None:
     path = tmp_path / path
     path.touch()
-    linter = FileLinter(LinterContext(migration_index), mock_path_lookup, Path(path), None, "")
+    dependency = Dependency(FileLoader(), path)
+    linter = FileLinter(dependency, mock_path_lookup, LinterContext(migration_index))
     advices = list(linter.lint())
     assert [advice.code for advice in advices] == ["unsupported-language"]
 
@@ -74,49 +63,12 @@ def test_file_linter_lints_not_yet_supported_language(tmp_path, path, migration_
 def test_file_linter_lints_ignorable_language(tmp_path, path, migration_index, mock_path_lookup) -> None:
     path = tmp_path / path
     path.touch()
-    linter = FileLinter(LinterContext(migration_index), mock_path_lookup, Path(path), None)
+    dependency = Dependency(FileLoader(), path)
+    linter = FileLinter(dependency, mock_path_lookup, LinterContext(migration_index))
     advices = list(linter.lint())
     assert not advices
 
 
-def test_file_linter_lints_non_ascii_encoded_file(migration_index, mock_path_lookup) -> None:
-    preferred_encoding = locale.getpreferredencoding(False)
-    non_ascii_encoded_file = Path(__file__).parent.parent / "samples" / "nonascii.py"
-    linter = FileLinter(LinterContext(migration_index), mock_path_lookup, non_ascii_encoded_file)
-
-    advices = list(linter.lint())
-
-    assert len(advices) == 1
-    assert advices[0].code == "unsupported-file-encoding"
-    assert advices[0].message == f"File without {preferred_encoding} encoding is not supported {non_ascii_encoded_file}"
-
-
-def test_file_linter_lints_file_with_missing_file(migration_index, mock_path_lookup) -> None:
-    path = create_autospec(Path)
-    path.suffix = ".py"
-    path.open.side_effect = FileNotFoundError("No such file or directory: 'test.py'")
-    linter = FileLinter(LinterContext(migration_index), mock_path_lookup, path)
-
-    advices = list(linter.lint())
-
-    assert len(advices) == 1
-    assert advices[0].code == "file-not-found"
-    assert advices[0].message == f"File not found: {path}"
-
-
-def test_file_linter_lints_file_with_missing_read_permission(migration_index, mock_path_lookup) -> None:
-    path = create_autospec(Path)
-    path.suffix = ".py"
-    path.open.side_effect = PermissionError("Permission denied")
-    linter = FileLinter(LinterContext(migration_index), mock_path_lookup, path)
-
-    advices = list(linter.lint())
-
-    assert len(advices) == 1
-    assert advices[0].code == "file-permission"
-    assert advices[0].message == f"Missing read permission for {path}"
-
-
 class _NotebookLinter(NotebookLinter):
     """A helper class to construct the notebook linter from source code for testing simplification."""
 
@@ -127,7 +79,7 @@ def from_source_code(
         context = LinterContext(index)
         notebook = Notebook.parse(Path(""), source, default_language)
         assert notebook is not None
-        return cls(context, path_lookup, notebook)
+        return cls(notebook, path_lookup, context)
 
 
 def test_notebook_linter_lints_source_yielding_no_advices(migration_index, mock_path_lookup) -> None:
@@ -198,7 +150,7 @@ def test_notebook_linter_lints_parent_child_context_from_grand_parent(migration_
     """Verify the NotebookLinter can resolve %run"""
     path = Path(__file__).parent.parent / "samples" / "parent-child-context" / "grand_parent.py"
     notebook = Notebook.parse(path, path.read_text(), Language.PYTHON)
-    linter = NotebookLinter(LinterContext(migration_index), mock_path_lookup.change_directory(path.parent), notebook)
+    linter = NotebookLinter(notebook, mock_path_lookup.change_directory(path.parent), LinterContext(migration_index))
 
     advices = list(linter.lint())
 

tests/unit/source_code/test_files.py

@@ -6,10 +6,9 @@
 from databricks.sdk.service.workspace import Language
 
 from databricks.labs.ucx.source_code.base import CurrentSessionState
+from databricks.labs.ucx.source_code.graph import Dependency, DependencyGraph, DependencyProblem
 from databricks.labs.ucx.source_code.files import FileLoader, LocalFile
-from databricks.labs.ucx.source_code.graph import Dependency
 from databricks.labs.ucx.source_code.path_lookup import PathLookup
-from databricks.labs.ucx.source_code.graph import DependencyGraph, DependencyProblem
 
 
 def test_local_file_content_is_accessible() -> None: