[mojom] Introduce [Stable] attribute

Types tagged with the [Stable] attribute can only depend on builtin
mojom types or other user-defined types tagged with the [Stable]
attribute.

This attribute can be used to indicate that a mojom definition is safe
to rely upon in situations where version skew must be tolerated.

A follow-up CL will introduce a presubmit to ensure that types marked
[Stable] do not change in a way that breaks backward-compatibility.

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

mojo/public/tools/bindings/README.md

@@ -407,6 +407,17 @@ interesting attributes supported today.
     field, enum value, interface method, or method parameter was introduced.
     See [Versioning](#Versioning) for more details.
 
+**`[Stable]`**
+:   The `Stable` attribute specifies that a given mojom type or interface
+    definition can be considered stable over time, meaning it is safe to use for
+    things like persistent storage or communication between independent
+    version-skewed binaries. Stable definitions may only depend on builtin mojom
+    types or other stable definitions, and changes to such definitions MUST
+    preserve backward-compatibility through appropriate use of versioning.
+    Backward-compatibility of changes is enforced in the Chromium tree using a
+    strict presubmit check. See [Versioning](#Versioning) for more details on
+    backward-compatibility constraints.
+
 **`[EnableIf=value]`**
 :   The `EnableIf` attribute is used to conditionally enable definitions when
     the mojom is parsed. If the `mojom` target in the GN file does not include

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

@@ -173,11 +173,15 @@ def AddComputedData(module):
         interface.version = max(interface.version, method.min_version)
 
       method.param_struct = _GetStructFromMethod(method)
+      if interface.stable:
+        method.param_struct.attributes[mojom.ATTRIBUTE_STABLE] = True
       interface.version = max(interface.version,
                               method.param_struct.versions[-1].version)
 
       if method.response_parameters is not None:
         method.response_param_struct = _GetResponseStructFromMethod(method)
+        if interface.stable:
+          method.response_param_struct.attributes[mojom.ATTRIBUTE_STABLE] = True
         interface.version = max(
             interface.version,
             method.response_param_struct.versions[-1].version)
@@ -188,7 +192,9 @@ def AddComputedData(module):
     """Converts a method's parameters into the fields of a struct."""
     params_class = "%s_%s_Params" % (method.interface.mojom_name,
                                      method.mojom_name)
-    struct = mojom.Struct(params_class, module=method.interface.module)
+    struct = mojom.Struct(params_class,
+                          module=method.interface.module,
+                          attributes={})
     for param in method.parameters:
       struct.AddField(
           param.mojom_name,
@@ -202,7 +208,9 @@ def AddComputedData(module):
     """Converts a method's response_parameters into the fields of a struct."""
     params_class = "%s_%s_ResponseParams" % (method.interface.mojom_name,
                                              method.mojom_name)
-    struct = mojom.Struct(params_class, module=method.interface.module)
+    struct = mojom.Struct(params_class,
+                          module=method.interface.module,
+                          attributes={})
     for param in method.response_parameters:
       struct.AddField(
           param.mojom_name,

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

@@ -254,6 +254,7 @@ PRIMITIVES = (
 
 ATTRIBUTE_MIN_VERSION = 'MinVersion'
 ATTRIBUTE_EXTENSIBLE = 'Extensible'
+ATTRIBUTE_STABLE = 'Stable'
 ATTRIBUTE_SYNC = 'Sync'
 
 
@@ -518,6 +519,11 @@ class Struct(ReferenceKind):
 
     return True
 
+  @property
+  def stable(self):
+    return self.attributes.get(ATTRIBUTE_STABLE, False) \
+        if self.attributes else False
+
   def __eq__(self, rhs):
     return (isinstance(rhs, Struct) and
             (self.mojom_name, self.native_only, self.fields, self.constants,
@@ -621,6 +627,11 @@ class Union(ReferenceKind):
 
     return True
 
+  @property
+  def stable(self):
+    return self.attributes.get(ATTRIBUTE_STABLE, False) \
+        if self.attributes else False
+
   def __eq__(self, rhs):
     return (isinstance(rhs, Union) and
             (self.mojom_name, self.fields,
@@ -1070,6 +1081,11 @@ class Interface(ReferenceKind):
 
     return True
 
+  @property
+  def stable(self):
+    return self.attributes.get(ATTRIBUTE_STABLE, False) \
+        if self.attributes else False
+
   def __eq__(self, rhs):
     return (isinstance(rhs, Interface)
             and (self.mojom_name, self.methods, self.enums, self.constants,
@@ -1159,6 +1175,11 @@ class Enum(Kind):
     return self.attributes.get(ATTRIBUTE_EXTENSIBLE, False) \
         if self.attributes else False
 
+  @property
+  def stable(self):
+    return self.attributes.get(ATTRIBUTE_STABLE, False) \
+        if self.attributes else False
+
   def IsBackwardCompatible(self, older_enum):
     """This enum is backward-compatible with older_enum if and only if one of
     the following conditions holds:

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

@@ -692,6 +692,36 @@ def _AssignDefaultOrdinals(items):
       next_ordinal += 1
 
 
+def _AssertTypeIsStable(kind):
+  """Raises an error if a type is not stable, meaning it is composed of at least
+  one type that is not marked [Stable]."""
+
+  def assertDependencyIsStable(dependency):
+    if (mojom.IsEnumKind(dependency) or mojom.IsStructKind(dependency)
+        or mojom.IsUnionKind(dependency) or mojom.IsInterfaceKind(dependency)):
+      if not dependency.stable:
+        raise Exception(
+            '%s is marked [Stable] but cannot be stable because it depends on '
+            '%s, which is not marked [Stable].' %
+            (kind.mojom_name, dependency.mojom_name))
+    elif mojom.IsArrayKind(dependency) or mojom.IsAnyInterfaceKind(dependency):
+      assertDependencyIsStable(dependency.kind)
+    elif mojom.IsMapKind(dependency):
+      assertDependencyIsStable(dependency.key_kind)
+      assertDependencyIsStable(dependency.value_kind)
+
+  if mojom.IsStructKind(kind) or mojom.IsUnionKind(kind):
+    for field in kind.fields:
+      assertDependencyIsStable(field.kind)
+  elif mojom.IsInterfaceKind(kind):
+    for method in kind.methods:
+      for param in method.param_struct.fields:
+        assertDependencyIsStable(param.kind)
+      if method.response_param_struct:
+        for response_param in method.response_param_struct.fields:
+          assertDependencyIsStable(response_param.kind)
+
+
 def _Module(tree, path, imports):
   """
   Args:
@@ -791,6 +821,13 @@ def _Module(tree, path, imports):
       if method.response_param_struct:
         _AssignDefaultOrdinals(method.response_param_struct.fields)
 
+  # Ensure that all types marked [Stable] are actually stable. Enums are
+  # automatically OK since they don't depend on other definitions.
+  for kinds in (module.structs, module.unions, module.interfaces):
+    for kind in kinds:
+      if kind.stable:
+        _AssertTypeIsStable(kind)
+
   return module
 
 

mojo/public/tools/mojom/stable_attribute_unittest.py

+# 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.
+
+from mojom_parser_test_case import MojomParserTestCase
+
+from mojom.generate import module
+
+
+class StableAttributeTest(MojomParserTestCase):
+  """Tests covering usage of the [Stable] attribute."""
+
+  def testStableAttributeTagging(self):
+    """Verify that we recognize the [Stable] attribute on relevant definitions
+    and the resulting parser outputs are tagged accordingly."""
+    mojom = 'test.mojom'
+    self.WriteFile(
+        mojom, """\
+        [Stable] enum TestEnum { kFoo };
+        enum UnstableEnum { kBar };
+        [Stable] struct TestStruct { TestEnum a; };
+        struct UnstableStruct { UnstableEnum a; };
+        [Stable] union TestUnion { TestEnum a; TestStruct b; };
+        union UnstableUnion { UnstableEnum a; UnstableStruct b; };
+        [Stable] interface TestInterface { Foo(TestUnion x) => (); };
+        interface UnstableInterface { Foo(UnstableUnion x) => (); };
+        """)
+    self.ParseMojoms([mojom])
+
+    m = self.LoadModule(mojom)
+    self.assertEqual(2, len(m.enums))
+    self.assertTrue(m.enums[0].stable)
+    self.assertFalse(m.enums[1].stable)
+    self.assertEqual(2, len(m.structs))
+    self.assertTrue(m.structs[0].stable)
+    self.assertFalse(m.structs[1].stable)
+    self.assertEqual(2, len(m.unions))
+    self.assertTrue(m.unions[0].stable)
+    self.assertFalse(m.unions[1].stable)
+    self.assertEqual(2, len(m.interfaces))
+    self.assertTrue(m.interfaces[0].stable)
+    self.assertFalse(m.interfaces[1].stable)
+
+  def testStableStruct(self):
+    """A [Stable] struct is valid if all its fields are also stable."""
+    self.ExtractTypes('[Stable] struct S {};')
+    self.ExtractTypes('[Stable] struct S { int32 x; bool b; };')
+    self.ExtractTypes('[Stable] enum E { A }; [Stable] struct S { E e; };')
+    self.ExtractTypes('[Stable] struct S {}; [Stable] struct T { S s; };')
+    self.ExtractTypes(
+        '[Stable] struct S {}; [Stable] struct T { array<S> ss; };')
+    self.ExtractTypes(
+        '[Stable] interface F {}; [Stable] struct T { pending_remote<F> f; };')
+
+    with self.assertRaisesRegexp(Exception, 'because it depends on E'):
+      self.ExtractTypes('enum E { A }; [Stable] struct S { E e; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on X'):
+      self.ExtractTypes('struct X {}; [Stable] struct S { X x; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on T'):
+      self.ExtractTypes('struct T {}; [Stable] struct S { array<T> xs; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on T'):
+      self.ExtractTypes('struct T {}; [Stable] struct S { map<int32, T> xs; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on T'):
+      self.ExtractTypes('struct T {}; [Stable] struct S { map<T, int32> xs; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on F'):
+      self.ExtractTypes(
+          'interface F {}; [Stable] struct S { pending_remote<F> f; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on F'):
+      self.ExtractTypes(
+          'interface F {}; [Stable] struct S { pending_receiver<F> f; };')
+
+  def testStableUnion(self):
+    """A [Stable] union is valid if all its fields' types are also stable."""
+    self.ExtractTypes('[Stable] union U {};')
+    self.ExtractTypes('[Stable] union U { int32 x; bool b; };')
+    self.ExtractTypes('[Stable] enum E { A }; [Stable] union U { E e; };')
+    self.ExtractTypes('[Stable] struct S {}; [Stable] union U { S s; };')
+    self.ExtractTypes(
+        '[Stable] struct S {}; [Stable] union U { array<S> ss; };')
+    self.ExtractTypes(
+        '[Stable] interface F {}; [Stable] union U { pending_remote<F> f; };')
+
+    with self.assertRaisesRegexp(Exception, 'because it depends on E'):
+      self.ExtractTypes('enum E { A }; [Stable] union U { E e; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on X'):
+      self.ExtractTypes('struct X {}; [Stable] union U { X x; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on T'):
+      self.ExtractTypes('struct T {}; [Stable] union U { array<T> xs; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on T'):
+      self.ExtractTypes('struct T {}; [Stable] union U { map<int32, T> xs; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on T'):
+      self.ExtractTypes('struct T {}; [Stable] union U { map<T, int32> xs; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on F'):
+      self.ExtractTypes(
+          'interface F {}; [Stable] union U { pending_remote<F> f; };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on F'):
+      self.ExtractTypes(
+          'interface F {}; [Stable] union U { pending_receiver<F> f; };')
+
+  def testStableInterface(self):
+    """A [Stable] interface is valid if all its methods' parameter types are
+    stable, including response parameters where applicable."""
+    self.ExtractTypes('[Stable] interface F {};')
+    self.ExtractTypes('[Stable] interface F { A(int32 x); };')
+    self.ExtractTypes('[Stable] interface F { A(int32 x) => (bool b); };')
+    self.ExtractTypes("""\
+        [Stable] enum E { A, B, C };
+        [Stable] struct S {};
+        [Stable] interface F { A(E e, S s) => (bool b, array<S> s); };
+        """)
+
+    with self.assertRaisesRegexp(Exception, 'because it depends on E'):
+      self.ExtractTypes('enum E { A, B, C }; [Stable] interface F { A(E e); };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on E'):
+      self.ExtractTypes(
+          'enum E { A, B, C }; [Stable] interface F { A(int32 x) => (E e); };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on S'):
+      self.ExtractTypes(
+          'struct S {}; [Stable] interface F { A(int32 x) => (S s); };')
+    with self.assertRaisesRegexp(Exception, 'because it depends on S'):
+      self.ExtractTypes(
+          'struct S {}; [Stable] interface F { A(S s) => (bool b); };')