Introduce presubmit check for stable mojom changes

Mojom types marked [Stable] must preserve backward-compatibility over
time. This presubmit tool checks for unsafe changes to any such types.

Fixed: 1070663
Change-Id: Ie1d3ca30d608dd95e256c782593c9df910512e96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2219020
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773276}

PRESUBMIT.py

@@ -4407,6 +4407,7 @@ def _CommonChecks(input_api, output_api):
   results.extend(_CheckBuildtoolsRevisionsAreInSync(input_api, output_api))
   results.extend(_CheckForTooLargeFiles(input_api, output_api))
   results.extend(_CheckPythonDevilInit(input_api, output_api))
+  results.extend(_CheckStableMojomChanges(input_api, output_api))
 
   for f in input_api.AffectedFiles():
     path, name = input_api.os_path.split(f.LocalPath())
@@ -5195,3 +5196,36 @@ def _CheckTranslationExpectations(input_api, output_api,
       ' - %s is not updated.\n'
       'Stack:\n%s' % (translation_expectations_path, str(e)))]
   return []
+
+
+def _CheckStableMojomChanges(input_api, output_api):
+  """Changes to [Stable] mojom types must preserve backward-compatibility."""
+  changed_mojoms = [f for f in input_api.AffectedSourceFiles(None)
+                    if f.LocalPath().endswith('.mojom')]
+  delta = []
+  for mojom in changed_mojoms:
+    old_contents = ''.join(mojom.OldContents()) or None
+    new_contents = ''.join(mojom.NewContents()) or None
+    delta.append({
+      'filename': mojom.LocalPath(),
+      'old': '\n'.join(mojom.OldContents()) or None,
+      'new': '\n'.join(mojom.NewContents()) or None,
+      })
+
+  process = input_api.subprocess.Popen(
+      [input_api.python_executable,
+       input_api.os_path.join(input_api.PresubmitLocalPath(), 'mojo',
+                              'public', 'tools', 'mojom',
+                              'check_stable_mojom_compatibility.py'),
+       '--src-root', input_api.PresubmitLocalPath()],
+       stdin=input_api.subprocess.PIPE,
+       stdout=input_api.subprocess.PIPE,
+       stderr=input_api.subprocess.PIPE,
+       universal_newlines=True)
+  (x, error) = process.communicate(input=input_api.json.dumps(delta))
+  if process.returncode:
+    return [output_api.PresubmitError(
+        'One or more [Stable] mojom definitions appears to have been changed '
+        'in a way that is not backward-compatible.',
+        long_text=error)]
+  return []

PRESUBMIT_test.py

@@ -3491,5 +3491,31 @@ class SetNoParentTest(unittest.TestCase):
     self.assertEqual([], errors)
 
 
+class MojomStabilityCheckTest(unittest.TestCase):
+  def runTestWithAffectedFiles(self, affected_files):
+    mock_input_api = MockInputApi()
+    mock_input_api.files = affected_files
+    mock_output_api = MockOutputApi()
+    return PRESUBMIT._CheckStableMojomChanges(
+        mock_input_api, mock_output_api)
+
+  def testSafeChangePasses(self):
+    errors = self.runTestWithAffectedFiles([
+      MockAffectedFile('foo/foo.mojom',
+                       ['[Stable] struct S { [MinVersion=1] int32 x; };'],
+                       old_contents=['[Stable] struct S {};'])
+    ])
+    self.assertEqual([], errors)
+
+  def testBadChangeFails(self):
+    errors = self.runTestWithAffectedFiles([
+      MockAffectedFile('foo/foo.mojom',
+                       ['[Stable] struct S { int32 x; };'],
+                       old_contents=['[Stable] struct S {};'])
+    ])
+    self.assertEqual(1, len(errors))
+    self.assertTrue('not backward-compatible' in errors[0].message)
+
+
 if __name__ == '__main__':
   unittest.main()

mojo/public/tools/mojom/check_stable_mojom_compatibility.py

