Don't complain about unowned string imports

src/python/pants/backend/python/dependency_inference/parse_python_imports.py

@@ -17,11 +17,17 @@
 from pants.util.logging import LogLevel
 
 
-class ParsedPythonImports(FrozenDict[str, int]):
-    """All the discovered imports from a Python source file mapped to the first line they appear.
+@dataclass(frozen=True)
+class ParsedPythonImportInfo:
+    lineno: int
+    # An import is considered "weak" if we're unsure if a dependency will exist between the parsed
+    # file and the parsed import.
+    # Examples of "weak" imports include string imports (if enabled).
+    weak: bool
+
 
-    May include string imports if the request specified to include them.
-    """
+class ParsedPythonImports(FrozenDict[str, ParsedPythonImportInfo]):
+    """All the discovered imports from a Python source file mapped to the relevant info."""
 
 
 @dataclass(frozen=True)
@@ -68,8 +74,10 @@ async def parse_python_imports(request: ParsePythonImportsRequest) -> ParsedPyth
     )
     # See above for where we explicitly encoded as utf8. Even though utf8 is the
     # default for decode(), we make that explicit here for emphasis.
-    process_output = process_result.stdout.decode("utf8")
-    return ParsedPythonImports(json.loads(process_output) if process_output else {})
+    process_output = process_result.stdout.decode("utf8") or "{}"
+    return ParsedPythonImports(
+        (imp, ParsedPythonImportInfo(**info)) for imp, info in json.loads(process_output).items()
+    )
 
 
 def rules():

src/python/pants/backend/python/dependency_inference/parse_python_imports_test.py

@@ -8,6 +8,9 @@
 import pytest
 
 from pants.backend.python.dependency_inference import parse_python_imports
+from pants.backend.python.dependency_inference.parse_python_imports import (
+    ParsedPythonImportInfo as ImpInfo,
+)
 from pants.backend.python.dependency_inference.parse_python_imports import (
     ParsedPythonImports,
     ParsePythonImportsRequest,
@@ -42,7 +45,7 @@ def assert_imports_parsed(
     rule_runner: RuleRunner,
     content: str,
     *,
-    expected: dict[str, int],
+    expected: dict[str, ImpInfo],
     filename: str = "project/foo.py",
     constraints: str = ">=3.6",
     string_imports: bool = True,
@@ -125,23 +128,23 @@ def test_normal_imports(rule_runner: RuleRunner) -> None:
         rule_runner,
         content,
         expected={
-            "__future__.print_function": 1,
-            "os": 3,
-            "os.path": 5,
-            "typing.TYPE_CHECKING": 6,
-            "requests": 8,
-            "demo": 10,
-            "project.demo.Demo": 11,
-            "project.demo.OriginalName": 12,
-            "multiline_import1.not_ignored1": 16,
-            "multiline_import1.not_ignored2": 23,
-            "multiline_import2.not_ignored": 26,
-            "project.circular_dep.CircularDep": 29,
-            "subprocess": 32,
-            "subprocess23": 34,
-            "pkg_resources": 36,
-            "not_ignored_but_looks_like_it_could_be": 39,
-            "also_not_ignored_but_looks_like_it_could_be": 45,
+            "__future__.print_function": ImpInfo(lineno=1, weak=False),
+            "os": ImpInfo(lineno=3, weak=False),
+            "os.path": ImpInfo(lineno=5, weak=False),
+            "typing.TYPE_CHECKING": ImpInfo(lineno=6, weak=False),
+            "requests": ImpInfo(lineno=8, weak=False),
+            "demo": ImpInfo(lineno=10, weak=False),
+            "project.demo.Demo": ImpInfo(lineno=11, weak=False),
+            "project.demo.OriginalName": ImpInfo(lineno=12, weak=False),
+            "multiline_import1.not_ignored1": ImpInfo(lineno=16, weak=False),
+            "multiline_import1.not_ignored2": ImpInfo(lineno=23, weak=False),
+            "multiline_import2.not_ignored": ImpInfo(lineno=26, weak=False),
+            "project.circular_dep.CircularDep": ImpInfo(lineno=29, weak=False),
+            "subprocess": ImpInfo(lineno=32, weak=False),
+            "subprocess23": ImpInfo(lineno=34, weak=False),
+            "pkg_resources": ImpInfo(lineno=36, weak=False),
+            "not_ignored_but_looks_like_it_could_be": ImpInfo(lineno=39, weak=False),
+            "also_not_ignored_but_looks_like_it_could_be": ImpInfo(lineno=45, weak=False),
         },
     )
 
@@ -165,12 +168,12 @@ def test_relative_imports(rule_runner: RuleRunner, basename: str) -> None:
         content,
         filename=f"project/util/{basename}",
         expected={
-            "project.util.sibling": 1,
-            "project.util.sibling.Nibling": 2,
-            "project.util.subdir.child.Child": 3,
-            "project.parent.Parent": 4,
-            "project.parent.Parent1": 6,
-            "project.parent.Guardian": 7,
+            "project.util.sibling": ImpInfo(lineno=1, weak=False),
+            "project.util.sibling.Nibling": ImpInfo(lineno=2, weak=False),
+            "project.util.subdir.child.Child": ImpInfo(lineno=3, weak=False),
+            "project.parent.Parent": ImpInfo(lineno=4, weak=False),
+            "project.parent.Parent1": ImpInfo(lineno=6, weak=False),
+            "project.parent.Guardian": ImpInfo(lineno=7, weak=False),
         },
     )
 
@@ -210,24 +213,32 @@ def test_imports_from_strings(rule_runner: RuleRunner, min_dots: int) -> None:
     )
 
     potentially_valid = {
-        "a.b": 3,
-        "a.Foo": 4,
-        "a.b.d": 5,
-        "a.b2.d": 6,
-        "a.b.c.Foo": 7,
-        "a.b.c.d.Foo": 8,
-        "a.b.c.d.FooBar": 9,
-        "a.b.c.d.e.f.g.Baz": 10,
-        "a.b_c.d._bar": 11,
-        "a.b2.c.D": 12,
-        "a.b.c_狗": 13,
+        "a.b": ImpInfo(lineno=3, weak=True),
+        "a.Foo": ImpInfo(lineno=4, weak=True),
+        "a.b.d": ImpInfo(lineno=5, weak=True),
+        "a.b2.d": ImpInfo(lineno=6, weak=True),
+        "a.b.c.Foo": ImpInfo(lineno=7, weak=True),
+        "a.b.c.d.Foo": ImpInfo(lineno=8, weak=True),
+        "a.b.c.d.FooBar": ImpInfo(lineno=9, weak=True),
+        "a.b.c.d.e.f.g.Baz": ImpInfo(lineno=10, weak=True),
+        "a.b_c.d._bar": ImpInfo(lineno=11, weak=True),
+        "a.b2.c.D": ImpInfo(lineno=12, weak=True),
+        "a.b.c_狗": ImpInfo(lineno=13, weak=True),
     }
-    expected = {sym: line for sym, line in potentially_valid.items() if sym.count(".") >= min_dots}
+    expected = {sym: info for sym, info in potentially_valid.items() if sym.count(".") >= min_dots}
 
     assert_imports_parsed(rule_runner, content, expected=expected, string_imports_min_dots=min_dots)
     assert_imports_parsed(rule_runner, content, string_imports=False, expected={})
 
 
+def test_real_import_beats_string_import(rule_runner: RuleRunner) -> None:
+    assert_imports_parsed(
+        rule_runner,
+        "import one.two.three; 'one.two.three'",
+        expected={"one.two.three": ImpInfo(lineno=1, weak=False)},
+    )
+
+
 def test_gracefully_handle_syntax_errors(rule_runner: RuleRunner) -> None:
     assert_imports_parsed(rule_runner, "x =", expected={})
 
@@ -261,13 +272,13 @@ def test_works_with_python2(rule_runner: RuleRunner) -> None:
         content,
         constraints="==2.7.*",
         expected={
-            "demo": 4,
-            "project.demo.Demo": 5,
-            "pkg_resources": 7,
-            "treat.as.a.regular.import.not.a.string.import": 8,
-            "dep.from.bytes": 10,
-            "dep.from.str": 11,
-            "dep.from.str_狗": 12,
+            "demo": ImpInfo(lineno=4, weak=False),
+            "project.demo.Demo": ImpInfo(lineno=5, weak=False),
+            "pkg_resources": ImpInfo(lineno=7, weak=False),
+            "treat.as.a.regular.import.not.a.string.import": ImpInfo(lineno=8, weak=False),
+            "dep.from.bytes": ImpInfo(lineno=10, weak=True),
+            "dep.from.str": ImpInfo(lineno=11, weak=True),
+            "dep.from.str_狗": ImpInfo(lineno=12, weak=True),
         },
     )
 
@@ -294,11 +305,11 @@ def test_works_with_python38(rule_runner: RuleRunner) -> None:
         content,
         constraints=">=3.8",
         expected={
-            "demo": 5,
-            "project.demo.Demo": 6,
-            "pkg_resources": 8,
-            "treat.as.a.regular.import.not.a.string.import": 9,
-            "dep.from.str": 11,
+            "demo": ImpInfo(lineno=5, weak=False),
+            "project.demo.Demo": ImpInfo(lineno=6, weak=False),
+            "pkg_resources": ImpInfo(lineno=8, weak=False),
+            "treat.as.a.regular.import.not.a.string.import": ImpInfo(lineno=9, weak=False),
+            "dep.from.str": ImpInfo(lineno=11, weak=True),
         },
     )
 
