pw_protobuf: Put codegen in a ::pwpb suffix namespace

The namespace suffix is applied at the package level, as specified in
the originating .proto file.

This change is mostly in order to avoid clashing with code generated by
the stock C++ representation outputted by protoc. This is useful, for
example, for projects that would like to use the stock C++ proto library
for additional tests, but which need to use pw_protobuf due to needs of
embedded targets.

For now, this change is set up in a way so that it is, by default,
backwards compatible with existing code. A flag has been added to the
plugin to opt out of legacy behaviour, but has not been implemented in
any rules that invoke the Pigweed proto compiler plugin.

Because of limitations with the current proto plugin implementation,
some external identifiers (references to things in other .proto files,
typically) are not parsed with a package annotation, and so we are
unable to codegen the ::pwpb suffix in the correct place. For now, I've
introduced an auxiliary namespace (discussed with [email protected],
seems like the best fast workaround).

In the long term, potentially we can coax the plugin to traverse the
build graph better. (Package or namespace overrides are supported for
Java / Obj-C / C#, so presumably some mechanism is available.)

b/250945489

Change-Id: Ic1fae107063ec75d81da34baa89be61f66d4f94d
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/117934
Reviewed-by: Wyatt Hepler <[email protected]>
Commit-Queue: Anqi Dong <[email protected]>

pw_protobuf/py/pw_protobuf/codegen_pwpb.py

@@ -29,6 +29,7 @@
 from pw_protobuf.proto_tree import ProtoEnum, ProtoMessage, ProtoMessageField
 from pw_protobuf.proto_tree import ProtoNode
 from pw_protobuf.proto_tree import build_node_tree
+from pw_protobuf.proto_tree import EXTERNAL_SYMBOL_WORKAROUND_NAMESPACE
 
 PLUGIN_NAME = 'pw_protobuf'
 PLUGIN_VERSION = '0.1.0'
@@ -125,6 +126,7 @@ def _relative_type_namespace(self, from_root: bool = False) -> str:
 
         ancestor = scope.common_ancestor(type_node)
         namespace = type_node.cpp_namespace(ancestor)
+
         assert namespace
         return namespace
 
@@ -1854,7 +1856,7 @@ def generate_class_for_message(message: ProtoMessage, root: ProtoNode,
     # and use its constructor.
     base_class = f'{PROTOBUF_NAMESPACE}::{base_class_name}'
     output.write_line(
-        f'class {message.cpp_namespace(root)}::{class_name} ' \
+        f'class {message.cpp_namespace(root=root)}::{class_name} ' \
         f': public {base_class} {{'
     )
     output.write_line(' public:')
@@ -1956,7 +1958,7 @@ def define_not_in_class_methods(message: ProtoMessage, root: ProtoNode,
                 continue
 
             output.write_line()
-            class_name = (f'{message.cpp_namespace(root)}::'
+            class_name = (f'{message.cpp_namespace(root=root)}::'
                           f'{class_type.codegen_class_name()}')
             method_signature = (
                 f'inline {method.return_type(from_root=True)} '
@@ -2006,7 +2008,7 @@ def generate_code_for_enum(proto_enum: ProtoEnum, root: ProtoNode,
     assert proto_enum.type() == ProtoNode.Type.ENUM
 
     common_prefix = _common_value_prefix(proto_enum)
-    output.write_line(f'enum class {proto_enum.cpp_namespace(root)} '
+    output.write_line(f'enum class {proto_enum.cpp_namespace(root=root)} '
                       f': uint32_t {{')
     with output.indent():
         for name, number in proto_enum.values():
@@ -2025,7 +2027,7 @@ def generate_function_for_enum(proto_enum: ProtoEnum, root: ProtoNode,
     """Creates a C++ validation function for for a proto enum."""
     assert proto_enum.type() == ProtoNode.Type.ENUM
 
-    enum_name = proto_enum.cpp_namespace(root)
+    enum_name = proto_enum.cpp_namespace(root=root)
     output.write_line(
         f'constexpr bool IsValid{enum_name}({enum_name} value) {{')
     with output.indent():
@@ -2041,7 +2043,7 @@ def generate_function_for_enum(proto_enum: ProtoEnum, root: ProtoNode,
 def forward_declare(node: ProtoMessage, root: ProtoNode,
                     output: OutputFile) -> None:
     """Generates code forward-declaring entities in a message's namespace."""
-    namespace = node.cpp_namespace(root)
+    namespace = node.cpp_namespace(root=root)
     output.write_line()
     output.write_line(f'namespace {namespace} {{')
 
@@ -2081,7 +2083,7 @@ def generate_struct_for_message(message: ProtoMessage, root: ProtoNode,
     """Creates a C++ struct to hold a protobuf message values."""
     assert message.type() == ProtoNode.Type.MESSAGE
 
-    output.write_line(f'struct {message.cpp_namespace(root)}::Message {{')
+    output.write_line(f'struct {message.cpp_namespace(root=root)}::Message {{')
 
     # Generate members for each of the message's fields.
     with output.indent():
@@ -2119,7 +2121,7 @@ def generate_table_for_message(message: ProtoMessage, root: ProtoNode,
     """Creates a C++ array to hold a protobuf message description."""
     assert message.type() == ProtoNode.Type.MESSAGE
 
-    namespace = message.cpp_namespace(root)
+    namespace = message.cpp_namespace(root=root)
     output.write_line(f'namespace {namespace} {{')
 
     properties = []
@@ -2180,7 +2182,7 @@ def generate_sizes_for_message(message: ProtoMessage, root: ProtoNode,
     """Creates C++ constants for the encoded sizes of a protobuf message."""
     assert message.type() == ProtoNode.Type.MESSAGE
 
-    namespace = message.cpp_namespace(root)
+    namespace = message.cpp_namespace(root=root)
     output.write_line(f'namespace {namespace} {{')
 
     property_sizes: List[str] = []
@@ -2255,7 +2257,8 @@ def dependency_sorted_messages(package: ProtoNode):
 
 
 def generate_code_for_package(file_descriptor_proto, package: ProtoNode,
-                              output: OutputFile) -> None:
+                              output: OutputFile,
+                              suppress_legacy_namespace: bool) -> None:
     """Generates code for a single .pb.h file corresponding to a .proto file."""
 
     assert package.type() == ProtoNode.Type.PACKAGE
@@ -2338,8 +2341,37 @@ def generate_code_for_package(file_descriptor_proto, package: ProtoNode,
     if package.cpp_namespace():
         output.write_line(f'\n}}  // namespace {package.cpp_namespace()}')
 
+        # Aliasing namespaces aren't needed if `package.cpp_namespace()` is
+        # empty (since everyone can see the global namespace). It shouldn't
+        # ever be empty, though.
+
+        if not suppress_legacy_namespace:
+            output.write_line()
+            output.write_line('// Aliases for legacy pwpb codegen interface. '
+                              'Please use the')
+            output.write_line('// `::pwpb`-suffixed names in new code.')
+            legacy_namespace = package.cpp_namespace(codegen_subnamespace=None)
+            output.write_line(f'namespace {legacy_namespace} {{')
+            output.write_line(f'using namespace ::{package.cpp_namespace()};')
+            output.write_line(f'}}  // namespace {legacy_namespace}')
+
+        # TODO(b/250945489) Remove this if possible
+        output.write_line()
+        output.write_line(
+            '// Codegen implementation detail; do not use this namespace!')
+
+        external_lookup_namespace = "{}::{}".format(
+            EXTERNAL_SYMBOL_WORKAROUND_NAMESPACE,
+            package.cpp_namespace(codegen_subnamespace=None))
+
+        output.write_line(f'namespace {external_lookup_namespace} {{')
+        output.write_line(f'using namespace ::{package.cpp_namespace()};')
+        output.write_line(f'}}  // namespace {external_lookup_namespace}')
+
 
-def process_proto_file(proto_file, proto_options) -> Iterable[OutputFile]:
+def process_proto_file(
+        proto_file, proto_options,
+        suppress_legacy_namespace: bool) -> Iterable[OutputFile]:
     """Generates code for a single .proto file."""
 
     # Two passes are made through the file. The first builds the tree of all
@@ -2350,6 +2382,7 @@ def process_proto_file(proto_file, proto_options) -> Iterable[OutputFile]:
 
     output_filename = _proto_filename_to_generated_header(proto_file.name)
     output_file = OutputFile(output_filename)
-    generate_code_for_package(proto_file, package_root, output_file)
+    generate_code_for_package(proto_file, package_root, output_file,
+                              suppress_legacy_namespace)
 
     return [output_file]

pw_protobuf/py/pw_protobuf/plugin.py

@@ -29,7 +29,12 @@
 
 
 def parse_parameter_options(parameter: str) -> Namespace:
-    """Parse parameters specified with --custom_opt="""
+    """Parses parameters passed through from protoc.
+
+    These parameters come in via passing `--${NAME}_out` parameters to protoc,
+    where protoc-gen-${NAME} is the supplied name of the plugin. At time of
+    writing, Blaze uses --pwpb_opt, whereas the script for GN uses --custom_opt.
+    """
     parser = ArgumentParser()
     parser.add_argument('-I',
                         '--include-path',
@@ -38,11 +43,17 @@ def parse_parameter_options(parameter: str) -> Namespace:
                         action='append',
                         default=[],
                         type=Path,
-                        help="Append DIR to options file search path")
-
-    # protoc passes the --custom_opt arguments in shell quoted form, separated
-    # by commas. Use shlex to split them, correctly handling quoted sections,
-    # with equivalent options to IFS=","
+                        help='Append DIR to options file search path')
+    parser.add_argument(
+        '--no-legacy-namespace',
+        dest='no_legacy_namespace',
+        action='store_true',
+        help='If set, suppresses `using namespace` declarations, which '
+        'disallows use of the legacy non-prefixed namespace')
+
+    # protoc passes the custom arguments in shell quoted form, separated by
+    # commas. Use shlex to split them, correctly handling quoted sections, with
+    # equivalent options to IFS=","
     lex = shlex(parameter)
     lex.whitespace_split = True
     lex.whitespace = ','
@@ -67,8 +78,10 @@ def process_proto_request(req: plugin_pb2.CodeGeneratorRequest,
     for proto_file in req.proto_file:
         proto_options = options.load_options(args.include_paths,
                                              Path(proto_file.name))
-        output_files = codegen_pwpb.process_proto_file(proto_file,
-                                                       proto_options)
+        output_files = codegen_pwpb.process_proto_file(
+            proto_file,
+            proto_options,
+            suppress_legacy_namespace=args.no_legacy_namespace)
         for output_file in output_files:
             fd = res.file.add()
             fd.name = output_file.name()

pw_protobuf/py/pw_protobuf/proto_tree.py

@@ -16,6 +16,7 @@
 import abc
 import collections
 import enum
+import itertools
 
 from typing import Callable, Dict, Iterator, List, Optional, Tuple, TypeVar
 from typing import cast
@@ -27,6 +28,18 @@
 
 T = TypeVar('T')  # pylint: disable=invalid-name
 
+# Currently, protoc does not do a traversal to look up the package name of all
+# messages that are referenced in the file. For such "external" message names,
+# we are unable to find where the "::pwpb" subnamespace would be inserted by our
+# codegen. This namespace provides us with an alternative, more verbose
+# namespace that the codegen can use as a fallback in these cases. For example,
+# for the symbol name `my.external.package.ProtoMsg.SubMsg`, we would use
+# `::pw::pwpb_codegen_detail::my::external::package:ProtoMsg::SubMsg` to refer
+# to the pw_protobuf generated code, when package name info is not available.
+#
+# TODO(b/258832150) Explore removing this if possible
+EXTERNAL_SYMBOL_WORKAROUND_NAMESPACE = 'pw::pwpb_codegen_detail'
+
 
 class ProtoNode(abc.ABC):
     """A ProtoNode represents a C++ scope mapping of an entity in a .proto file.
@@ -61,6 +74,9 @@ def type(self) -> 'ProtoNode.Type':
     def children(self) -> List['ProtoNode']:
         return list(self._children.values())
 
+    def parent(self) -> Optional['ProtoNode']:
+        return self._parent
+
     def name(self) -> str:
         return self._name
 
@@ -69,10 +85,104 @@ def cpp_name(self) -> str:
         return symbol_name_mapping.fix_cc_identifier(self._name).replace(
             '.', '::')
 
-    def cpp_namespace(self, root: Optional['ProtoNode'] = None) -> str:
-        """C++ namespace of the node, up to the specified root."""
-        return '::'.join(name for name in self._attr_hierarchy(
-            lambda node: node.cpp_name(), root) if name)
+    def _package_or_external(self) -> 'ProtoNode':
+        """Returns this node's deepest package or external ancestor node.
+
+        This method may need to return an external node, as a fallback for
+        external names that are referenced, but not processed into a more
+        regular proto tree. This is because there is no way to find the package
+        name of a node referring to an external symbol.
+        """
+        node: Optional['ProtoNode'] = self
+        while (node and node.type() != ProtoNode.Type.PACKAGE
+               and node.type() != ProtoNode.Type.EXTERNAL):
+            node = node.parent()
+
+        assert node, 'proto tree was built without a root'
+        return node
+
+    def cpp_namespace(self,
+                      root: Optional['ProtoNode'] = None,
+                      codegen_subnamespace: Optional[str] = 'pwpb') -> str:
+        """C++ namespace of the node, up to the specified root.
+
+        Args:
+          root: Namespace from which this ProtoNode is referred. If this
+            ProtoNode has `root` as an ancestor namespace, then the ancestor
+            namespace scopes above `root` are omitted.
+
+          codegen_subnamespace: A subnamespace that is appended to the package
+            declared in the .proto file. It is appended to the declared package,
+            but before any namespaces that are needed for messages etc. This
+            feature can be used to allow different codegen tools to output
+            different, non-conflicting symbols for the same protos.
+
+            By default, this is "pwpb", which reflects the default behaviour
+            of the pwpb codegen.
+        """
+        self_pkg_or_ext = self._package_or_external()
+        root_pkg_or_ext = (
+            root._package_or_external()  # pylint: disable=protected-access
+            if root is not None else None)
+        if root_pkg_or_ext:
+            assert root_pkg_or_ext.type() != ProtoNode.Type.EXTERNAL
+
+        def compute_hierarchy() -> Iterator[str]:
+            same_package = True
+
+            if self_pkg_or_ext.type() == ProtoNode.Type.EXTERNAL:
+                # Can't figure out where the namespace cutoff is. Punt to using
+                # the external symbol workaround.
+                #
+                # TODO(b/250945489) Investigate removing this limitation / hack
+                return itertools.chain([EXTERNAL_SYMBOL_WORKAROUND_NAMESPACE],
+                                       self._attr_hierarchy(ProtoNode.cpp_name,
+                                                            root=None))
+
+            if root is None or root_pkg_or_ext is None:  # extra check for mypy
+                # TODO(b/250945489): maybe elide "::{codegen_subnamespace}"
+                # here, if this node doesn't have any package?
+                same_package = False
+            else:
+                paired_hierarchy = itertools.zip_longest(
+                    self_pkg_or_ext._attr_hierarchy(  # pylint: disable=protected-access
+                        ProtoNode.cpp_name,
+                        root=None),
+                    root_pkg_or_ext._attr_hierarchy(  # pylint: disable=protected-access
+                        ProtoNode.cpp_name,
+                        root=None))
+                for str_a, str_b in paired_hierarchy:
+                    if str_a != str_b:
+                        same_package = False
+                        break
+
+            if same_package:
+                # This ProtoNode and the requested root are in the same package,
+                # so the `codegen_subnamespace` should be omitted.
+                hierarchy = self._attr_hierarchy(ProtoNode.cpp_name, root)
+                return hierarchy
+
+            # The given root is either effectively nonexistent (common ancestor
+            # is ""), or is only a partial match for the package of this node.
+            # Either way, we will have to insert `codegen_subnamespace` after
+            # the relevant package string.
+            package_hierarchy = (
+                self_pkg_or_ext._attr_hierarchy(  # pylint: disable=protected-access
+                    ProtoNode.cpp_name, root))
+            maybe_subnamespace = ([codegen_subnamespace]
+                                  if codegen_subnamespace else [])
+            inside_hierarchy = self._attr_hierarchy(ProtoNode.cpp_name,
+                                                    self_pkg_or_ext)
+
+            hierarchy = itertools.chain(package_hierarchy, maybe_subnamespace,
+                                        inside_hierarchy)
+            return hierarchy
+
+        joined_namespace = '::'.join(name for name in compute_hierarchy()
+                                     if name)
+
+        return ('' if joined_namespace == codegen_subnamespace else
+                joined_namespace)
 
     def proto_path(self) -> str:
         """Fully-qualified package path of the node."""
@@ -178,9 +288,6 @@ def find(self, path: str) -> Optional['ProtoNode']:
 
         return node
 
-    def parent(self) -> Optional['ProtoNode']:
-        return self._parent
-
     def __iter__(self) -> Iterator['ProtoNode']:
         """Iterates depth-first through all nodes in this node's subtree."""
         yield self

pw_rpc/py/pw_rpc/codegen.py

@@ -154,8 +154,8 @@ def generate_package(file_descriptor_proto, proto_package: ProtoNode,
 
     gen.line()
 
-    if proto_package.cpp_namespace():
-        file_namespace = proto_package.cpp_namespace()
+    if proto_package.cpp_namespace(codegen_subnamespace=None):
+        file_namespace = proto_package.cpp_namespace(codegen_subnamespace=None)
         if file_namespace.startswith('::'):
             file_namespace = file_namespace[2:]
 
@@ -458,8 +458,8 @@ def _select_stub_methods(gen: StubGenerator, method: ProtoServiceMethod):
 def package_stubs(proto_package: ProtoNode, gen: CodeGenerator,
                   stub_generator: StubGenerator) -> None:
     """Generates the RPC stubs for a package."""
-    if proto_package.cpp_namespace():
-        file_ns = proto_package.cpp_namespace()
+    if proto_package.cpp_namespace(codegen_subnamespace=None):
+        file_ns = proto_package.cpp_namespace(codegen_subnamespace=None)
         if file_ns.startswith('::'):
             file_ns = file_ns[2:]