Fix recursion in mojom compat check

Self-referencing structs and unions should not cause the backcompat
checker to fail. This fixes it.

Bug: None
Change-Id: Ia852c0aa68c63ff7f8c613b830f7a259489fe41b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425304Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#809659}

mojo/public/tools/mojom/check_stable_mojom_compatibility.py

@@ -131,7 +131,8 @@ def _ValidateDelta(root, delta):
           'renamed, please add a [RenamedFrom] attribute to the new type. This '
           'can be deleted by a subsequent change.' % qualified_name)
 
-    if not new_types[new_name].IsBackwardCompatible(kind):
+    checker = module.BackwardCompatibilityChecker()
+    if not checker.IsBackwardCompatible(new_types[new_name], kind):
       raise Exception('Stable type %s appears to have changed in a way which '
                       'breaks backward-compatibility. Please fix!\n\nIf you '
                       'believe this assessment to be incorrect, please file a '

mojo/public/tools/mojom/mojom/generate/module.py

@@ -14,6 +14,27 @@
 
 import pickle
 
+
+class BackwardCompatibilityChecker(object):
+  """Used for memoization while recursively checking two type definitions for
+  backward-compatibility."""
+
+  def __init__(self):
+    self._cache = {}
+
+  def IsBackwardCompatible(self, new_kind, old_kind):
+    key = (new_kind, old_kind)
+    result = self._cache.get(key)
+    if result is None:
+      # Assume they're compatible at first to effectively ignore recursive
+      # checks between these types, e.g. if both kinds are a struct or union
+      # that references itself in a field.
+      self._cache[key] = True
+      result = new_kind.IsBackwardCompatible(old_kind, self)
+      self._cache[key] = result
+    return result
+
+
 # We use our own version of __repr__ when displaying the AST, as the
 # AST currently doesn't capture which nodes are reference (e.g. to
 # types) and which nodes are definitions. This allows us to e.g. print
@@ -114,7 +135,8 @@ class Kind(object):
     # during a subsequent run of the parser.
     return hash((self.spec, self.parent_kind))
 
-  def IsBackwardCompatible(self, rhs):
+  # pylint: disable=unused-argument
+  def IsBackwardCompatible(self, rhs, checker):
     return self == rhs
 
 
@@ -198,8 +220,8 @@ class ReferenceKind(Kind):
   def __hash__(self):
     return hash((super(ReferenceKind, self).__hash__(), self.is_nullable))
 
-  def IsBackwardCompatible(self, rhs):
-    return (super(ReferenceKind, self).IsBackwardCompatible(rhs)
+  def IsBackwardCompatible(self, rhs, checker):
+    return (super(ReferenceKind, self).IsBackwardCompatible(rhs, checker)
             and self.is_nullable == rhs.is_nullable)
 
 
@@ -383,11 +405,11 @@ class UnionField(Field):
   pass
 
 
-def _IsFieldBackwardCompatible(new_field, old_field):
+def _IsFieldBackwardCompatible(new_field, old_field, checker):
   if (new_field.min_version or 0) != (old_field.min_version or 0):
     return False
 
-  return new_field.kind.IsBackwardCompatible(old_field.kind)
+  return checker.IsBackwardCompatible(new_field.kind, old_field.kind)
 
 
 class Struct(ReferenceKind):
@@ -462,7 +484,7 @@ class Struct(ReferenceKind):
     for constant in self.constants:
       constant.Stylize(stylizer)
 
-  def IsBackwardCompatible(self, older_struct):
+  def IsBackwardCompatible(self, older_struct, checker):
     """This struct is backward-compatible with older_struct if and only if all
     of the following conditions hold:
       - Any newly added field is tagged with a [MinVersion] attribute specifying
@@ -501,7 +523,7 @@ class Struct(ReferenceKind):
       old_field = old_fields[ordinal]
       if (old_field.min_version or 0) > max_old_min_version:
         max_old_min_version = old_field.min_version
-      if not _IsFieldBackwardCompatible(new_field, old_field):
+      if not _IsFieldBackwardCompatible(new_field, old_field, checker):
         # Type or min-version mismatch between old and new versions of the same
         # ordinal field.
         return False
@@ -595,7 +617,7 @@ class Union(ReferenceKind):
     for field in self.fields:
       field.Stylize(stylizer)
 
-  def IsBackwardCompatible(self, older_union):
+  def IsBackwardCompatible(self, older_union, checker):
     """This union is backward-compatible with older_union if and only if all
     of the following conditions hold:
       - Any newly added field is tagged with a [MinVersion] attribute specifying
@@ -628,7 +650,7 @@ class Union(ReferenceKind):
       if not new_field:
         # A field was removed, which is not OK.
         return False
-      if not _IsFieldBackwardCompatible(new_field, old_field):
+      if not _IsFieldBackwardCompatible(new_field, old_field, checker):
         # An field changed its type or MinVersion, which is not OK.
         return False
       old_min_version = old_field.min_version or 0
@@ -708,9 +730,9 @@ class Array(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
+  def IsBackwardCompatible(self, rhs, checker):
     return (isinstance(rhs, Array) and self.length == rhs.length
-            and self.kind.IsBackwardCompatible(rhs.kind))
+            and checker.IsBackwardCompatible(self.kind, rhs.kind))
 
 
 class Map(ReferenceKind):
@@ -756,10 +778,10 @@ class Map(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
+  def IsBackwardCompatible(self, rhs, checker):
     return (isinstance(rhs, Map)
-            and self.key_kind.IsBackwardCompatible(rhs.key_kind)
-            and self.value_kind.IsBackwardCompatible(rhs.value_kind))
+            and checker.IsBackwardCompatible(self.key_kind, rhs.key_kind)
+            and checker.IsBackwardCompatible(self.value_kind, rhs.value_kind))
 
 
 class PendingRemote(ReferenceKind):
@@ -782,9 +804,9 @@ class PendingRemote(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
+  def IsBackwardCompatible(self, rhs, checker):
     return (isinstance(rhs, PendingRemote)
-            and self.kind.IsBackwardCompatible(rhs.kind))
+            and checker.IsBackwardCompatible(self.kind, rhs.kind))
 
 
 class PendingReceiver(ReferenceKind):
@@ -807,9 +829,9 @@ class PendingReceiver(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
-    return isinstance(rhs, PendingReceiver) and self.kind.IsBackwardCompatible(
-        rhs.kind)
+  def IsBackwardCompatible(self, rhs, checker):
+    return isinstance(rhs, PendingReceiver) and checker.IsBackwardCompatible(
+        self.kind, rhs.kind)
 
 
 class PendingAssociatedRemote(ReferenceKind):
@@ -832,10 +854,10 @@ class PendingAssociatedRemote(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
-    return isinstance(
-        rhs, PendingAssociatedRemote) and self.kind.IsBackwardCompatible(
-            rhs.kind)
+  def IsBackwardCompatible(self, rhs, checker):
+    return isinstance(rhs,
+                      PendingAssociatedRemote) and checker.IsBackwardCompatible(
+                          self.kind, rhs.kind)
 
 
 class PendingAssociatedReceiver(ReferenceKind):
@@ -858,10 +880,10 @@ class PendingAssociatedReceiver(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
+  def IsBackwardCompatible(self, rhs, checker):
     return isinstance(
-        rhs, PendingAssociatedReceiver) and self.kind.IsBackwardCompatible(
-            rhs.kind)
+        rhs, PendingAssociatedReceiver) and checker.IsBackwardCompatible(
+            self.kind, rhs.kind)
 
 
 class InterfaceRequest(ReferenceKind):
@@ -883,9 +905,9 @@ class InterfaceRequest(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
-    return isinstance(rhs, InterfaceRequest) and self.kind.IsBackwardCompatible(
-        rhs.kind)
+  def IsBackwardCompatible(self, rhs, checker):
+    return isinstance(rhs, InterfaceRequest) and checker.IsBackwardCompatible(
+        self.kind, rhs.kind)
 
 
 class AssociatedInterfaceRequest(ReferenceKind):
@@ -909,10 +931,10 @@ class AssociatedInterfaceRequest(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
+  def IsBackwardCompatible(self, rhs, checker):
     return isinstance(
-        rhs, AssociatedInterfaceRequest) and self.kind.IsBackwardCompatible(
-            rhs.kind)
+        rhs, AssociatedInterfaceRequest) and checker.IsBackwardCompatible(
+            self.kind, rhs.kind)
 
 
 class Parameter(object):
@@ -1075,7 +1097,7 @@ class Interface(ReferenceKind):
     for constant in self.constants:
       constant.Stylize(stylizer)
 
-  def IsBackwardCompatible(self, older_interface):
+  def IsBackwardCompatible(self, older_interface, checker):
     """This interface is backward-compatible with older_interface if and only
     if all of the following conditions hold:
       - All defined methods in older_interface (when identified by ordinal) have
@@ -1113,8 +1135,8 @@ class Interface(ReferenceKind):
         # A method was removed, which is not OK.
         return False
 
-      if not new_method.param_struct.IsBackwardCompatible(
-          old_method.param_struct):
+      if not checker.IsBackwardCompatible(new_method.param_struct,
+                                          old_method.param_struct):
         # The parameter list is not backward-compatible, which is not OK.
         return False
 
@@ -1127,8 +1149,8 @@ class Interface(ReferenceKind):
         if new_method.response_param_struct is None:
           # A reply was removed from a message, which is not OK.
           return False
-        if not new_method.response_param_struct.IsBackwardCompatible(
-            old_method.response_param_struct):
+        if not checker.IsBackwardCompatible(new_method.response_param_struct,
+                                            old_method.response_param_struct):
           # The new message's reply is not backward-compatible with the old
           # message's reply, which is not OK.
           return False
@@ -1190,9 +1212,10 @@ class AssociatedInterface(ReferenceKind):
   def __hash__(self):
     return id(self)
 
-  def IsBackwardCompatible(self, rhs):
-    return isinstance(
-        rhs, AssociatedInterface) and self.kind.IsBackwardCompatible(rhs.kind)
+  def IsBackwardCompatible(self, rhs, checker):
+    return isinstance(rhs,
+                      AssociatedInterface) and checker.IsBackwardCompatible(
+                          self.kind, rhs.kind)
 
 
 class EnumField(object):
@@ -1266,7 +1289,8 @@ class Enum(Kind):
       prefix = self.module.GetNamespacePrefix()
     return '%s%s' % (prefix, self.mojom_name)
 
-  def IsBackwardCompatible(self, older_enum):
+  # pylint: disable=unused-argument
+  def IsBackwardCompatible(self, older_enum, checker):
     """This enum is backward-compatible with older_enum if and only if one of
     the following conditions holds:
         - Neither enum is [Extensible] and both have the exact same set of valid

mojo/public/tools/mojom/version_compatibility_unittest.py

@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+from mojom.generate import module
 from mojom_parser_test_case import MojomParserTestCase
 
 
@@ -20,9 +21,11 @@ class VersionCompatibilityTest(MojomParserTestCase):
     self.assertEqual(set(old.keys()), set(new.keys()),
                      'Old and new test mojoms should use the same type names.')
 
+    checker = module.BackwardCompatibilityChecker()
     compatibility_map = {}
     for name in old.keys():
-      compatibility_map[name] = new[name].IsBackwardCompatible(old[name])
+      compatibility_map[name] = checker.IsBackwardCompatible(
+          new[name], old[name])
     return compatibility_map
 
   def assertBackwardCompatible(self, old_mojom, new_mojom):
@@ -261,6 +264,20 @@ class VersionCompatibilityTest(MojomParserTestCase):
         struct S { pending_receiver<F> r; };
         """)
 
+  def testRecursiveTypeChange(self):
+    """Recursive types do not break the compatibility checker."""
+    self.assertBackwardCompatible(
+        """\
+        struct S {
+          string a;
+          array<S> others;
+        };""", """\
+        struct S {
+          string a;
+          array<S> others;
+          [MinVersion=1] string? b;
+        };""")
+
   def testUnionFieldBecomingNonOptional(self):
     """Changing a field from optional to non-optional breaks
     backward-compatibility."""