@@ -327,10 +338,10 @@ def test_works_with_python39(rule_runner: RuleRunner) -> None:
         content,
         constraints=">=3.9",
         expected={
-            "demo": 7,
-            "project.demo.Demo": 8,
-            "pkg_resources": 10,
-            "treat.as.a.regular.import.not.a.string.import": 11,
-            "dep.from.str": 13,
+            "demo": ImpInfo(lineno=7, weak=False),
+            "project.demo.Demo": ImpInfo(lineno=8, weak=False),
+            "pkg_resources": ImpInfo(lineno=10, weak=False),
+            "treat.as.a.regular.import.not.a.string.import": ImpInfo(lineno=11, weak=False),
+            "dep.from.str": ImpInfo(lineno=13, weak=True),
         },
     )

src/python/pants/backend/python/dependency_inference/rules.py

@@ -172,7 +172,7 @@ async def infer_python_dependencies_via_imports(
         return InferredDependencies([])
 
     wrapped_tgt = await Get(WrappedTarget, Address, request.sources_field.address)
-    explicitly_provided_deps, detected_imports = await MultiGet(
+    explicitly_provided_deps, parsed_imports = await MultiGet(
         Get(ExplicitlyProvidedDependencies, DependenciesRequest(wrapped_tgt.target[Dependencies])),
         Get(
             ParsedPythonImports,
@@ -186,14 +186,13 @@ async def infer_python_dependencies_via_imports(
     )
 
     owners_per_import = await MultiGet(
-        Get(PythonModuleOwners, PythonModule(imported_module))
-        for imported_module in detected_imports.keys()
+        Get(PythonModuleOwners, PythonModule(imported_module)) for imported_module in parsed_imports
     )
 
     merged_result: set[Address] = set()
     unowned_imports: set[str] = set()
     address = wrapped_tgt.target.address
-    for owners, imp in zip(owners_per_import, detected_imports):
+    for owners, imp in zip(owners_per_import, parsed_imports):
         merged_result.update(owners.unambiguous)
         explicitly_provided_deps.maybe_warn_of_ambiguous_dependency_inference(
             owners.ambiguous,
@@ -205,13 +204,17 @@ async def infer_python_dependencies_via_imports(
         if maybe_disambiguated:
             merged_result.add(maybe_disambiguated)
 
-        if not owners.unambiguous and imp.split(".")[0] not in DEFAULT_UNOWNED_DEPENDENCIES:
+        if (
+            not owners.unambiguous
+            and imp.split(".")[0] not in DEFAULT_UNOWNED_DEPENDENCIES
+            and not parsed_imports[imp].weak
+        ):
             unowned_imports.add(imp)
 
     unowned_dependency_behavior = python_infer_subsystem.unowned_dependency_behavior
     if unowned_imports and unowned_dependency_behavior is not UnownedDependencyUsage.DoNothing:
         unowned_imports_with_lines = [
-            f"{module_name} ({request.sources_field.file_path}:{detected_imports[module_name]})"
+            f"{module_name} ({request.sources_field.file_path}:{parsed_imports[module_name].lineno})"
             for module_name in sorted(unowned_imports)
         ]
         raise_error = unowned_dependency_behavior is UnownedDependencyUsage.RaiseError

src/python/pants/backend/python/dependency_inference/rules_test.py

@@ -274,7 +274,12 @@ def test_infer_python_strict(caplog) -> None:
 
     rule_runner.write_files(
         {
-            "src/python/cheesey.py": "import venezuelan_beaver_cheese",
+            "src/python/cheesey.py": dedent(
+                """\
+                    import venezuelan_beaver_cheese
+                    "japanese.sage.derby"
+                """
+            ),
             "src/python/BUILD": "python_sources()",
         }
     )
@@ -286,6 +291,7 @@ def run_dep_inference(
         rule_runner.set_options(
             [
                 f"--python-infer-unowned-dependency-behavior={unowned_dependency_behavior}",
+                "--python-infer-string-imports",
                 "--source-root-patterns=src/python",
             ],
             env_inherit={"PATH", "PYENV_ROOT", "HOME"},
@@ -301,6 +307,7 @@ def run_dep_inference(
     assert len(caplog.records) == 1
     assert "The following imports in src/python/cheesey.py have no owners:" in caplog.text
     assert "  * venezuelan_beaver_cheese (src/python/cheesey.py:1)" in caplog.text
+    assert "japanese.sage.derby" not in caplog.text
 
     # Now test with "error"
     caplog.clear()
@@ -311,6 +318,7 @@ def run_dep_inference(
     assert len(caplog.records) == 2  # one for the error being raised and one for our message
     assert "The following imports in src/python/cheesey.py have no owners:" in caplog.text
     assert "  * venezuelan_beaver_cheese (src/python/cheesey.py:1)" in caplog.text
+    assert "japanese.sage.derby" not in caplog.text
 
     caplog.clear()