Fix mojom backcompat checks on some field types

We weren't properly checking for backward-compatibility (but were rather
checking for strict equality) of types nested within generics. So if a
stable structure S has a field of type `array<T>` and T is changing in
a backward-compatible way, S would trigger a presubmit failure.

This change corrects the logic here to properly nest such
backward-compatibility checks.

Fixed: 1130671
Change-Id: If739bac2440927fbaf24f7feae5bbd04f9b29c09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422189Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#809541}

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

@@ -114,6 +114,9 @@ class Kind(object):
     # during a subsequent run of the parser.
     return hash((self.spec, self.parent_kind))
 
+  def IsBackwardCompatible(self, rhs):
+    return self == rhs
+
 
 class ReferenceKind(Kind):
   """ReferenceKind represents pointer and handle types.
@@ -195,6 +198,10 @@ 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)
+            and self.is_nullable == rhs.is_nullable)
+
 
 # Initialize the set of primitive types. These can be accessed by clients.
 BOOL = Kind('b')
@@ -380,10 +387,7 @@ def _IsFieldBackwardCompatible(new_field, old_field):
   if (new_field.min_version or 0) != (old_field.min_version or 0):
     return False
 
-  if isinstance(new_field.kind, (Enum, Struct, Union)):
-    return new_field.kind.IsBackwardCompatible(old_field.kind)
-
-  return new_field.kind == old_field.kind
+  return new_field.kind.IsBackwardCompatible(old_field.kind)
 
 
 class Struct(ReferenceKind):
@@ -704,6 +708,10 @@ class Array(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return (isinstance(rhs, Array) and self.length == rhs.length
+            and self.kind.IsBackwardCompatible(rhs.kind))
+
 
 class Map(ReferenceKind):
   """A map.
@@ -748,6 +756,11 @@ class Map(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return (isinstance(rhs, Map)
+            and self.key_kind.IsBackwardCompatible(rhs.key_kind)
+            and self.value_kind.IsBackwardCompatible(rhs.value_kind))
+
 
 class PendingRemote(ReferenceKind):
   ReferenceKind.AddSharedProperty('kind')
@@ -769,6 +782,10 @@ class PendingRemote(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return (isinstance(rhs, PendingRemote)
+            and self.kind.IsBackwardCompatible(rhs.kind))
+
 
 class PendingReceiver(ReferenceKind):
   ReferenceKind.AddSharedProperty('kind')
@@ -790,6 +807,10 @@ class PendingReceiver(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return isinstance(rhs, PendingReceiver) and self.kind.IsBackwardCompatible(
+        rhs.kind)
+
 
 class PendingAssociatedRemote(ReferenceKind):
   ReferenceKind.AddSharedProperty('kind')
@@ -811,6 +832,11 @@ class PendingAssociatedRemote(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return isinstance(
+        rhs, PendingAssociatedRemote) and self.kind.IsBackwardCompatible(
+            rhs.kind)
+
 
 class PendingAssociatedReceiver(ReferenceKind):
   ReferenceKind.AddSharedProperty('kind')
@@ -832,6 +858,11 @@ class PendingAssociatedReceiver(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return isinstance(
+        rhs, PendingAssociatedReceiver) and self.kind.IsBackwardCompatible(
+            rhs.kind)
+
 
 class InterfaceRequest(ReferenceKind):
   ReferenceKind.AddSharedProperty('kind')
@@ -852,6 +883,10 @@ class InterfaceRequest(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return isinstance(rhs, InterfaceRequest) and self.kind.IsBackwardCompatible(
+        rhs.kind)
+
 
 class AssociatedInterfaceRequest(ReferenceKind):
   ReferenceKind.AddSharedProperty('kind')
@@ -874,6 +909,11 @@ class AssociatedInterfaceRequest(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return isinstance(
+        rhs, AssociatedInterfaceRequest) and self.kind.IsBackwardCompatible(
+            rhs.kind)
+
 
 class Parameter(object):
   def __init__(self,
@@ -1150,6 +1190,10 @@ class AssociatedInterface(ReferenceKind):
   def __hash__(self):
     return id(self)
 
+  def IsBackwardCompatible(self, rhs):
+    return isinstance(
+        rhs, AssociatedInterface) and self.kind.IsBackwardCompatible(rhs.kind)
+
 
 class EnumField(object):
   def __init__(self,

mojo/public/tools/mojom/version_compatibility_unittest.py

@@ -234,6 +234,33 @@ class VersionCompatibilityTest(MojomParserTestCase):
     self.assertNotBackwardCompatible('union U { string a; };',
                                      'union U { string? a; };')
 
+  def testFieldNestedTypeChanged(self):
+    """Changing the definition of a nested type within a field (such as an array
+    element or interface endpoint type) should only break backward-compatibility
+    if the changes to that type are not backward-compatible."""
+    self.assertBackwardCompatible(
+        """\
+        struct S { string a; };
+        struct T { array<S> ss; };
+        """, """\
+        struct S {
+          string a;
+          [MinVersion=1] string? b;
+        };
+        struct T { array<S> ss; };
+        """)
+    self.assertBackwardCompatible(
+        """\
+        interface F { Do(); };
+        struct S { pending_receiver<F> r; };
+        """, """\
+        interface F {
+          Do();
+          [MinVersion=1] Say();
+        };
+        struct S { pending_receiver<F> r; };
+        """)
+
   def testUnionFieldBecomingNonOptional(self):
     """Changing a field from optional to non-optional breaks
     backward-compatibility."""