Describe conflicts when raising MiniscriptPropertyError

[stickies-v]

Adds a list of type and property conflicts to make debugging easier. With the lambda function approach, it should remain pretty flexible to add other types of conflicts.

For example:

from bip380.miniscript.property import Property
p = Property("K")
p.check_valid()

raises

bip380.miniscript.errors.MiniscriptPropertyError: Conflicting types and properties: (not K or u), (not K or s)

[darosior]

Thanks for the PR! It reminds me we never perform this check anywhere for now.

[darosior]

Nice hack with inspect :-). But i think it would be cleaner if we leveraged the string representation here? For instance (untested):

diff --git a/bip380/miniscript/property.py b/bip380/miniscript/property.py
index 2f30b05..fa0c251 100644
--- a/bip380/miniscript/property.py
+++ b/bip380/miniscript/property.py
@@ -60,27 +60,27 @@ class Property:
                     )
                 num_types += 1
 
-        # Check for conflicts in type & properties.
-
-        checks = [
-            lambda: (not self.z or not self.o),
-            lambda: (not self.n or not self.z),
-            lambda: (not self.V or not self.d),
-            lambda: (not self.K or self.u),
-            lambda: (not self.V or not self.u),
-            lambda: (not self.e or not self.f),
-            lambda: (not self.e or self.d),
-            lambda: (not self.V or not self.e),
-            lambda: (not self.d or not self.f),
-            lambda: (not self.V or self.f),
-            lambda: (not self.K or self.s),
-            lambda: (not self.z or self.m),
+        # Check for conflicts in type & properties. For each type checked, give
+        # the properties it must have and the one it must not have.
+        conflicts = [
+            ("K", "", "us"),
+            ("V", "de", "f"),
+            ("z", "o", "m"),
+            ("n", "z", ""),
+            ("e", "d", "f"),
+            ("d", "", "f"),
         ]
-        conflicts = [fn for fn in checks if not fn()]
-        if conflicts:
-            formatted_conflicts = " ".join([inspect.getsource(fn).strip().replace("self.", "").replace("lambda: ", "")
-                                            for fn in conflicts])[:-1]
-            raise MiniscriptPropertyError(f"Conflicting types and properties: {formatted_conflicts}")
+        for (typ, must_be, must_not_be) in conflicts:
+            if not getattr(self, typ):
+                continue
+            if not all(getattr(self, prop) for prop in must_be):
+                raise MiniscriptPropertyError(
+                    f"Conflicting types and properties: '{typ}' needs always be '{must_be}'"
+                )
+            if any(getattr(self, prop) for prop in must_not_be):
+                raise MiniscriptPropertyError(
+                    f"Conflicting types and properties: '{typ}' can't be any of '{must_not_be}'"
+                )
 
     def type(self):
         return "".join(filter(lambda x: x in self.types, str(self)))

[stickies-v]

I've pushed "Use structured checks" that incorporates your suggestion, but I like it when errors contain as many detected problems as possible instead of just the first one so I modified that (and I think you had some of the must_be and must_not_be switched up - I'm just basing myself off the previous version, not sure how correct that is).

The reason I went for lambda & inspect is because I couldn't anticipate how much flexibility you'd like to keep with the conflict checking system. For example, with the lambda approach it would be trivial to add a check like "if (K and u), then must be s". If that flexibility is not (likely to be) necessary soon, then it would probably be better not to have to rely on the inspect module and have things a bit more structured. I think both approaches are sensible and don't really have a preference, so if you prefer the structured approach I'll squash those 2 commits and force push.

[darosior]

I've pushed "Use structured checks" that incorporates your suggestion, but I like it when errors contain as many detected problems as possible instead of just the first one so I modified that (and I think you had some of the must_be and must_not_be switched up - I'm just basing myself off the previous version, not sure how correct that is).

Signaling all errors at once is certainly preferable. Regarding the switch-ups the first version is probably more trust-worthy as i didn't verify the previous patch, it was meant as an illustration of the approach. I'll have a look tomorrow!

The reason I went for lambda & inspect is because I couldn't anticipate how much flexibility you'd like to keep with the conflict checking system. For example, with the lambda approach it would be trivial to add a check like "if (K and u), then must be s". If that flexibility is not (likely to be) necessary soon, then it would probably be better not to have to rely on the inspect module and have things a bit more structured. I think both approaches are sensible and don't really have a preference, so if you prefer the structured approach I'll squash those 2 commits and force push.

Yeah i find the str approach a bit more elegant, we can re-visit when/if we add more conflict checks.

[darosior]

ACK 96dcb1d

[darosior]

Heh, nice 😅