Handle typing collisions and add validation to a files module for overlaping declarations (#582)

* Fix 'typing' import collisions.

* Fix formatting.

* Fix self-test issues.

* Validation for modules, different typing configurations

* add readme

* make warning

* fix format

---------

Co-authored-by: Scott Hendricks <scott.hendricks@confluent.io>

tests/generate.py

@@ -108,6 +108,7 @@ async def generate_test_case_output(
         print(
             f"\033[31;1;4mFailed to generate reference output for {test_case_name!r}\033[0m"
         )
+        print(ref_err.decode())
 
     if verbose:
         if ref_out:
@@ -126,6 +127,7 @@ async def generate_test_case_output(
         print(
             f"\033[31;1;4mFailed to generate plugin output for {test_case_name!r}\033[0m"
         )
+        print(plg_err.decode())
 
     if verbose:
         if plg_out:
@@ -146,6 +148,7 @@ async def generate_test_case_output(
         print(
             f"\033[31;1;4mFailed to generate plugin (pydantic compatible) output for {test_case_name!r}\033[0m"
         )
+        print(plg_err_pyd.decode())
 
     if verbose:
         if plg_out_pyd:

tests/test_get_ref_type.py

@@ -4,6 +4,15 @@ from betterproto.compile.importing import (
     get_type_reference,
     parse_source_type_name,
 )
+from betterproto.plugin.typing_compiler import DirectImportTypingCompiler
+
+
+@pytest.fixture
+def typing_compiler() -> DirectImportTypingCompiler:
+    """
+    Generates a simple Direct Import Typing Compiler for testing.
+    """
+    return DirectImportTypingCompiler()
 
 
 @pytest.mark.parametrize(
@@ -32,11 +41,18 @@ from betterproto.compile.importing import (
     ],
 )
 def test_reference_google_wellknown_types_non_wrappers(
-    google_type: str, expected_name: str, expected_import: str
+    google_type: str,
+    expected_name: str,
+    expected_import: str,
+    typing_compiler: DirectImportTypingCompiler,
 ):
     imports = set()
     name = get_type_reference(
-        package="", imports=imports, source_type=google_type, pydantic=False
+        package="",
+        imports=imports,
+        source_type=google_type,
+        typing_compiler=typing_compiler,
+        pydantic=False,
     )
 
     assert name == expected_name
@@ -71,11 +87,18 @@ def test_reference_google_wellknown_types_non_wrappers(
     ],
 )
 def test_reference_google_wellknown_types_non_wrappers_pydantic(
-    google_type: str, expected_name: str, expected_import: str
+    google_type: str,
+    expected_name: str,
+    expected_import: str,
+    typing_compiler: DirectImportTypingCompiler,
 ):
     imports = set()
     name = get_type_reference(
-        package="", imports=imports, source_type=google_type, pydantic=True
+        package="",
+        imports=imports,
+        source_type=google_type,
+        typing_compiler=typing_compiler,
+        pydantic=True,
     )
 
     assert name == expected_name
@@ -99,10 +122,15 @@ def test_reference_google_wellknown_types_non_wrappers_pydantic(
     ],
 )
 def test_referenceing_google_wrappers_unwraps_them(
-    google_type: str, expected_name: str
+    google_type: str, expected_name: str, typing_compiler: DirectImportTypingCompiler
 ):
     imports = set()
-    name = get_type_reference(package="", imports=imports, source_type=google_type)
+    name = get_type_reference(
+        package="",
+        imports=imports,
+        source_type=google_type,
+        typing_compiler=typing_compiler,
+    )
 
     assert name == expected_name
     assert imports == set()
@@ -135,223 +163,321 @@ def test_referenceing_google_wrappers_unwraps_them(
     ],
 )
 def test_referenceing_google_wrappers_without_unwrapping(
-    google_type: str, expected_name: str
+    google_type: str, expected_name: str, typing_compiler: DirectImportTypingCompiler
 ):
     name = get_type_reference(
-        package="", imports=set(), source_type=google_type, unwrap=False
+        package="",
+        imports=set(),
+        source_type=google_type,
+        typing_compiler=typing_compiler,
+        unwrap=False,
     )
 
     assert name == expected_name
 
 
-def test_reference_child_package_from_package():
+def test_reference_child_package_from_package(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
-        package="package", imports=imports, source_type="package.child.Message"
+        package="package",
+        imports=imports,
+        source_type="package.child.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from . import child"}
     assert name == '"child.Message"'
 
 
-def test_reference_child_package_from_root():
+def test_reference_child_package_from_root(typing_compiler: DirectImportTypingCompiler):
     imports = set()
-    name = get_type_reference(package="", imports=imports, source_type="child.Message")
+    name = get_type_reference(
+        package="",
+        imports=imports,
+        source_type="child.Message",
+        typing_compiler=typing_compiler,
+    )
 
     assert imports == {"from . import child"}
     assert name == '"child.Message"'
 
 
-def test_reference_camel_cased():
+def test_reference_camel_cased(typing_compiler: DirectImportTypingCompiler):
     imports = set()
     name = get_type_reference(
-        package="", imports=imports, source_type="child_package.example_message"
+        package="",
+        imports=imports,
+        source_type="child_package.example_message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from . import child_package"}
     assert name == '"child_package.ExampleMessage"'
 
 
-def test_reference_nested_child_from_root():
+def test_reference_nested_child_from_root(typing_compiler: DirectImportTypingCompiler):
     imports = set()
     name = get_type_reference(
-        package="", imports=imports, source_type="nested.child.Message"
+        package="",
+        imports=imports,
+        source_type="nested.child.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from .nested import child as nested_child"}
     assert name == '"nested_child.Message"'
 
 
-def test_reference_deeply_nested_child_from_root():
+def test_reference_deeply_nested_child_from_root(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
-        package="", imports=imports, source_type="deeply.nested.child.Message"
+        package="",
+        imports=imports,
+        source_type="deeply.nested.child.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from .deeply.nested import child as deeply_nested_child"}
     assert name == '"deeply_nested_child.Message"'
 
 
-def test_reference_deeply_nested_child_from_package():
+def test_reference_deeply_nested_child_from_package(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
         package="package",
         imports=imports,
         source_type="package.deeply.nested.child.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from .deeply.nested import child as deeply_nested_child"}
     assert name == '"deeply_nested_child.Message"'
 
 
-def test_reference_root_sibling():
-    imports = set()
-    name = get_type_reference(package="", imports=imports, source_type="Message")
-
-    assert imports == set()
-    assert name == '"Message"'
-
-
-def test_reference_nested_siblings():
-    imports = set()
-    name = get_type_reference(package="foo", imports=imports, source_type="foo.Message")
-
-    assert imports == set()
-    assert name == '"Message"'
-
-
-def test_reference_deeply_nested_siblings():
+def test_reference_root_sibling(typing_compiler: DirectImportTypingCompiler):
     imports = set()
     name = get_type_reference(
-        package="foo.bar", imports=imports, source_type="foo.bar.Message"
+        package="",
+        imports=imports,
+        source_type="Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == set()
     assert name == '"Message"'
 
 
-def test_reference_parent_package_from_child():
+def test_reference_nested_siblings(typing_compiler: DirectImportTypingCompiler):
     imports = set()
     name = get_type_reference(
-        package="package.child", imports=imports, source_type="package.Message"
+        package="foo",
+        imports=imports,
+        source_type="foo.Message",
+        typing_compiler=typing_compiler,
+    )
+
+    assert imports == set()
+    assert name == '"Message"'
+
+
+def test_reference_deeply_nested_siblings(typing_compiler: DirectImportTypingCompiler):
+    imports = set()
+    name = get_type_reference(
+        package="foo.bar",
+        imports=imports,
+        source_type="foo.bar.Message",
+        typing_compiler=typing_compiler,
+    )
+
+    assert imports == set()
+    assert name == '"Message"'
+
+
+def test_reference_parent_package_from_child(
+    typing_compiler: DirectImportTypingCompiler,
+):
+    imports = set()
+    name = get_type_reference(
+        package="package.child",
+        imports=imports,
+        source_type="package.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ... import package as __package__"}
     assert name == '"__package__.Message"'
 
 
-def test_reference_parent_package_from_deeply_nested_child():
+def test_reference_parent_package_from_deeply_nested_child(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
         package="package.deeply.nested.child",
         imports=imports,
         source_type="package.deeply.nested.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ... import nested as __nested__"}
     assert name == '"__nested__.Message"'
 
 
-def test_reference_ancestor_package_from_nested_child():
+def test_reference_ancestor_package_from_nested_child(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
         package="package.ancestor.nested.child",
         imports=imports,
         source_type="package.ancestor.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from .... import ancestor as ___ancestor__"}
     assert name == '"___ancestor__.Message"'
 
 
-def test_reference_root_package_from_child():
+def test_reference_root_package_from_child(typing_compiler: DirectImportTypingCompiler):
     imports = set()
     name = get_type_reference(
-        package="package.child", imports=imports, source_type="Message"
+        package="package.child",
+        imports=imports,
+        source_type="Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ... import Message as __Message__"}
     assert name == '"__Message__"'
 
 
-def test_reference_root_package_from_deeply_nested_child():
+def test_reference_root_package_from_deeply_nested_child(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
-        package="package.deeply.nested.child", imports=imports, source_type="Message"
+        package="package.deeply.nested.child",
+        imports=imports,
+        source_type="Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ..... import Message as ____Message__"}
     assert name == '"____Message__"'
 
 
-def test_reference_unrelated_package():
+def test_reference_unrelated_package(typing_compiler: DirectImportTypingCompiler):
     imports = set()
-    name = get_type_reference(package="a", imports=imports, source_type="p.Message")
+    name = get_type_reference(
+        package="a",
+        imports=imports,
+        source_type="p.Message",
+        typing_compiler=typing_compiler,
+    )
 
     assert imports == {"from .. import p as _p__"}
     assert name == '"_p__.Message"'
 
 
-def test_reference_unrelated_nested_package():
+def test_reference_unrelated_nested_package(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
-    name = get_type_reference(package="a.b", imports=imports, source_type="p.q.Message")
+    name = get_type_reference(
+        package="a.b",
+        imports=imports,
+        source_type="p.q.Message",
+        typing_compiler=typing_compiler,
+    )
 
     assert imports == {"from ...p import q as __p_q__"}
     assert name == '"__p_q__.Message"'
 
 
-def test_reference_unrelated_deeply_nested_package():
+def test_reference_unrelated_deeply_nested_package(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
-        package="a.b.c.d", imports=imports, source_type="p.q.r.s.Message"
+        package="a.b.c.d",
+        imports=imports,
+        source_type="p.q.r.s.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from .....p.q.r import s as ____p_q_r_s__"}
     assert name == '"____p_q_r_s__.Message"'
 
 
-def test_reference_cousin_package():
+def test_reference_cousin_package(typing_compiler: DirectImportTypingCompiler):
     imports = set()
-    name = get_type_reference(package="a.x", imports=imports, source_type="a.y.Message")
+    name = get_type_reference(
+        package="a.x",
+        imports=imports,
+        source_type="a.y.Message",
+        typing_compiler=typing_compiler,
+    )
 
     assert imports == {"from .. import y as _y__"}
     assert name == '"_y__.Message"'
 
 
-def test_reference_cousin_package_different_name():
+def test_reference_cousin_package_different_name(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
-        package="test.package1", imports=imports, source_type="cousin.package2.Message"
+        package="test.package1",
+        imports=imports,
+        source_type="cousin.package2.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ...cousin import package2 as __cousin_package2__"}
     assert name == '"__cousin_package2__.Message"'
 
 
-def test_reference_cousin_package_same_name():
+def test_reference_cousin_package_same_name(
+    typing_compiler: DirectImportTypingCompiler,
+):
     imports = set()
     name = get_type_reference(
-        package="test.package", imports=imports, source_type="cousin.package.Message"
+        package="test.package",
+        imports=imports,
+        source_type="cousin.package.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ...cousin import package as __cousin_package__"}
     assert name == '"__cousin_package__.Message"'
 
 
-def test_reference_far_cousin_package():
+def test_reference_far_cousin_package(typing_compiler: DirectImportTypingCompiler):
     imports = set()
     name = get_type_reference(
-        package="a.x.y", imports=imports, source_type="a.b.c.Message"
+        package="a.x.y",
+        imports=imports,
+        source_type="a.b.c.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ...b import c as __b_c__"}
     assert name == '"__b_c__.Message"'
 
 
-def test_reference_far_far_cousin_package():
+def test_reference_far_far_cousin_package(typing_compiler: DirectImportTypingCompiler):
     imports = set()
     name = get_type_reference(
-        package="a.x.y.z", imports=imports, source_type="a.b.c.d.Message"
+        package="a.x.y.z",
+        imports=imports,
+        source_type="a.b.c.d.Message",
+        typing_compiler=typing_compiler,
     )
 
     assert imports == {"from ....b.c import d as ___b_c_d__"}

tests/test_module_validation.py

@@ -0,0 +1,111 @@
+from typing import (
+    List,
+    Optional,
+    Set,
+)
+
+import pytest
+
+from betterproto.plugin.module_validation import ModuleValidator
+
+
+@pytest.mark.parametrize(
+    ["text", "expected_collisions"],
+    [
+        pytest.param(
+            ["import os"],
+            None,
+            id="single import",
+        ),
+        pytest.param(
+            ["import os", "import sys"],
+            None,
+            id="multiple imports",
+        ),
+        pytest.param(
+            ["import os", "import os"],
+            {"os"},
+            id="duplicate imports",
+        ),
+        pytest.param(
+            ["from os import path", "import os"],
+            None,
+            id="duplicate imports with alias",
+        ),
+        pytest.param(
+            ["from os import path", "import os as os_alias"],
+            None,
+            id="duplicate imports with alias",
+        ),
+        pytest.param(
+            ["from os import path", "import os as path"],
+            {"path"},
+            id="duplicate imports with alias",
+        ),
+        pytest.param(
+            ["import os", "class os:"],
+            {"os"},
+            id="duplicate import with class",
+        ),
+        pytest.param(
+            ["import os", "class os:", "  pass", "import sys"],
+            {"os"},
+            id="duplicate import with class and another",
+        ),
+        pytest.param(
+            ["def test(): pass", "class test:"],
+            {"test"},
+            id="duplicate class and function",
+        ),
+        pytest.param(
+            ["def test(): pass", "def test(): pass"],
+            {"test"},
+            id="duplicate functions",
+        ),
+        pytest.param(
+            ["def test(): pass", "test = 100"],
+            {"test"},
+            id="function and variable",
+        ),
+        pytest.param(
+            ["def test():", "    test = 3"],
+            None,
+            id="function and variable in function",
+        ),
+        pytest.param(
+            [
+                "def test(): pass",
+                "'''",
+                "def test(): pass",
+                "'''",
+                "def test_2(): pass",
+            ],
+            None,
+            id="duplicate functions with multiline string",
+        ),
+        pytest.param(
+            ["def test(): pass", "# def test(): pass"],
+            None,
+            id="duplicate functions with comments",
+        ),
+        pytest.param(
+            ["from test import (", "    A", "   B", "   C", ")"],
+            None,
+            id="multiline import",
+        ),
+        pytest.param(
+            ["from test import (", "    A", "   B", "   C", ")", "from test import A"],
+            {"A"},
+            id="multiline import with duplicate",
+        ),
+    ],
+)
+def test_module_validator(text: List[str], expected_collisions: Optional[Set[str]]):
+    line_iterator = iter(text)
+    validator = ModuleValidator(line_iterator)
+    valid = validator.validate()
+    if expected_collisions is None:
+        assert valid
+    else:
+        assert set(validator.collisions.keys()) == expected_collisions
+        assert not valid

tests/test_typing_compiler.py

@@ -0,0 +1,80 @@
+import pytest
+
+from betterproto.plugin.typing_compiler import (
+    DirectImportTypingCompiler,
+    NoTyping310TypingCompiler,
+    TypingImportTypingCompiler,
+)
+
+
+def test_direct_import_typing_compiler():
+    compiler = DirectImportTypingCompiler()
+    assert compiler.imports() == {}
+    assert compiler.optional("str") == "Optional[str]"
+    assert compiler.imports() == {"typing": {"Optional"}}
+    assert compiler.list("str") == "List[str]"
+    assert compiler.imports() == {"typing": {"Optional", "List"}}
+    assert compiler.dict("str", "int") == "Dict[str, int]"
+    assert compiler.imports() == {"typing": {"Optional", "List", "Dict"}}
+    assert compiler.union("str", "int") == "Union[str, int]"
+    assert compiler.imports() == {"typing": {"Optional", "List", "Dict", "Union"}}
+    assert compiler.iterable("str") == "Iterable[str]"
+    assert compiler.imports() == {
+        "typing": {"Optional", "List", "Dict", "Union", "Iterable"}
+    }
+    assert compiler.async_iterable("str") == "AsyncIterable[str]"
+    assert compiler.imports() == {
+        "typing": {"Optional", "List", "Dict", "Union", "Iterable", "AsyncIterable"}
+    }
+    assert compiler.async_iterator("str") == "AsyncIterator[str]"
+    assert compiler.imports() == {
+        "typing": {
+            "Optional",
+            "List",
+            "Dict",
+            "Union",
+            "Iterable",
+            "AsyncIterable",
+            "AsyncIterator",
+        }
+    }
+
+
+def test_typing_import_typing_compiler():
+    compiler = TypingImportTypingCompiler()
+    assert compiler.imports() == {}
+    assert compiler.optional("str") == "typing.Optional[str]"
+    assert compiler.imports() == {"typing": None}
+    assert compiler.list("str") == "typing.List[str]"
+    assert compiler.imports() == {"typing": None}
+    assert compiler.dict("str", "int") == "typing.Dict[str, int]"
+    assert compiler.imports() == {"typing": None}
+    assert compiler.union("str", "int") == "typing.Union[str, int]"
+    assert compiler.imports() == {"typing": None}
+    assert compiler.iterable("str") == "typing.Iterable[str]"
+    assert compiler.imports() == {"typing": None}
+    assert compiler.async_iterable("str") == "typing.AsyncIterable[str]"
+    assert compiler.imports() == {"typing": None}
+    assert compiler.async_iterator("str") == "typing.AsyncIterator[str]"
+    assert compiler.imports() == {"typing": None}
+
+
+def test_no_typing_311_typing_compiler():
+    compiler = NoTyping310TypingCompiler()
+    assert compiler.imports() == {}
+    assert compiler.optional("str") == "str | None"
+    assert compiler.imports() == {}
+    assert compiler.list("str") == "list[str]"
+    assert compiler.imports() == {}
+    assert compiler.dict("str", "int") == "dict[str, int]"
+    assert compiler.imports() == {}
+    assert compiler.union("str", "int") == "str | int"
+    assert compiler.imports() == {}
+    assert compiler.iterable("str") == "Iterable[str]"
+    assert compiler.imports() == {"typing": {"Iterable"}}
+    assert compiler.async_iterable("str") == "AsyncIterable[str]"
+    assert compiler.imports() == {"typing": {"Iterable", "AsyncIterable"}}
+    assert compiler.async_iterator("str") == "AsyncIterator[str]"
+    assert compiler.imports() == {
+        "typing": {"Iterable", "AsyncIterable", "AsyncIterator"}
+    }