[develop2] fix require and reference equality

conans/model/graph_lock.py

@@ -49,16 +49,19 @@ def deserialize(data):
         return result
 
     def add(self, ref, package_ids=None):
-        # In case we have an existing, incomplete thing
-        pop_ref = RecipeReference.loads(str(ref))
-        self._requires.pop(pop_ref, None)  # Partial cannot have package_ids
-        old_package_ids = self._requires.pop(ref, None)
-        if old_package_ids is not None:
-            if package_ids is not None:
-                package_ids = old_package_ids.update(package_ids)
-            else:
-                package_ids = old_package_ids
-        self._requires[ref] = package_ids
+        if ref.revision is not None:
+            old_package_ids = self._requires.pop(ref, None)  # Get existing one
+            if old_package_ids is not None:
+                if package_ids is not None:
+                    package_ids = old_package_ids.update(package_ids)
+                else:
+                    package_ids = old_package_ids
+            self._requires[ref] = package_ids
+        else:  # Manual addition of something without revision
+            existing = {r: r for r in self._requires}.get(ref)
+            if existing and existing.revision is not None:
+                raise ConanException(f"Cannot add {ref} to lockfile, already exists")
+            self._requires[ref] = package_ids
 
     def sort(self):
         self._requires = OrderedDict(reversed(sorted(self._requires.items())))

conans/model/recipe_ref.py

@@ -78,12 +78,16 @@ def __eq__(self, ref):
         # This is necessary for building an ordered list of UNIQUE recipe_references for Lockfile
         if ref is None:
             return False
-        return (self.name, self.version, self.user, self.channel, self.revision) == \
-               (ref.name, ref.version, ref.user, ref.channel, ref.revision)
+        # If one revision is not defined, they are equal
+        if self.revision is not None and ref.revision is not None:
+            return (self.name, self.version, self.user, self.channel, self.revision) == \
+                   (ref.name, ref.version, ref.user, ref.channel, ref.revision)
+        return (self.name, self.version, self.user, self.channel) == \
+               (ref.name, ref.version, ref.user, ref.channel)
 
     def __hash__(self):
         # This is necessary for building an ordered list of UNIQUE recipe_references for Lockfile
-        return hash((self.name, self.version, self.user, self.channel, self.revision))
+        return hash((self.name, self.version, self.user, self.channel))
 
     @staticmethod
     def loads(rref):  # TODO: change this default to validate only on end points

conans/test/integration/lockfile/test_lock_requires.py

@@ -450,3 +450,82 @@ def test(self):
             assert "dep/1.0" in c.out
             assert "dep/2.0" not in c.out
             assert "package tested" in c.out
+
+
+class TestErrorDuplicates:
+    def test_error_duplicates(self):
+        """ the problem is having 2 different, almost identical requires that will point to the same
+        thing, with different traits and not colliding.
+        Lockfiles do a ``require.ref`` update and that alters some dictionaries iteration, producing
+        an infinite loop and blocking
+        """
+        c = TestClient()
+        pkg = textwrap.dedent("""
+            from conan import ConanFile
+            class Pkg(ConanFile):
+                name = "pkg"
+                version = "0.1"
+                def requirements(self):
+                    self.requires("dep/0.1#f8c2264d0b32a4c33f251fe2944bb642", headers=False, libs=False,
+                                visible=False)
+                    self.requires("dep/0.1", headers=True, libs=False, visible=False)
+                """)
+        c.save({"dep/conanfile.py": GenConanfile("dep", "0.1"),
+                "pkg/conanfile.py": pkg})
+        c.run("create dep --lockfile-out=conan.lock")
+        c.run("create pkg", assert_error=True)
+        assert "Duplicated requirement: dep/0.1" in c.out
+        c.run("create pkg --lockfile=conan.lock", assert_error=True)
+        assert "Duplicated requirement: dep/0.1" in c.out
+
+    def test_error_duplicates_reverse(self):
+        """ Same as above, but order requires changed
+        """
+        c = TestClient()
+        pkg = textwrap.dedent("""
+            from conan import ConanFile
+            class Pkg(ConanFile):
+                name = "pkg"
+                version = "0.1"
+                def requirements(self):
+                    self.requires("dep/0.1", headers=True, libs=False, visible=False)
+                    self.requires("dep/0.1#f8c2264d0b32a4c33f251fe2944bb642", headers=False, libs=False,
+                                visible=False)
+                """)
+        c.save({"dep/conanfile.py": GenConanfile("dep", "0.1"),
+                "pkg/conanfile.py": pkg})
+        c.run("create dep --lockfile-out=conan.lock")
+        c.run("create pkg", assert_error=True)
+        assert "Duplicated requirement: dep/0.1" in c.out
+        c.run("create pkg --lockfile=conan.lock", assert_error=True)
+        assert "Duplicated requirement: dep/0.1" in c.out
+
+    def test_error_duplicates_revisions(self):
+        """ 2 different revisions can be added without conflict, if they are not visible and not
+        other conflicting traits
+        """
+        c = TestClient()
+        pkg = textwrap.dedent("""
+            from conan import ConanFile
+            class Pkg(ConanFile):
+                name = "pkg"
+                version = "0.1"
+                def requirements(self):
+                    self.requires("dep/0.1#f8c2264d0b32a4c33f251fe2944bb642", headers=False,
+                                  libs=False, visible=False)
+                    self.requires("dep/0.1#7b91e6100797b8b012eb3cdc5544800b", headers=True,
+                                  libs=False, visible=False)
+                """)
+        c.save({"dep/conanfile.py": GenConanfile("dep", "0.1"),
+                "dep2/conanfile.py": GenConanfile("dep", "0.1").with_class_attribute("potato=42"),
+                "pkg/conanfile.py": pkg})
+        c.run("create dep --lockfile-out=conan.lock")
+        c.run("create dep2 --lockfile=conan.lock --lockfile-out=conan.lock")
+
+        c.run("create pkg")
+        assert "dep/0.1#f8c2264d0b32a4c33f251fe2944bb642 - Cache" in c.out
+        assert "dep/0.1#7b91e6100797b8b012eb3cdc5544800b - Cache" in c.out
+        print(c.load("conan.lock"))
+        c.run("create pkg --lockfile=conan.lock")
+        assert "dep/0.1#f8c2264d0b32a4c33f251fe2944bb642 - Cache" in c.out
+        assert "dep/0.1#7b91e6100797b8b012eb3cdc5544800b - Cache" in c.out

conans/test/integration/lockfile/test_user_overrides.py

@@ -181,12 +181,11 @@ def test_add_multiple_revisions():
     assert ['math/1.0#revx%0.0', 'math/1.0#rev2', 'math/1.0#rev1', 'math/1.0#rev0'] == \
            new_lock["requires"]
 
-    # add without revision at all
-    # It is not that this makes a lot of sense, but it is up to the user, the important thing
-    # is that it doesn't crash
-    c.run("lock add --requires=math/1.0")
+    # add without revision at all, will give us an error, as it doesn't make sense
+    c.run("lock add --requires=math/1.0", assert_error=True)
+    assert "Cannot add math/1.0 to lockfile, already exists" in c.out
     new_lock = json.loads(c.load("conan.lock"))
-    assert ['math/1.0#revx%0.0', 'math/1.0#rev2', 'math/1.0#rev1', 'math/1.0#rev0', 'math/1.0'] == \
+    assert ['math/1.0#revx%0.0', 'math/1.0#rev2', 'math/1.0#rev1', 'math/1.0#rev0'] == \
            new_lock["requires"]