Extract mojom parser from bindings generator

This extracts the mojom parsing logic from mojom_bindings_generator.py
into a new mojom_parser.py script within the mojom tool directory.

Apart from simply moving the parsing logic to a new script, this also
changes how the parse result is persisted by an intermediate build step
for subsequent build steps to consume.

Prior to this change, the parsing step emitted a ".p" file for each
processed .mojom file. This was nothing more than a serialized version
of the parsed AST with [EnableIf] feature filtering applied. Every
subsequent step for the mojom in question would load this AST, then
recursively process its import dependencies while loading their ASTs in
turn, before finally processing all of the aggregated data (resolving
references, computing field packing data, etc) to produce a Module
object suitable for bindings generation.

This CL rolls most of that processing into the parsing step (with more
to follow in future CLs), and the resulting computed Module object is
persisted to a ".mojom-module" file.

To generate bindings for a given .mojom file, the generator can simply
call Module.Load() on the corresponding .mojom-module produced by the
parser. The loaded Module is sufficient to inform code generation
with no need to resolve dependencies, process other modules, or
perform other complex computations on the model.

Each mojom-module is thus entirely self-contained, replicating the full
contents of all transitive import dependencies. This means the build
consumes more disk space with intermediate build artifacts, but it
also means generation steps are faster and simpler.

This CL should result in no net changes to the final build outputs.

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

mojo/public/tools/bindings/mojom.gni

@@ -65,9 +65,8 @@ enable_scrambled_message_ids =
 
 _mojom_tools_root = "//mojo/public/tools"
 _mojom_library_root = "$_mojom_tools_root/mojom/mojom"
