Add global types (structs/enums/bitmaps) to matter file parsing (#34515)

* Make the IDL parser parse global types

* Restyle

* Parsing ok, started adding compatibility unit tests (which fail)

* backwards compatibility tests pass now

* Start implementing global type merging. Still needs unit tests

* Prepare matter tests

* A first test for global parsing

* Fix bugs and have a working unit test for the non-recursive bit

* Recursive unit test passes

* Fix lint

* Fix lint

* Add check for global type uniqueness

* Restyle

---------

Co-authored-by: Andrei Litvin <[email protected]>

scripts/py_matter_idl/matter_idl/backwards_compatibility.py

@@ -318,6 +318,12 @@ def check(self):
 
         self._check_cluster_list_compatible(
             self._original_idl.clusters, self._updated_idl.clusters)
+        self._check_enum_list_compatible(
+            "", self._original_idl.global_enums, self._updated_idl.global_enums)
+        self._check_bitmap_list_compatible(
+            "", self._original_idl.global_bitmaps, self._updated_idl.global_bitmaps)
+        self._check_struct_list_compatible(
+            "", self._original_idl.global_structs, self._updated_idl.global_structs)
 
         return self.compatible
 

scripts/py_matter_idl/matter_idl/generators/idl/MatterIdl.jinja

@@ -32,13 +32,65 @@
 
 // This IDL was auto-generated from a parsed data structure
 
-{%  for cluster in idl.clusters %}
+{% for enum in idl.global_enums %}
+enum {{enum.name}} : {{ enum.base_type}} {
+  {% for entry in enum.entries %}
+  {{entry.name}} = {{entry.code}};
+  {% endfor %}
+}
+
+{% endfor %}
+
+{%- for bitmap in idl.global_bitmaps %}
+bitmap {{bitmap.name}} : {{ bitmap.base_type}} {
+  {% for entry in bitmap.entries %}
+  {{entry.name}} = 0x{{"%X" | format(entry.code)}};
+  {% endfor %}
+}
+
+{% endfor %}
+
+{%- for s in idl.global_structs %}
+{{render_struct(s)}}
+
+{% endfor %}
+
+{%- for cluster in idl.clusters %}
 {% if cluster.description %}/** {{cluster.description}} */
 {% endif %}
 {{cluster.api_maturity | idltxt}}cluster {{cluster.name}} = {{cluster.code}} {
   revision {{cluster.revision}};
 
-  {% for enum in cluster.enums %}
+  {% for enum in cluster.enums | selectattr("is_global")%}
+  /* GLOBAL:
+  enum {{enum.name}} : {{ enum.base_type}} {
+    {% for entry in enum.entries %}
+    {{entry.name}} = {{entry.code}};
+    {% endfor %}
+  }
+  */
+
+  {% endfor %}
+
+  {%- for bitmap in cluster.bitmaps | selectattr("is_global")%}
+  /* GLOBAL:
+  bitmap {{bitmap.name}} : {{ bitmap.base_type}} {
+    {% for entry in bitmap.entries %}
+    {{entry.name}} = 0x{{"%X" | format(entry.code)}};
+    {% endfor %}
+  }
+  */
+
+  {% endfor %}
+
+  {%- for s in cluster.structs | selectattr("is_global") %}
+  /* GLOBAL:
+  {{render_struct(s)}}
+   */
+
+  {% endfor %}
+
+  {%- for enum in cluster.enums | rejectattr("is_global")%}
   enum {{enum.name}} : {{ enum.base_type}} {
     {% for entry in enum.entries %}
     {{entry.name}} = {{entry.code}};
@@ -47,7 +99,7 @@
 
   {% endfor %}
 
-  {%- for bitmap in cluster.bitmaps %}
+  {%- for bitmap in cluster.bitmaps | rejectattr("is_global")%}
   bitmap {{bitmap.name}} : {{ bitmap.base_type}} {
     {% for entry in bitmap.entries %}
     {{entry.name}} = 0x{{"%X" | format(entry.code)}};
@@ -56,7 +108,7 @@
 
   {% endfor %}
 
-  {%- for s in cluster.structs | rejectattr("tag") %}
+  {%- for s in cluster.structs | rejectattr("tag") | rejectattr("is_global") %}
   {{render_struct(s)}}
 
   {% endfor %}

scripts/py_matter_idl/matter_idl/matter_grammar.lark

@@ -116,7 +116,7 @@ POSITIVE_INTEGER: /\d+/
 HEX_INTEGER: /0x[A-Fa-f0-9]+/
 ID: /[a-zA-Z_][a-zA-Z0-9_]*/
 
-idl: (cluster|endpoint)*
+idl: (struct|enum|bitmap|cluster|endpoint)*
 
 %import common.ESCAPED_STRING
 %import common.WS

scripts/py_matter_idl/matter_idl/matter_idl_parser.py

@@ -1,8 +1,9 @@
 #!/usr/bin/env python
 
+import dataclasses
 import functools
 import logging
-from typing import Dict, Optional
+from typing import Dict, Optional, Set
 
 from lark import Lark
 from lark.lexer import Token
@@ -504,15 +505,25 @@ def idl(self, items):
         clusters = []
         endpoints = []
 
+        global_bitmaps = []
+        global_enums = []
+        global_structs = []
+
         for item in items:
             if isinstance(item, Cluster):
                 clusters.append(item)
             elif isinstance(item, Endpoint):
                 endpoints.append(item)
+            elif isinstance(item, Enum):
+                global_enums.append(dataclasses.replace(item, is_global=True))
+            elif isinstance(item, Bitmap):
+                global_bitmaps.append(dataclasses.replace(item, is_global=True))
+            elif isinstance(item, Struct):
+                global_structs.append(dataclasses.replace(item, is_global=True))
             else:
                 raise Exception("UNKNOWN idl content item: %r" % item)
 
-        return Idl(clusters=clusters, endpoints=endpoints)
+        return Idl(clusters=clusters, endpoints=endpoints, global_bitmaps=global_bitmaps, global_enums=global_enums, global_structs=global_structs)
 
     def prefix_doc_comment(self):
         print("TODO: prefix")
@@ -524,9 +535,92 @@ def c_comment(self, token: Token):
             self.doc_comments.append(PrefixCppDocComment(token))
 
 
+def _referenced_type_names(cluster: Cluster) -> Set[str]:
+    """
+    Return the names of all data types referenced by the given cluster.
+    """
+    types = set()
+    for s in cluster.structs:
+        for f in s.fields:
+            types.add(f.data_type.name)
+
+    for e in cluster.events:
+        for f in e.fields:
+            types.add(f.data_type.name)
+
+    for a in cluster.attributes:
+        types.add(a.definition.data_type.name)
+
+    return types
+
+
+class GlobalMapping:
+    """
+    Maintains global type mapping from an IDL
+    """
+
+    def __init__(self, idl: Idl):
+        self.bitmap_map = {b.name: b for b in idl.global_bitmaps}
+        self.enum_map = {e.name: e for e in idl.global_enums}
+        self.struct_map = {s.name: s for s in idl.global_structs}
+
+        self.global_types = set(self.bitmap_map.keys()).union(set(self.enum_map.keys())).union(set(self.struct_map.keys()))
+
+        # Spec does not enforce unique naming in bitmap/enum/struct, however in practice
+        # if we have both enum Foo and bitmap Foo for example, it would be impossible
+        # to disambiguate `attribute Foo foo = 1` for the actual type we want.
+        #
+        # As a result, we do not try to namespace this and just error out
+        if len(self.global_types) != len(self.bitmap_map) + len(self.enum_map) + len(self.struct_map):
+            raise ValueError("Global type names are not unique.")
+
+    def merge_global_types_into_cluster(self, cluster: Cluster) -> Cluster:
+        """
+        Merges all referenced global types (bitmaps/enums/structs) into the cluster types.
+        This happens recursively.
+        """
+        global_types_added = set()
+
+        changed = True
+        while changed:
+            changed = False
+            for type_name in _referenced_type_names(cluster):
+                if type_name not in self.global_types:
+                    continue  # not a global type name
+
+                if type_name in global_types_added:
+                    continue  # already added
+
+                # check if this is a global type
+                if type_name in self.bitmap_map:
+                    global_types_added.add(type_name)
+                    changed = True
+                    cluster.bitmaps.append(self.bitmap_map[type_name])
+                elif type_name in self.enum_map:
+                    global_types_added.add(type_name)
+                    changed = True
+                    cluster.enums.append(self.enum_map[type_name])
+                elif type_name in self.struct_map:
+                    global_types_added.add(type_name)
+                    changed = True
+                    cluster.structs.append(self.struct_map[type_name])
+
+        return cluster
+
+
+def _merge_global_types_into_clusters(idl: Idl) -> Idl:
+    """
+    Adds bitmaps/enums/structs from idl.global_* into clusters as long as
+    clusters reference those type names
+    """
+    mapping = GlobalMapping(idl)
+    return dataclasses.replace(idl, clusters=[mapping.merge_global_types_into_cluster(cluster) for cluster in idl.clusters])
+
+
 class ParserWithLines:
-    def __init__(self, skip_meta: bool):
+    def __init__(self, skip_meta: bool, merge_globals: bool):
         self.transformer = MatterIdlTransformer(skip_meta)
+        self.merge_globals = merge_globals
 
         # NOTE: LALR parser is fast. While Earley could parse more ambigous grammars,
         #       earley is much slower:
@@ -572,14 +666,28 @@ def parse(self, file: str, file_name: Optional[str] = None):
         for comment in self.transformer.doc_comments:
             comment.appply_to_idl(idl, file)
 
+        if self.merge_globals:
+            idl = _merge_global_types_into_clusters(idl)
+
         return idl
 
 
-def CreateParser(skip_meta: bool = False):
+def CreateParser(skip_meta: bool = False, merge_globals=True):
     """
     Generates a parser that will process a ".matter" file into a IDL
+
+    Arguments:
+       skip_meta - do not add metadata (line position) for items. Metadata is
+                   useful for error reporting, however it does not work well
+                   for unit test comparisons
+
+       merge_globals - places global items (enums/bitmaps/structs) into any
+                       clusters that reference them, so that cluster types
+                       are self-sufficient. Useful as a backwards-compatible
+                       code generation if global definitions are not supported.
+
     """
-    return ParserWithLines(skip_meta)
+    return ParserWithLines(skip_meta, merge_globals)
 
 
 if __name__ == '__main__':

scripts/py_matter_idl/matter_idl/matter_idl_types.py

@@ -162,6 +162,7 @@ class Struct:
     code: Optional[int] = None  # for responses only
     qualities: StructQuality = StructQuality.NONE
     api_maturity: ApiMaturity = ApiMaturity.STABLE
+    is_global: bool = False
 
 
 @dataclass
@@ -193,6 +194,7 @@ class Enum:
     base_type: str
     entries: List[ConstantEntry]
     api_maturity: ApiMaturity = ApiMaturity.STABLE
+    is_global: bool = False
 
 
 @dataclass
@@ -201,6 +203,7 @@ class Bitmap:
     base_type: str
     entries: List[ConstantEntry]
     api_maturity: ApiMaturity = ApiMaturity.STABLE
+    is_global: bool = False
 
 
 @dataclass
@@ -290,5 +293,10 @@ class Idl:
     clusters: List[Cluster] = field(default_factory=list)
     endpoints: List[Endpoint] = field(default_factory=list)
 
+    # Global types
+    global_bitmaps: List[Bitmap] = field(default_factory=list)
+    global_enums: List[Enum] = field(default_factory=list)
+    global_structs: List[Struct] = field(default_factory=list)
+
     # IDL file name is available only if parsing provides a file name
     parse_file_name: Optional[str] = field(default=None)

scripts/py_matter_idl/matter_idl/test_backwards_compatibility.py

@@ -90,6 +90,83 @@ def ValidateUpdate(self, name: str, old: str, new: str, flags: Compatibility):
             with self.subTest(direction="OLD-to-OLD"):
                 self._AssumeCompatiblity(old, old, old_idl, old_idl, True)
 
+    def test_global_enums_delete(self):
+        self.ValidateUpdate(
+            "delete a top level enum",
+            "enum A: ENUM8{} enum B: ENUM8{}",
+            "enum A: ENUM8{}",
+            Compatibility.FORWARD_FAIL)
+
+    def test_global_enums_change(self):
+        self.ValidateUpdate(
+            "change an enum type",
+            "enum A: ENUM8{}",
+            "enum A: ENUM16{}",
+            Compatibility.FORWARD_FAIL | Compatibility.BACKWARD_FAIL)
+
+    def test_global_enums_add_remove(self):
+        self.ValidateUpdate(
+            "Adding enum values is ok, removing values is not",
+            "enum A: ENUM8 {A = 1; B = 2;}",
+            "enum A: ENUM8 {A = 1; }",
+            Compatibility.FORWARD_FAIL)
+
+    def test_global_enums_code(self):
+        self.ValidateUpdate(
+            "Switching enum codes is never ok",
+            "enum A: ENUM8 {A = 1; B = 2; }",
+            "enum A: ENUM8 {A = 1; B = 3; }",
+            Compatibility.FORWARD_FAIL | Compatibility.BACKWARD_FAIL)
+
+    def test_global_bitmaps_delete(self):
+        self.ValidateUpdate(
+            "Deleting a global bitmap",
+            "bitmap A: BITMAP8{} bitmap B: BITMAP8{}",
+            "bitmap A: BITMAP8{} ",
+            Compatibility.FORWARD_FAIL)
+
+    def test_global_bitmaps_type(self):
+        self.ValidateUpdate(
+            "Changing a bitmap type is never ok",
+            " bitmap A: BITMAP8{}",
+            " bitmap A: BITMAP16{}",
+            Compatibility.FORWARD_FAIL | Compatibility.BACKWARD_FAIL)
+
+    def test_global_bitmap_values(self):
+        self.ValidateUpdate(
+            "Adding bitmap values is ok, removing values is not",
+            " bitmap A: BITMAP8 { kA = 0x01; kB = 0x02; } ",
+            " bitmap A: BITMAP8 { kA = 0x01; } ",
+            Compatibility.FORWARD_FAIL)
+
+    def test_global_struct_removal(self):
+        self.ValidateUpdate(
+            "Structure removal is not ok, but adding is ok",
+            "struct Foo {} struct Bar {} ",
+            "struct Foo {} ",
+            Compatibility.FORWARD_FAIL)
+
+    def test_global_struct_content_type_change(self):
+        self.ValidateUpdate(
+            "Changing structure data types is never ok",
+            "struct Foo { int32u x = 1; }",
+            "struct Foo { int64u x = 1; }",
+            Compatibility.FORWARD_FAIL | Compatibility.BACKWARD_FAIL)
+
+    def test_global_struct_content_rename_reorder(self):
+        self.ValidateUpdate(
+            "Structure content renames and reorder is ok.",
+            "struct Foo { int32u x = 1; int8u y = 2; }",
+            "struct Foo { int8u a = 2; int32u y = 1; }",
+            Compatibility.ALL_OK)
+
+    def test_global_struct_content_add_remove(self):
+        self.ValidateUpdate(
+            "Structure content change is not ok.",
+            "struct Foo { int32u x = 1; }",
+            "struct Foo { int32u x = 1; int8u y = 2; }",
+            Compatibility.FORWARD_FAIL | Compatibility.BACKWARD_FAIL)
+
     def test_basic_clusters_enum(self):
         self.ValidateUpdate(
             "Adding an enum is ok. Also validates code formatting",

scripts/py_matter_idl/matter_idl/test_data_model_xml.py

@@ -94,6 +94,7 @@ def assertIdlEqual(self, a: Idl, b: Idl):
                              tofile='expected.matter',
                              )
         self.assertEqual(a, b, '\n' + ''.join(delta))
+        self.fail("IDLs are not equal (above delta should have failed)")
 
     def testBasicInput(self):