@@ -0,0 +1,157 @@
+#!/usr/bin/env python
+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""Verifies backward-compatibility of mojom type changes.
+
+Given a set of pre- and post-diff mojom file contents, and a root directory
+for a project, this tool verifies that any changes to [Stable] mojom types are
+backward-compatible with the previous version.
+
+This can be used e.g. by a presubmit check to prevent developers from making
+breaking changes to stable mojoms."""
+
+import argparse
+import errno
+import json
+import os
+import os.path
+import shutil
+import sys
+import tempfile
+
+from mojom.generate import module
+from mojom.generate import translate
+from mojom.parse import parser
+
+
+def _ValidateDelta(root, delta):
+  """Parses all modified mojoms (including all transitive mojom dependencies,
+  even if unmodified) to perform backward-compatibility checks on any types
+  marked with the [Stable] attribute.
+
+  Note that unlike the normal build-time parser in mojom_parser.py, this does
+  not produce or rely on cached module translations, but instead parses the full
+  transitive closure of a mojom's input dependencies all at once.
+  """
+
+  # First build a map of all files covered by the delta
+  affected_files = set()
+  old_files = {}
+  new_files = {}
+  for change in delta:
+    filename = change['filename']
+    affected_files.add(filename)
+    if change['old']:
+      old_files[filename] = change['old']
+    if change['new']:
+      new_files[filename] = change['new']
+
+  # Parse and translate all mojoms relevant to the delta, including transitive
+  # imports that weren't modified.
+  unmodified_modules = {}
+
+  def parseMojom(mojom, file_overrides, override_modules):
+    if mojom in unmodified_modules or mojom in override_modules:
+      return
+
+    contents = file_overrides.get(mojom)
+    if contents:
+      modules = override_modules
+    else:
+      modules = unmodified_modules
+      with open(os.path.join(root, mojom)) as f:
+        contents = ''.join(f.readlines())
+
+    ast = parser.Parse(str(contents), mojom)
+    for imp in ast.import_list:
+      parseMojom(imp.import_filename, file_overrides, override_modules)
+
+    # Now that the transitive set of dependencies has been imported and parsed
+    # above, translate each mojom AST into a Module so that all types are fully
+    # defined and can be inspected.
+    all_modules = {}
+    all_modules.update(unmodified_modules)
+    all_modules.update(override_modules)
+    modules[mojom] = translate.OrderedModule(ast, mojom, all_modules)
+
+  old_modules = {}
+  for mojom in old_files.keys():
+    parseMojom(mojom, old_files, old_modules)
+  new_modules = {}
+  for mojom in new_files.keys():
+    parseMojom(mojom, new_files, new_modules)
+
+  # At this point we have a complete set of translated Modules from both the
+  # pre- and post-diff mojom contents. Now we can analyze backward-compatibility
+  # of the deltas.
+  #
+  # Note that for backward-compatibility checks we only care about types which
+  # were marked [Stable] before the diff. Types newly marked as [Stable] are not
+  # checked.
+  def collectTypes(modules):
+    types = {}
+    for m in modules.values():
+      for kinds in (m.enums, m.structs, m.unions, m.interfaces):
+        for kind in kinds:
+          types[kind.qualified_name] = kind
+    return types
+
+  old_types = collectTypes(old_modules)
+  new_types = collectTypes(new_modules)
+
+  # Collect any renamed types so they can be compared accordingly.
+  renamed_types = {}
+  for name, kind in new_types.items():
+    old_name = kind.attributes and kind.attributes.get('RenamedFrom')
+    if old_name:
+      renamed_types[old_name] = name
+
+  for qualified_name, kind in old_types.items():
+    if not kind.stable:
+      continue
+
+    new_name = renamed_types.get(qualified_name, qualified_name)
+    if new_name not in new_types:
+      raise Exception(
+          'Stable type %s appears to be deleted by this change. If it was '
+          '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):
+      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 '
+                      'Chromium bug against the "Internals>Mojo>Bindings" '
+                      'component.' % qualified_name)
+
+
+def Run(command_line, delta=None):
+  """Runs the tool with the given command_line. Normally this will read the
+  change description from stdin as a JSON-encoded list, but tests may pass a
+  delta directly for convenience."""
+  arg_parser = argparse.ArgumentParser(
+      description='Verifies backward-compatibility of mojom type changes.',
+      epilog="""
+This tool reads a change description from stdin and verifies that all modified
+[Stable] mojom types will retain backward-compatibility. The change description
+must be a JSON-encoded list of objects, each with a "filename" key (path to a
+changed mojom file, relative to ROOT); an "old" key whose value is a string of
+the full file contents before the change, or null if the file is being added;
+and a "new" key whose value is a string of the full file contents after the
+change, or null if the file is being deleted.""")
+  arg_parser.add_argument(
+      '--src-root',
+      required=True,
+      action='store',
+      metavar='ROOT',
+      help='The root of the source tree in which the checked mojoms live.')
+
+  args, _ = arg_parser.parse_known_args(command_line)
+  if not delta:
+    delta = json.load(sys.stdin)
+  _ValidateDelta(args.src_root, delta)
+
+
+if __name__ == '__main__':
+  Run(sys.argv[1:])

mojo/public/tools/mojom/check_stable_mojom_compatibility_unittest.py

@@ -0,0 +1,260 @@
+#!/usr/bin/env python
+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import json
+import os
+import os.path
+import shutil
+import tempfile
+import unittest
+
+import check_stable_mojom_compatibility
+
+from mojom.generate import module
+
+
+class Change(object):
+  """Helper to clearly define a mojom file delta to be analyzed."""
+
+  def __init__(self, filename, old=None, new=None):
+    """If old is None, this is a file addition. If new is None, this is a file
+    deletion. Otherwise it's a file change."""
+    self.filename = filename
+    self.old = old
+    self.new = new
+
+
+class UnchangedFile(Change):
+  def __init__(self, filename, contents):
+    super(UnchangedFile, self).__init__(filename, old=contents, new=contents)
+
+
+class CheckStableMojomCompatibilityTest(unittest.TestCase):
+  """Tests covering the behavior of the compatibility checking tool. Note that
+  details of different compatibility checks and relevant failure modes are NOT
+  covered by these tests. Those are instead covered by unittests in
+  version_compatibility_unittest.py. Additionally, the tests which ensure a
+  given set of [Stable] mojom definitions are indeed plausibly stable (i.e. they
+  have no unstable dependencies) are covered by stable_attribute_unittest.py.
+
+  These tests cover higher-level concerns of the compatibility checking tool,
+  like file or symbol, renames, changes spread over multiple files, etc."""
+
+  def verifyBackwardCompatibility(self, changes):
+    """Helper for implementing assertBackwardCompatible and
+    assertNotBackwardCompatible"""
+
+    temp_dir = tempfile.mkdtemp()
+    for change in changes:
+      if change.old:
+        # Populate the old file on disk in our temporary fake source root
+        file_path = os.path.join(temp_dir, change.filename)
+        dir_path = os.path.dirname(file_path)
+        if not os.path.exists(dir_path):
+          os.makedirs(dir_path)
+        with open(file_path, 'w') as f:
+          f.write(change.old)
+
+    delta = []
+    for change in changes:
+      if change.old != change.new:
+        delta.append({
+            'filename': change.filename,
+            'old': change.old,
+            'new': change.new
+        })
+
+    try:
+      check_stable_mojom_compatibility.Run(['--src-root', temp_dir],
+                                           delta=delta)
+    finally:
+      shutil.rmtree(temp_dir)
+
+  def assertBackwardCompatible(self, changes):
+    self.verifyBackwardCompatibility(changes)
+
+  def assertNotBackwardCompatible(self, changes):
+    try:
+      self.verifyBackwardCompatibility(changes)
+    except Exception:
+      return
+
+    raise Exception('Change unexpectedly passed a backward-compatibility check')
+
+  def testBasicCompatibility(self):
+    """Minimal smoke test to verify acceptance of a simple valid change."""
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom',
+               old='[Stable] struct S {};',
+               new='[Stable] struct S { [MinVersion=1] int32 x; };')
+    ])
+
+  def testBasicIncompatibility(self):
+    """Minimal smoke test to verify rejection of a simple invalid change."""
+    self.assertNotBackwardCompatible([
+        Change('foo/foo.mojom',
+               old='[Stable] struct S {};',
+               new='[Stable] struct S { int32 x; };')
+    ])
+
+  def testIgnoreIfNotStable(self):
+    """We don't care about types not marked [Stable]"""
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom',
+               old='struct S {};',
+               new='struct S { int32 x; };')
+    ])
+
+  def testRename(self):
+    """We can do checks for renamed types."""
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom',
+               old='[Stable] struct S {};',
+               new='[Stable, RenamedFrom="S"] struct T {};')
+    ])
+    self.assertNotBackwardCompatible([
+        Change('foo/foo.mojom',
+               old='[Stable] struct S {};',
+               new='[Stable, RenamedFrom="S"] struct T { int32 x; };')
+    ])
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom',
+               old='[Stable] struct S {};',
+               new="""\
+               [Stable, RenamedFrom="S"]
+               struct T { [MinVersion=1] int32 x; };
+               """)
+    ])
+
+  def testNewlyStable(self):
+    """We don't care about types newly marked as [Stable]."""
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom',
+               old='struct S {};',
+               new='[Stable] struct S { int32 x; };')
+    ])
+
+  def testFileRename(self):
+    """Make sure we can still do compatibility checks after a file rename."""
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom', old='[Stable] struct S {};', new=None),
+        Change('bar/bar.mojom',
+               old=None,
+               new='[Stable] struct S { [MinVersion=1] int32 x; };')
+    ])
+    self.assertNotBackwardCompatible([
+        Change('foo/foo.mojom', old='[Stable] struct S {};', new=None),
+        Change('bar/bar.mojom', old=None, new='[Stable] struct S { int32 x; };')
+    ])
+
+  def testWithImport(self):
+    """Ensure that cross-module dependencies do not break the compatibility
+    checking tool."""
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom',
+               old="""\
+               module foo;
+               [Stable] struct S {};
+               """,
+               new="""\
+               module foo;
+               [Stable] struct S { [MinVersion=2] int32 x; };
+               """),
+        Change('bar/bar.mojom',
+               old="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; };
+               """,
+               new="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; [MinVersion=1] int32 y; };
+               """)
+    ])
+
+  def testWithMovedDefinition(self):
+    """If a definition moves from one file to another, we should still be able
+    to check compatibility accurately."""
+    self.assertBackwardCompatible([
+        Change('foo/foo.mojom',
+               old="""\
+               module foo;
+               [Stable] struct S {};
+               """,
+               new="""\
+               module foo;
+               """),
+        Change('bar/bar.mojom',
+               old="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; };
+               """,
+               new="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable, RenamedFrom="foo.S"] struct S {
+                 [MinVersion=2] int32 x;
+               };
+               [Stable] struct T { S s; [MinVersion=1] int32 y; };
+               """)
+    ])
+
+    self.assertNotBackwardCompatible([
+        Change('foo/foo.mojom',
+               old="""\
+               module foo;
+               [Stable] struct S {};
+               """,
+               new="""\
+               module foo;
+               """),
+        Change('bar/bar.mojom',
+               old="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; };
+               """,
+               new="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable, RenamedFrom="foo.S"] struct S { int32 x; };
+               [Stable] struct T { S s; [MinVersion=1] int32 y; };
+               """)
+    ])
+
+  def testWithUnmodifiedImport(self):
+    """Unchanged files in the filesystem are still parsed by the compatibility
+    checking tool if they're imported by a changed file."""
+    self.assertBackwardCompatible([
+        UnchangedFile('foo/foo.mojom', 'module foo; [Stable] struct S {};'),
+        Change('bar/bar.mojom',
+               old="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; };
+               """,
+               new="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; [MinVersion=1] int32 x; };
+               """)
+    ])
+
+    self.assertNotBackwardCompatible([
+        UnchangedFile('foo/foo.mojom', 'module foo; [Stable] struct S {};'),
+        Change('bar/bar.mojom',
+               old="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; };
+               """,
+               new="""\
+               module bar;
+               import "foo/foo.mojom";
+               [Stable] struct T { foo.S s; int32 x; };
+               """)
+    ])

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

@@ -524,6 +524,14 @@ class Struct(ReferenceKind):
     return self.attributes.get(ATTRIBUTE_STABLE, False) \
         if self.attributes else False
 
+  @property
+  def qualified_name(self):
+    if self.parent_kind:
+      prefix = self.parent_kind.qualified_name + '.'
+    else:
+      prefix = self.module.GetNamespacePrefix()
+    return '%s%s' % (prefix, self.mojom_name)
+
   def __eq__(self, rhs):
     return (isinstance(rhs, Struct) and
             (self.mojom_name, self.native_only, self.fields, self.constants,
@@ -632,6 +640,14 @@ class Union(ReferenceKind):
     return self.attributes.get(ATTRIBUTE_STABLE, False) \
         if self.attributes else False
 
+  @property
+  def qualified_name(self):
+    if self.parent_kind:
+      prefix = self.parent_kind.qualified_name + '.'
+    else:
+      prefix = self.module.GetNamespacePrefix()
+    return '%s%s' % (prefix, self.mojom_name)
+
   def __eq__(self, rhs):
     return (isinstance(rhs, Union) and
             (self.mojom_name, self.fields,
@@ -1086,6 +1102,14 @@ class Interface(ReferenceKind):
     return self.attributes.get(ATTRIBUTE_STABLE, False) \
         if self.attributes else False
 
+  @property
+  def qualified_name(self):
+    if self.parent_kind:
+      prefix = self.parent_kind.qualified_name + '.'
+    else:
+      prefix = self.module.GetNamespacePrefix()
+    return '%s%s' % (prefix, self.mojom_name)
+
   def __eq__(self, rhs):
     return (isinstance(rhs, Interface)
             and (self.mojom_name, self.methods, self.enums, self.constants,
@@ -1180,6 +1204,14 @@ class Enum(Kind):
     return self.attributes.get(ATTRIBUTE_STABLE, False) \
         if self.attributes else False
 
+  @property
+  def qualified_name(self):
+    if self.parent_kind:
+      prefix = self.parent_kind.qualified_name + '.'
+    else:
+      prefix = self.module.GetNamespacePrefix()
+    return '%s%s' % (prefix, self.mojom_name)
+
   def IsBackwardCompatible(self, older_enum):
     """This enum is backward-compatible with older_enum if and only if one of
     the following conditions holds: