[mojom] Interface backward-compatibility checks

Implements a test for backward-compatibility between any two versions
of a mojom interface definition.

Bug: 1070663
Change-Id: I832d7ecaf6d8b2c778eb9f1378f183c027585d28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216910Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#772498}

mojo/public/tools/bindings/mojom_bindings_generator.py

@@ -42,7 +42,7 @@ import mojom.fileutil as fileutil
 from mojom.generate.module import Module
 from mojom.generate import template_expander
 from mojom.generate import translate
-from mojom.generate.generator import AddComputedData, WriteFile
+from mojom.generate.generator import WriteFile
 
 sys.path.append(
     os.path.join(_GetDirAbove("mojo"), "tools", "diagnosis"))
@@ -185,7 +185,6 @@ class MojomProcessor(object):
       ScrambleMethodOrdinals(module.interfaces, salt)
 
     if self._should_generate(rel_filename.path):
-      AddComputedData(module)
       for language, generator_module in generator_modules.items():
         generator = generator_module.Generator(
             module, args.output_dir, typemap=self._typemap.get(language, {}),

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

@@ -164,14 +164,10 @@ def AddComputedData(module):
     struct.exported = exported
 
   def _AddInterfaceComputedData(interface):
-    next_ordinal = 0
     interface.version = 0
     for method in interface.methods:
-      if method.ordinal is None:
-        method.ordinal = next_ordinal
       # this field is never scrambled
-      method.sequential_ordinal = next_ordinal
-      next_ordinal = method.ordinal + 1
+      method.sequential_ordinal = method.ordinal
 
       if method.min_version is not None:
         interface.version = max(interface.version, method.min_version)

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

@@ -998,6 +998,78 @@ class Interface(ReferenceKind):
     for constant in self.constants:
       constant.Stylize(stylizer)
 
+  def IsBackwardCompatible(self, older_interface):
+    """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
+        backward-compatible definitions in this interface. For each method this
+        means:
+          - The parameter list is backward-compatible, according to backward-
+            compatibility rules for structs, where each parameter is essentially
+            a struct field.
+          - If the old method definition does not specify a reply message, the
+            new method definition must not specify a reply message.
+          - If the old method definition specifies a reply message, the new
+            method definition must also specify a reply message with a parameter
+            list that is backward-compatible according to backward-compatibility
+            rules for structs.
+      - All newly introduced methods in this interface have a [MinVersion]
+        attribute specifying a version greater than any method in
+        older_interface.
+    """
+
+    def buildOrdinalMethodMap(interface):
+      methods_by_ordinal = {}
+      for method in interface.methods:
+        if method.ordinal in methods_by_ordinal:
+          raise Exception('Multiple methods with ordinal %s in interface %s.' %
+                          (method.ordinal, interface.mojom_name))
+        methods_by_ordinal[method.ordinal] = method
+      return methods_by_ordinal
+
+    new_methods = buildOrdinalMethodMap(self)
+    old_methods = buildOrdinalMethodMap(older_interface)
+    max_old_min_version = 0
+    for ordinal, old_method in old_methods.items():
+      new_method = new_methods.get(ordinal)
+      if not new_method:
+        # A method was removed, which is not OK.
+        return False
+
+      if not new_method.param_struct.IsBackwardCompatible(
+          old_method.param_struct):
+        # The parameter list is not backward-compatible, which is not OK.
+        return False
+
+      if old_method.response_param_struct is None:
+        if new_method.response_param_struct is not None:
+          # A reply was added to a message which didn't have one before, and
+          # this is not OK.
+          return False
+      else:
+        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):
+          # The new message's reply is not backward-compatible with the old
+          # message's reply, which is not OK.
+          return False
+
+      if (old_method.min_version or 0) > max_old_min_version:
+        max_old_min_version = old_method.min_version
+
+    # All the old methods are compatible with their new counterparts. Now verify
+    # that newly added methods are properly versioned.
+    new_ordinals = set(new_methods.keys()) - set(old_methods.keys())
+    for ordinal in new_ordinals:
+      new_method = new_methods[ordinal]
+      if (new_method.min_version or 0) <= max_old_min_version:
+        # A method was added to an existing version, which is not OK.
+        return False
+
+    return True
+
   def __eq__(self, rhs):
     return (isinstance(rhs, Interface)
             and (self.mojom_name, self.methods, self.enums, self.constants,

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

@@ -13,6 +13,7 @@ import itertools
 import os
 import re
 
+from mojom.generate import generator
 from mojom.generate import module as mojom
 from mojom.parse import ast
 
@@ -767,6 +768,7 @@ def _Module(tree, path, imports):
     interface.methods = list(
         map(lambda method: _Method(module, method, interface),
             interface.methods_data))
+    _AssignDefaultOrdinals(interface.methods)
     del interface.methods_data
     all_defined_kinds[interface.spec] = interface
     for enum in interface.enums:
@@ -781,6 +783,14 @@ def _Module(tree, path, imports):
   module.imported_kinds = dict(
       (spec, all_referenced_kinds[spec]) for spec in imported_kind_specs)
 
+  generator.AddComputedData(module)
+  for iface in module.interfaces:
+    for method in iface.methods:
+      if method.param_struct:
+        _AssignDefaultOrdinals(method.param_struct.fields)
+      if method.response_param_struct:
+        _AssignDefaultOrdinals(method.response_param_struct.fields)
+
   return module
 
 

mojo/public/tools/mojom/version_compatibility_unittest.py

@@ -306,3 +306,92 @@ class VersionCompatibilityTest(MojomParserTestCase):
     ordinals are explicitly labeled and remain unchanged."""
     self.assertBackwardCompatible('union U { bool b@1; int32 a@0; };',
                                   'union U { int32 a@0; bool b@1; };')
+
+  def testNewInterfaceMethodUnversioned(self):
+    """Adding a new method to an interface without a new (i.e. higher than any
+    existing version) [MinVersion] tag breaks backward-compatibility."""
+    self.assertNotBackwardCompatible('interface F { A(); };',
+                                     'interface F { A(); B(); };')
+
+  def testInterfaceMethodRemoval(self):
+    """Removing a method from an interface breaks backward-compatibility."""
+    self.assertNotBackwardCompatible('interface F { A(); B(); };',
+                                     'interface F { A(); };')
+
+  def testInterfaceMethodParamsChanged(self):
+    """Changes to the parameter list are only backward-compatible if they meet
+    backward-compatibility requirements of an equivalent struct definition."""
+    self.assertNotBackwardCompatible('interface F { A(); };',
+                                     'interface F { A(int32 x); };')
+    self.assertNotBackwardCompatible('interface F { A(int32 x); };',
+                                     'interface F { A(bool x); };')
+    self.assertNotBackwardCompatible(
+        'interface F { A(int32 x, [MinVersion=1] string? s); };', """\
+        interface F {
+          A(int32 x, [MinVersion=1] string? s, [MinVersion=1] int32 y);
+        };""")
+
+    self.assertBackwardCompatible('interface F { A(int32 x); };',
+                                  'interface F { A(int32 a); };')
+    self.assertBackwardCompatible(
+        'interface F { A(int32 x); };',
+        'interface F { A(int32 x, [MinVersion=1] string? s); };')
+
+    self.assertBackwardCompatible(
+        'struct S {}; interface F { A(S s); };',
+        'struct S { [MinVersion=1] int32 x; }; interface F { A(S s); };')
+    self.assertBackwardCompatible(
+        'struct S {}; struct T {}; interface F { A(S s); };',
+        'struct S {}; struct T {}; interface F { A(T s); };')
+    self.assertNotBackwardCompatible(
+        'struct S {}; struct T { int32 x; }; interface F { A(S s); };',
+        'struct S {}; struct T { int32 x; }; interface F { A(T t); };')
+
+  def testInterfaceMethodReplyAdded(self):
+    """Adding a reply to a message breaks backward-compatibilty."""
+    self.assertNotBackwardCompatible('interface F { A(); };',
+                                     'interface F { A() => (); };')
+
+  def testInterfaceMethodReplyRemoved(self):
+    """Removing a reply from a message breaks backward-compatibility."""
+    self.assertNotBackwardCompatible('interface F { A() => (); };',
+                                     'interface F { A(); };')
+
+  def testInterfaceMethodReplyParamsChanged(self):
+    """Similar to request parameters, a change to reply parameters is considered
+    backward-compatible if it meets the same backward-compatibility
+    requirements imposed on equivalent struct changes."""
+    self.assertNotBackwardCompatible('interface F { A() => (); };',
+                                     'interface F { A() => (int32 x); };')
+    self.assertNotBackwardCompatible('interface F { A() => (int32 x); };',
+                                     'interface F { A() => (); };')
+    self.assertNotBackwardCompatible('interface F { A() => (bool x); };',
+                                     'interface F { A() => (int32 x); };')
+
+    self.assertBackwardCompatible('interface F { A() => (int32 a); };',
+                                  'interface F { A() => (int32 x); };')
+    self.assertBackwardCompatible(
+        'interface F { A() => (int32 x); };',
+        'interface F { A() => (int32 x, [MinVersion] string? s); };')
+
+  def testNewInterfaceMethodWithInvalidMinVersion(self):
+    """Adding a new method to an existing version is not backward-compatible."""
+    self.assertNotBackwardCompatible(
+        """\
+        interface F {
+          A();
+          [MinVersion=1] B();
+        };
+        """, """\
+        interface F {
+          A();
+          [MinVersion=1] B();
+          [MinVersion=1] C();
+        };
+        """)
+
+  def testNewInterfaceMethodWithValidMinVersion(self):
+    """Adding a new method is fine as long as its MinVersion exceeds that of any
+    method on the old interface definition."""
+    self.assertBackwardCompatible('interface F { A(); };',
+                                  'interface F { A(); [MinVersion=1] B(); };')