Fix for Issue #104 (#107)

This changes our logic for detecting duplicate definitions on case-sensitive file-systems to only operate on files of interest and to fix this detection logic per issue #104. If globular searches find duplicates we do not waste time detecting this until/unless these duplicates are actually considered when parsing target definitions. --------- Co-authored-by: Pavel Kirienko <[email protected]>
OpenCyphal · Jul 16, 2024 · c0afc34 · c0afc34
1 parent c7d8ef9
commit c0afc34
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 99 deletions.
diff --git a/pydsdl/__init__.py b/pydsdl/__init__.py
@@ -7,7 +7,7 @@
 import sys as _sys
 from pathlib import Path as _Path
 
-__version__ = "1.21.0"
+__version__ = "1.21.1"
 __version_info__ = tuple(map(int, __version__.split(".")[:3]))
 __license__ = "MIT"
 __author__ = "OpenCyphal"

diff --git a/pydsdl/_data_type_builder.py b/pydsdl/_data_type_builder.py
@@ -35,6 +35,18 @@ class MissingSerializationModeError(_error.InvalidDefinitionError):
     pass
 
 
+class DataTypeCollisionError(_error.InvalidDefinitionError):
+    """
+    Raised when there are conflicting data type definitions.
+    """
+
+
+class DataTypeNameCollisionError(DataTypeCollisionError):
+    """
+    Raised when type collisions are caused by naming conflicts.
+    """
+
+
 _logger = logging.getLogger(__name__)
 
 
@@ -197,7 +209,11 @@ def resolve_versioned_data_type(self, name: str, version: _serializable.Version)
             _logger.debug("The full name of a relatively referred type %r reconstructed as %r", name, full_name)
 
         del name
-        found = list(filter(lambda d: d.full_name == full_name and d.version == version, self._lookup_definitions))
+        found = list(
+            filter(
+                lambda d: d.full_name.lower() == full_name.lower() and d.version == version, self._lookup_definitions
+            )
+        )
         if not found:
             # Play Sherlock to help the user with mistakes like https://forum.opencyphal.org/t/904/2
             requested_ns = full_name.split(_serializable.CompositeType.NAME_COMPONENT_SEPARATOR)[0]
@@ -218,16 +234,34 @@ def resolve_versioned_data_type(self, name: str, version: _serializable.Version)
                 error_description += " Please make sure that you specified the directories correctly."
             raise UndefinedDataTypeError(error_description)
 
-        if len(found) > 1:  # pragma: no cover
-            raise _error.InternalError("Conflicting definitions: %r" % found)
+        if len(found) > 1:
+            if (
+                found[0].full_name != found[1].full_name and found[0].full_name.lower() == found[1].full_name.lower()
+            ):  # pragma: no cover
+                # This only happens if the file system is case-insensitive.
+                raise DataTypeNameCollisionError(
+                    "Full name of this definition differs from %s only by letter case, "
+                    "which is not permitted" % found[0].file_path,
+                    path=found[1].file_path,
+                )
+            raise DataTypeCollisionError("Conflicting definitions: %r" % found)
+        elif found[0].full_name != full_name and found[0].full_name.lower() == full_name.lower():
+            # pragma: no cover
+            # This only happens if the file system is case-sensitive.
+            raise DataTypeNameCollisionError(
+                "Full name of required definition %s differs from %s only by letter case, "
+                "which is not permitted" % (full_name, found[0].full_name),
+                path=found[0].file_path,
+            )
 
         target_definition = found[0]
-        for visitor in self._definition_visitors:
-            visitor.on_definition(self._definition, target_definition)
 
         assert isinstance(target_definition, ReadableDSDLFile)
-        assert target_definition.full_name == full_name
         assert target_definition.version == version
+
+        for visitor in self._definition_visitors:
+            visitor.on_definition(self._definition, target_definition)
+
         # Recursion is cool.
         dt = target_definition.read(
             lookup_definitions=self._lookup_definitions,

diff --git a/pydsdl/_namespace.py b/pydsdl/_namespace.py
@@ -27,18 +27,6 @@ class RootNamespaceNameCollisionError(_error.InvalidDefinitionError):
     """
 
 
-class DataTypeCollisionError(_error.InvalidDefinitionError):
-    """
-    Raised when there are conflicting data type definitions.
-    """
-
-
-class DataTypeNameCollisionError(DataTypeCollisionError):
-    """
-    Raised when there are conflicting data type names.
-    """
-
-
 class NestedRootNamespaceError(_error.InvalidDefinitionError):
     """
     Nested root namespaces are not allowed. This exception is thrown when this rule is violated.
@@ -260,9 +248,6 @@ def _complete_read_function(
 
     lookup_dsdl_definitions = _construct_dsdl_definitions_from_namespaces(lookup_directories_path_list)
 
-    # Check for collisions against the lookup definitions also.
-    _ensure_no_collisions(target_dsdl_definitions, lookup_dsdl_definitions)
-
     _logger.debug("Lookup DSDL definitions are listed below:")
     for x in lookup_dsdl_definitions:
         _logger.debug(_LOG_LIST_ITEM_PREFIX + str(x))
@@ -386,44 +371,6 @@ def _construct_dsdl_definitions_from_namespaces(
     return dsdl_file_sort([_dsdl_definition.DSDLDefinition(*p) for p in source_file_paths])
 
 
-def _ensure_no_collisions(
-    target_definitions: list[ReadableDSDLFile],
-    lookup_definitions: list[ReadableDSDLFile],
-) -> None:
-    for tg in target_definitions:
-        tg_full_namespace_period = tg.full_namespace.lower() + "."
-        tg_full_name_period = tg.full_name.lower() + "."
-        for lu in lookup_definitions:
-            lu_full_namespace_period = lu.full_namespace.lower() + "."
-            lu_full_name_period = lu.full_name.lower() + "."
-            # This is to allow the following messages to coexist happily:
-            #   zubax/non_colliding/iceberg/Ice.0.1.dsdl
-            #   zubax/non_colliding/IceB.0.1.dsdl
-            # The following is still not allowed:
-            #   zubax/colliding/iceberg/Ice.0.1.dsdl
-            #   zubax/colliding/Iceberg.0.1.dsdl
-            if tg.full_name != lu.full_name and tg.full_name.lower() == lu.full_name.lower():
-                raise DataTypeNameCollisionError(
-                    "Full name of this definition differs from %s only by letter case, "
-                    "which is not permitted" % lu.file_path,
-                    path=tg.file_path,
-                )
-            if (tg_full_namespace_period).startswith(lu_full_name_period):
-                raise DataTypeNameCollisionError(
-                    "The namespace of this type conflicts with %s" % lu.file_path, path=tg.file_path
-                )
-            if (lu_full_namespace_period).startswith(tg_full_name_period):
-                raise DataTypeNameCollisionError(
-                    "This type conflicts with the namespace of %s" % lu.file_path, path=tg.file_path
-                )
-            if (
-                tg_full_name_period == lu_full_name_period
-                and tg.version == lu.version
-                and not tg.file_path.samefile(lu.file_path)
-            ):  # https://github.com/OpenCyphal/pydsdl/issues/94
-                raise DataTypeCollisionError("This type is redefined in %s" % lu.file_path, path=tg.file_path)
-
-
 def _ensure_no_fixed_port_id_collisions(types: list[_serializable.CompositeType]) -> None:
     for a in types:
         for b in types:
@@ -864,22 +811,33 @@ def _unittest_read_files_empty_args() -> None:
     assert len(transitive) == 0
 
 
-def _unittest_ensure_no_collisions(temp_dsdl_factory) -> None:  # type: ignore
-    from pytest import raises as expect_raises
+def _unittest_ensure_no_namespace_name_collisions_or_nested_root_namespaces() -> None:
+    """gratuitous coverage of the collision check where other tests don't cover some edge cases."""
+    _ensure_no_namespace_name_collisions_or_nested_root_namespaces([], False)
 
-    _ = temp_dsdl_factory
 
-    # gratuitous coverage of the collision check where other tests don't cover some edge cases
-    _ensure_no_namespace_name_collisions_or_nested_root_namespaces([], False)
+def _unittest_issue_104(temp_dsdl_factory) -> None:  # type: ignore
+    """demonstrate that removing _ensure_no_collisions is okay"""
 
-    with expect_raises(DataTypeNameCollisionError):
-        _ensure_no_collisions(
-            [_dsdl_definition.DSDLDefinition(Path("a/b.1.0.dsdl"), Path("a"))],
-            [_dsdl_definition.DSDLDefinition(Path("a/B.1.0.dsdl"), Path("a"))],
-        )
+    from pytest import raises
 
-    with expect_raises(DataTypeNameCollisionError):
-        _ensure_no_collisions(
-            [_dsdl_definition.DSDLDefinition(Path("a/b/c.1.0.dsdl"), Path("a"))],
-            [_dsdl_definition.DSDLDefinition(Path("a/b.1.0.dsdl"), Path("a"))],
-        )
+    thing_1_0 = Path("a/b/thing.1.0.dsdl")
+    thing_type_1_0 = Path("a/b/thing/thingtype.1.0.dsdl")
+
+    file_at_root = temp_dsdl_factory.new_file(Path("a/Nothing.1.0.dsdl"), "@sealed\n")
+    thing_file = temp_dsdl_factory.new_file(thing_1_0, "@sealed\na.b.thing.thingtype.1.0 thing\n")
+    _ = temp_dsdl_factory.new_file(thing_type_1_0, "@sealed\n")
+
+    direct, transitive = read_files(thing_file, file_at_root.parent, file_at_root.parent)
+
+    assert len(direct) == 1
+    assert len(transitive) == 1
+
+    thing_1_1 = Path("a/b/thing.1.1.dsdl")
+
+    thing_file2 = temp_dsdl_factory.new_file(thing_1_1, "@sealed\na.b.thing.Thingtype.1.0 thing\n")
+
+    from ._data_type_builder import DataTypeNameCollisionError
+
+    with raises(DataTypeNameCollisionError):
+        read_files(thing_file2, file_at_root.parent, file_at_root.parent)
diff --git a/pydsdl/_test.py b/pydsdl/_test.py
@@ -11,6 +11,7 @@
 from pathlib import Path
 from textwrap import dedent
 import pytest  # This is only safe to import in test files!
+from . import _data_type_builder
 from . import _expression
 from . import _error
 from . import _parser
@@ -25,6 +26,8 @@
 class Workspace:
     def __init__(self) -> None:
         self._tmp_dir = tempfile.TemporaryDirectory(prefix="pydsdl-test-")
+        name = self._tmp_dir.name.upper()
+        self._is_case_sensitive = not Path(name).exists()
 
     @property
     def directory(self) -> Path:
@@ -58,6 +61,10 @@ def drop(self, rel_path_glob: str) -> None:
         for g in self.directory.glob(rel_path_glob):
             g.unlink()
 
+    @property
+    def is_fs_case_sensitive(self) -> bool:
+        return self._is_case_sensitive
+
 
 def parse_definition(
     definition: _dsdl_definition.DSDLDefinition, lookup_definitions: Sequence[_dsdl_definition.DSDLDefinition]
@@ -1096,7 +1103,7 @@ def print_handler(d: Path, line: int, text: str) -> None:
         """
         ),
     )
-    with raises(_namespace.DataTypeNameCollisionError):
+    with raises(_namespace.FixedPortIDCollisionError):
         _namespace.read_namespace(
             wrkspc.directory / "zubax",
             [
@@ -1105,30 +1112,27 @@ def print_handler(d: Path, line: int, text: str) -> None:
         )
 
     # Do again to test single lookup-directory override
-    with raises(_namespace.DataTypeNameCollisionError):
+    with raises(_namespace.FixedPortIDCollisionError):
         _namespace.read_namespace(wrkspc.directory / "zubax", wrkspc.directory / "zubax")
 
-    try:
-        (wrkspc.directory / "zubax/colliding/iceberg/300.Ice.30.0.dsdl").unlink()
-        wrkspc.new(
-            "zubax/COLLIDING/300.Iceberg.30.0.dsdl",
-            dedent(
-                """
-            @extent 1024
-            ---
-            @extent 1024
+    (wrkspc.directory / "zubax/colliding/iceberg/300.Ice.30.0.dsdl").unlink()
+    wrkspc.new(
+        "zubax/COLLIDING/300.Iceberg.30.0.dsdl",
+        dedent(
             """
-            ),
-        )
-        with raises(_namespace.DataTypeNameCollisionError, match=".*letter case.*"):
-            _namespace.read_namespace(
+        @extent 1024
+        ---
+        @extent 1024
+        """
+        ),
+    )
+    with raises(_namespace.FixedPortIDCollisionError):
+        _namespace.read_namespace(
+            wrkspc.directory / "zubax",
+            [
                 wrkspc.directory / "zubax",
-                [
-                    wrkspc.directory / "zubax",
-                ],
-            )
-    except _namespace.FixedPortIDCollisionError:  # pragma: no cover
-        pass  # We're running on a platform where paths are not case-sensitive.
+            ],
+        )
 
     # Test namespace can intersect with type name
     (wrkspc.directory / "zubax/COLLIDING/300.Iceberg.30.0.dsdl").unlink()
@@ -1161,6 +1165,52 @@ def print_handler(d: Path, line: int, text: str) -> None:
     assert "zubax.noncolliding.Iceb" in [x.full_name for x in parsed]
 
 
+def _unittest_collision_on_case_sensitive_filesystem(wrkspc: Workspace) -> None:
+    from pytest import raises
+
+    if not wrkspc.is_fs_case_sensitive:  # pragma: no cover
+        pytest.skip("This test is only relevant on case-sensitive filesystems.")
+
+    # Empty namespace.
+    assert [] == _namespace.read_namespace(wrkspc.directory)
+
+    wrkspc.new(
+        "atlantic/ships/Titanic.1.0.dsdl",
+        dedent(
+            """
+        greenland.colliding.IceBerg.1.0[<=2] bergs
+        @sealed
+        """
+        ),
+    )
+
+    wrkspc.new(
+        "greenland/colliding/IceBerg.1.0.dsdl",
+        dedent(
+            """
+        @sealed
+        """
+        ),
+    )
+
+    wrkspc.new(
+        "greenland/COLLIDING/IceBerg.1.0.dsdl",
+        dedent(
+            """
+        @sealed
+        """
+        ),
+    )
+
+    with raises(_data_type_builder.DataTypeNameCollisionError, match=".*letter case.*"):
+        _namespace.read_namespace(
+            wrkspc.directory / "atlantic",
+            [
+                wrkspc.directory / "greenland",
+            ],
+        )
+
+
 def _unittest_parse_namespace_versioning(wrkspc: Workspace) -> None:
     from pytest import raises
 
@@ -1265,8 +1315,7 @@ def _unittest_parse_namespace_versioning(wrkspc: Workspace) -> None:
         ),
     )
 
-    with raises(_namespace.DataTypeCollisionError):
-        _namespace.read_namespace((wrkspc.directory / "ns"), [])
+    _namespace.read_namespace((wrkspc.directory / "ns"), [])
 
     wrkspc.drop("ns/Spartans.30.2.dsdl")
 
@@ -1614,7 +1663,7 @@ def _unittest_issue94(wrkspc: Workspace) -> None:
     wrkspc.new("outer_b/ns/Foo.1.0.dsdl", "@sealed")  # Conflict!
     wrkspc.new("outer_a/ns/Bar.1.0.dsdl", "Foo.1.0 fo\n@sealed")  # Which Foo.1.0?
 
-    with raises(_namespace.DataTypeCollisionError):
+    with raises(_data_type_builder.DataTypeCollisionError):
         _namespace.read_namespace(
             wrkspc.directory / "outer_a" / "ns",
             [wrkspc.directory / "outer_b" / "ns"],