-mojom_generator_root = "$_mojom_tools_root/bindings"
-mojom_generator_script = "$mojom_generator_root/mojom_bindings_generator.py"
-mojom_generator_sources = [
+mojom_parser_script = "$_mojom_tools_root/mojom/mojom_parser.py"
+mojom_parser_sources = [
   "$_mojom_library_root/__init__.py",
   "$_mojom_library_root/error.py",
   "$_mojom_library_root/generate/__init__.py",
@@ -81,13 +80,19 @@ mojom_generator_sources = [
   "$_mojom_library_root/parse/ast.py",
   "$_mojom_library_root/parse/lexer.py",
   "$_mojom_library_root/parse/parser.py",
-  "$mojom_generator_root/generators/mojom_cpp_generator.py",
-  "$mojom_generator_root/generators/mojom_java_generator.py",
-  "$mojom_generator_root/generators/mojom_js_generator.py",
-  "$mojom_generator_root/generators/mojom_ts_generator.py",
-  "$mojom_generator_script",
 ]
 
+mojom_generator_root = "$_mojom_tools_root/bindings"
+mojom_generator_script = "$mojom_generator_root/mojom_bindings_generator.py"
+mojom_generator_sources =
+    mojom_parser_sources + [
+      "$mojom_generator_root/generators/mojom_cpp_generator.py",
+      "$mojom_generator_root/generators/mojom_java_generator.py",
+      "$mojom_generator_root/generators/mojom_js_generator.py",
+      "$mojom_generator_root/generators/mojom_ts_generator.py",
+      "$mojom_generator_script",
+    ]
+
 if (enable_scrambled_message_ids) {
   declare_args() {
     # The path to a file whose contents can be used as the basis for a message
@@ -600,8 +605,22 @@ template("mojom") {
       (!defined(invoker.enable_fuzzing) || invoker.enable_fuzzing) &&
       enable_ipc_fuzzer && (!defined(invoker.testonly) || !invoker.testonly)
 
-  if (sources_list != []) {
-    parser_target_name = "${target_name}__parser"
+  parser_target_name = "${target_name}__parser"
+  parser_deps = []
+  foreach(dep, all_deps) {
+    _label = get_label_info(dep, "label_no_toolchain")
+    parser_deps += [ "${_label}__parser" ]
+  }
+  if (defined(invoker.parser_deps)) {
+    parser_deps += invoker.parser_deps
+  }
+  if (sources_list == []) {
+    # Even without sources we generate a parser target to at least forward
+    # other parser dependencies.
+    group(parser_target_name) {
+      public_deps = parser_deps
+    }
+  } else {
     enabled_features = []
     if (defined(invoker.enabled_features)) {
       enabled_features += invoker.enabled_features
@@ -626,57 +645,46 @@ template("mojom") {
     }
 
     action(parser_target_name) {
-      script = mojom_generator_script
-      inputs = mojom_generator_sources + jinja2_sources
+      script = mojom_parser_script
+      inputs = mojom_parser_sources
       sources = sources_list
-      if (defined(invoker.parser_deps)) {
-        deps = invoker.parser_deps
-      }
+      deps = parser_deps
       outputs = []
-      filelist = []
-      foreach(source, sources_list) {
-        filelist += [ rebase_path(source, root_build_dir) ]
-      }
       foreach(base_path, output_file_base_paths) {
-        filename = get_path_info(base_path, "name")
+        filename = get_path_info(base_path, "file")
         dirname = get_path_info(base_path, "dir")
-        outputs += [ "$root_gen_dir/$dirname/$filename.p" ]
+        outputs += [ "$root_gen_dir/$dirname/${filename}-module" ]
       }
 
+      filelist = []
+      foreach(source, sources_list) {
+        filelist += [ rebase_path(source) ]
+      }
       response_file_contents = filelist
 
       args = [
-        "-o",
-        rebase_path(root_gen_dir, root_build_dir),
-        "parse",
-        "--filelist={{response_file_name}}",
-        "-d",
-        rebase_path("//", root_build_dir),
+        # Resolve relative input mojom paths against both the root src dir and
+        # the root gen dir.
+        "--input-root",
+        rebase_path("//"),
+        "--input-root",
+        rebase_path(root_gen_dir),
+
+        "--output-root",
+        rebase_path(root_gen_dir),
+
+        "--mojom-file-list={{response_file_name}}",
       ]
+
       foreach(enabled_feature, enabled_features) {
         args += [
-          "--enable_feature",
+          "--enable-feature",
           enabled_feature,
         ]
       }
     }
   }
 
-  parsed_target_name = "${target_name}__parsed"
-  group(parsed_target_name) {
-    public_deps = []
-    if (sources_list != []) {
-      public_deps += [ ":$parser_target_name" ]
-    }
-    foreach(d, all_deps) {
-      # Resolve the name, so that a target //mojo/something becomes
-      # //mojo/something:something and we can append the parsed
-      # suffix to get the mojom dependency name.
-      full_name = get_label_info("$d", "label_no_toolchain")
-      public_deps += [ "${full_name}__parsed" ]
-    }
-  }
-
   if (sources_list != []) {
     verify_deps_target_names = []
     verify_deps_target_name = "${target_name}__verify_deps"
@@ -687,7 +695,7 @@ template("mojom") {
       script = mojom_generator_script
       inputs = mojom_generator_sources + jinja2_sources
       sources = sources_list
-      deps = [ ":$parsed_target_name" ]
+      deps = [ ":$parser_target_name" ]
       if (defined(invoker.parser_deps)) {
         deps += invoker.parser_deps
       }
@@ -789,7 +797,7 @@ template("mojom") {
       inputs = mojom_generator_sources + jinja2_sources
       sources = sources_list
       deps = [
-        ":$parsed_target_name",
+        ":$parser_target_name",
         "//mojo/public/tools/bindings:precompile_templates",
       ]
       if (defined(invoker.parser_deps)) {
@@ -829,7 +837,7 @@ template("mojom") {
       inputs = mojom_generator_sources + jinja2_sources
       sources = sources_list
       deps = [
-               ":$parsed_target_name",
+               ":$parser_target_name",
                "//mojo/public/tools/bindings:precompile_templates",
              ] + verify_deps_target_names
       if (defined(invoker.parser_deps)) {
@@ -1089,7 +1097,7 @@ template("mojom") {
         inputs = mojom_generator_sources + jinja2_sources
         sources = sources_list
         deps = [
-                 ":$parsed_target_name",
+                 ":$parser_target_name",
                  ":$type_mappings_target_name",
                  "//mojo/public/tools/bindings:precompile_templates",
                ] + verify_deps_target_names
@@ -1430,7 +1438,7 @@ template("mojom") {
           inputs = mojom_generator_sources + jinja2_sources
           sources = sources_list
           deps = [
-                   ":$parsed_target_name",
+                   ":$parser_target_name",
                    ":$type_mappings_target_name",
                    "//mojo/public/tools/bindings:precompile_templates",
                  ] + verify_deps_target_names
@@ -1527,7 +1535,7 @@ template("mojom") {
         inputs = mojom_generator_sources + jinja2_sources
         sources = sources_list
         deps = [
-                 ":$parsed_target_name",
+                 ":$parser_target_name",
                  "//mojo/public/tools/bindings:precompile_templates",
                ] + verify_deps_target_names
         if (defined(invoker.parser_deps)) {
@@ -1691,7 +1699,7 @@ template("mojom") {
       inputs = mojom_generator_sources + jinja2_sources
       sources = sources_list
       deps = [
-               ":$parsed_target_name",
+               ":$parser_target_name",
                "//mojo/public/tools/bindings:precompile_templates",
              ] + verify_deps_target_names
       outputs = ts_outputs

mojo/public/tools/bindings/mojom_bindings_generator.py

@@ -9,11 +9,6 @@ from __future__ import print_function
 
 import argparse
 
-try:
-  import cPickle as pickle
-except ImportError:
-  import pickle
-
 import hashlib
 import importlib
 import json
@@ -44,11 +39,10 @@ sys.path.insert(
 
 from mojom.error import Error
 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.parse.conditional_features import RemoveDisabledDefinitions
-from mojom.parse.parser import Parse
 
 sys.path.append(
     os.path.join(_GetDirAbove("mojo"), "tools", "diagnosis"))
@@ -104,16 +98,8 @@ class RelativePath(object):
         os.path.abspath(self.path), os.path.abspath(self.root))
 
 
-def FindImportFile(args, rel_dir, file_name, search_rel_dirs):
-  """Finds |file_name| in either |rel_dir| or |search_rel_dirs|. Returns a
-  RelativePath with first file found, or an arbitrary non-existent file
-  otherwise."""
-  for rel_search_dir in [rel_dir] + search_rel_dirs:
-    path = os.path.join(rel_search_dir.path, file_name)
-    if os.path.isfile(path):
-      return RelativePath(path, rel_search_dir.root, args.output_dir)
-  return RelativePath(
-      os.path.join(rel_dir.path, file_name), rel_dir.root, args.output_dir)
+def _GetModulePath(path, output_dir):
+  return os.path.join(output_dir, path.relative_path() + '-module')
 
 
 def ScrambleMethodOrdinals(interfaces, salt):
@@ -154,7 +140,7 @@ def ReadFileContents(filename):
 
 
 class MojomProcessor(object):
-  """Parses mojom files and creates ASTs for them.
+  """Takes parsed mojom modules and generates language bindings from them.
 
   Attributes:
     _processed_files: {Dict[str, mojom.generate.module.Module]} Mapping from
@@ -189,26 +175,9 @@ class MojomProcessor(object):
           MakeImportStackMessage(imported_filename_stack + [rel_filename.path]))
       sys.exit(1)
 
-    tree = _UnpickleAST(_FindPicklePath(rel_filename, args.gen_directories +
-                                        [args.output_dir]))
-    dirname = os.path.dirname(rel_filename.path)
-
-    # Process all our imports first and collect the module object for each.
-    # We use these to generate proper type info.
-    imports = {}
-    for parsed_imp in tree.import_list:
-      rel_import_file = FindImportFile(
-          args, RelativePath(dirname, rel_filename.root, args.output_dir),
-          parsed_imp.import_filename, args.import_directories)
-      imports[parsed_imp.import_filename] = self._GenerateModule(
-          args, remaining_args, generator_modules, rel_import_file,
-          imported_filename_stack + [rel_filename.path])
-
-    # Set the module path as relative to the source root.
-    # Normalize to unix-style path here to keep the generators simpler.
-    module_path = rel_filename.relative_path().replace('\\', '/')
-
-    module = translate.OrderedModule(tree, module_path, imports)
+    module_path = _GetModulePath(rel_filename, args.output_dir)
+    with open(module_path, 'rb') as f:
+      module = Module.Load(f)
 
     if args.scrambled_message_id_salt_paths:
       salt = b''.join(
@@ -278,71 +247,6 @@ def _Generate(args, remaining_args):
   return 0
 
 
-def _FindPicklePath(rel_filename, search_dirs):
-  filename, _ = os.path.splitext(rel_filename.relative_path())
-  pickle_path = filename + '.p'
-  for search_dir in search_dirs:
-    path = os.path.join(search_dir, pickle_path)
-    if os.path.isfile(path):
-      return path
-  raise Exception("%s: Error: Could not find file in %r" %
-                  (pickle_path, search_dirs))
-
-
-def _GetPicklePath(rel_filename, output_dir):
-  filename, _ = os.path.splitext(rel_filename.relative_path())
-  pickle_path = filename + '.p'
-  return os.path.join(output_dir, pickle_path)
-
-
-def _PickleAST(ast, output_file):
-  full_dir = os.path.dirname(output_file)
-  fileutil.EnsureDirectoryExists(full_dir)
-
-  try:
-    WriteFile(pickle.dumps(ast), output_file)
-  except (IOError, pickle.PicklingError) as e:
-    print("%s: Error: %s" % (output_file, str(e)))
-    sys.exit(1)
-
-def _UnpickleAST(input_file):
-  try:
-    with open(input_file, "rb") as f:
-      return pickle.load(f)
-  except (IOError, pickle.UnpicklingError) as e:
-    print("%s: Error: %s" % (input_file, str(e)))
-    sys.exit(1)
-
-
-def _ParseFile(args, rel_filename):
-  try:
-    with open(rel_filename.path) as f:
-      source = f.read()
-  except IOError as e:
-    print("%s: Error: %s" % (rel_filename.path, e.strerror))
-    sys.exit(1)
-
-  try:
-    tree = Parse(source, rel_filename.path)
-    RemoveDisabledDefinitions(tree, args.enabled_features)
-  except Error as e:
-    print("%s: Error: %s" % (rel_filename.path, str(e)))
-    sys.exit(1)
-  _PickleAST(tree, _GetPicklePath(rel_filename, args.output_dir))
-
-
-def _Parse(args, _):
-  fileutil.EnsureDirectoryExists(args.output_dir)
-
-  if args.filelist:
-    with open(args.filelist) as f:
-      args.filename.extend(f.read().split())
-
-  for filename in args.filename:
-    _ParseFile(args, RelativePath(filename, args.depth, args.output_dir))
-  return 0
-
-
 def _Precompile(args, _):
   generator_modules = LoadGenerators(",".join(_BUILTIN_GENERATORS.keys()))
 
@@ -376,11 +280,10 @@ def _VerifyImportDeps(args, __):
 
   for filename in args.filename:
     rel_path = RelativePath(filename, args.depth, args.output_dir)
-    tree = _UnpickleAST(_GetPicklePath(rel_path, args.output_dir))
-
-    mojom_imports = set(
-      parsed_imp.import_filename for parsed_imp in tree.import_list
-      )
+    module_path = _GetModulePath(rel_path, args.output_dir)
+    with open(module_path, 'rb') as f:
+      module = Module.Load(f)
+      mojom_imports = set(imp.path for imp in module.imports)
 
     sources = set()
 
@@ -421,23 +324,6 @@ def main():
 
   subparsers = parser.add_subparsers()
 
-  parse_parser = subparsers.add_parser(
-      "parse", description="Parse mojom to AST and remove disabled definitions."
-                           " Pickle pruned AST into output_dir.")
-  parse_parser.add_argument("filename", nargs="*", help="mojom input file")
-  parse_parser.add_argument("--filelist", help="mojom input file list")
-  parse_parser.add_argument(
-      "-d", "--depth", dest="depth", default=".", help="depth from source root")
-  parse_parser.add_argument(
-      "--enable_feature",
-      dest = "enabled_features",
-      default=[],
-      action="append",
-      help="Controls which definitions guarded by an EnabledIf attribute "
-      "will be enabled. If an EnabledIf attribute does not specify a value "
-      "that matches one of the enabled features, it will be disabled.")
-  parse_parser.set_defaults(func=_Parse)
-
   generate_parser = subparsers.add_parser(
       "generate", description="Generate bindings from mojom files.")
   generate_parser.add_argument("filename", nargs="*",

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

@@ -292,12 +292,15 @@ def _Struct(module, parsed_struct):
             lambda enum: _Enum(module, enum, struct),
             _ElemsOfType(parsed_struct.body, ast.Enum,
                          parsed_struct.mojom_name)))
-    struct.constants = map(
-        lambda constant: _Constant(module, constant, struct),
-        _ElemsOfType(parsed_struct.body, ast.Const, parsed_struct.mojom_name))
+    struct.constants = list(
+        map(
+            lambda constant: _Constant(module, constant, struct),
+            _ElemsOfType(parsed_struct.body, ast.Const,
+                         parsed_struct.mojom_name)))
     # Stash fields parsed_struct here temporarily.
     struct.fields_data = _ElemsOfType(parsed_struct.body, ast.StructField,
                                       parsed_struct.mojom_name)
+
   struct.attributes = _AttributeListToDict(parsed_struct.attribute_list)
 
   # Enforce that a [Native] attribute is set to make native-only struct
@@ -446,9 +449,9 @@ def _Interface(module, parsed_iface):
   interface.enums = list(
       map(lambda enum: _Enum(module, enum, interface),
           _ElemsOfType(parsed_iface.body, ast.Enum, parsed_iface.mojom_name)))
-  interface.constants = map(
-      lambda constant: _Constant(module, constant, interface),
-      _ElemsOfType(parsed_iface.body, ast.Const, parsed_iface.mojom_name))
+  interface.constants = list(
+      map(lambda constant: _Constant(module, constant, interface),
+          _ElemsOfType(parsed_iface.body, ast.Const, parsed_iface.mojom_name)))
   # Stash methods parsed_iface here temporarily.
   interface.methods_data = _ElemsOfType(parsed_iface.body, ast.Method,
                                         parsed_iface.mojom_name)

mojo/public/tools/mojom/mojom_parser_unittest.py

+#!/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 os
+import os.path
+import shutil
+import tempfile
+import unittest
+
+import mojom_parser
+
+from mojom.generate import module
+
+
+class MojomParserTest(unittest.TestCase):
+  """Tests covering the behavior defined by the main mojom_parser.py script.
+  This includes behavior around input and output path manipulation, dependency
+  resolution, and module serialization and deserialization."""
+
+  def __init__(self, method_name):
+    super(MojomParserTest, self).__init__(method_name)
+    self._temp_dir = None
+
+  def setUp(self):
+    self._temp_dir = tempfile.mkdtemp()
+
+  def tearDown(self):
+    shutil.rmtree(self._temp_dir)
+    self._temp_dir = None
+
+  def GetPath(self, path):
+    assert not os.path.isabs(path)
+    return os.path.join(self._temp_dir, path)
+
+  def GetModulePath(self, path):
+    assert not os.path.isabs(path)
+    return os.path.join(self.GetPath('out'), path) + '-module'
+
+  def WriteFile(self, path, contents):
+    full_path = self.GetPath(path)
+    dirname = os.path.dirname(full_path)
+    if not os.path.exists(dirname):
+      os.makedirs(dirname)
+    with open(full_path, 'w') as f:
+      f.write(contents)
+
+  def LoadModule(self, mojom_path):
+    with open(self.GetModulePath(mojom_path), 'rb') as f:
+      return module.Module.Load(f)
+
+  def ParseMojoms(self, mojoms):
+    """Parse all input mojoms relative the temp dir."""
+    out_dir = self.GetPath('out')
+    mojom_parser.ParseMojoms(
+        map(lambda mojom: os.path.join(self._temp_dir, mojom), mojoms),
+        [self._temp_dir, out_dir], out_dir, [])
+
+  def testBasicParse(self):
+    """Basic test to verify that we can parse a mojom file and get a module."""
+    mojom = 'foo/bar.mojom'
+    self.WriteFile(
+        mojom, """\
+        module test;
+        enum TestEnum { kFoo };
+        """)
+    self.ParseMojoms([mojom])
+
+    m = self.LoadModule(mojom)
+    self.assertEqual('foo/bar.mojom', m.path)
+    self.assertEqual('test', m.mojom_namespace)
+    self.assertEqual(1, len(m.enums))
+
+  def testBasicParseWithAbsolutePaths(self):
+    """Verifies that we can parse a mojom file given an absolute path input."""
+    mojom = 'foo/bar.mojom'
+    self.WriteFile(
+        mojom, """\
+        module test;
+        enum TestEnum { kFoo };
+        """)
+    self.ParseMojoms([self.GetPath(mojom)])
+
+    m = self.LoadModule(mojom)
+    self.assertEqual('foo/bar.mojom', m.path)
+    self.assertEqual('test', m.mojom_namespace)
+    self.assertEqual(1, len(m.enums))
+
+  def testImport(self):
+    """Verify imports within the same set of mojom inputs."""
+    a = 'a.mojom'
+    b = 'b.mojom'
+    self.WriteFile(
+        a, """\
+        module a;
+        import "b.mojom";
+        struct Foo { b.Bar bar; };""")
+    self.WriteFile(b, """\
+        module b;
+        struct Bar {};""")
+    self.ParseMojoms([a, b])
+
+    ma = self.LoadModule(a)
+    mb = self.LoadModule(b)
+    self.assertEqual('a.mojom', ma.path)
+    self.assertEqual('b.mojom', mb.path)
+    self.assertEqual(1, len(ma.imports))
+    self.assertEqual(mb, ma.imports[0])
+
+  def testPreProcessedImport(self):
+    """Verify imports processed by a previous parser execution can be loaded
+    properly when parsing a dependent mojom."""
+    a = 'a.mojom'
+    self.WriteFile(a, """\
+        module a;
+        struct Bar {};""")
+    self.ParseMojoms([a])
+
+    b = 'b.mojom'
+    self.WriteFile(
+        b, """\
+        module b;
+        import "a.mojom";
+        struct Foo { a.Bar bar; };""")
+    self.ParseMojoms([b])
+
+  def testMissingImport(self):
+    """Verify that an import fails if the imported mojom does not exist."""
+    a = 'a.mojom'
+    self.WriteFile(
+        a, """\
+        module a;
+        import "non-existent.mojom";
+        struct Bar {};""")
+    with self.assertRaisesRegexp(ValueError, "does not exist"):
+      self.ParseMojoms([a])
+
+  def testUnparsedImport(self):
+    """Verify that an import fails if the imported mojom is not in the set of
+    mojoms provided to the parser on this execution AND there is no pre-existing
+    parsed output module already on disk for it."""
+    a = 'a.mojom'
+    b = 'b.mojom'
+    self.WriteFile(a, """\
+        module a;
+        struct Bar {};""")
+    self.WriteFile(
+        b, """\
+        module b;
+        import "a.mojom";
+        struct Foo { a.Bar bar; };""")
+
+    # a.mojom has not been parsed yet, so its import will fail when processing
+    # b.mojom here.
+    with self.assertRaisesRegexp(ValueError, "does not exist"):
+      self.ParseMojoms([b])