From db843159eadf5f638b92d609dbc687046875328c Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 3 May 2023 13:41:18 +0200 Subject: [PATCH] msggen: Move overrides into the model itself We were using per-type overrides which caused some asymmetries, where conversions could end up dropping fields as we went along. Essentially each conversion would need to override a superset of the previous one, which then caused issues when attempting to close the loop. By overriding on the model level we ensure that all representations are equivalent and convertible into one another, at the expense of overriding a bit more aggressively, which should be fine anyway. --- contrib/msggen/README.md | 19 ++++++++++ contrib/msggen/msggen/__main__.py | 6 +++- contrib/msggen/msggen/gen/grpc.py | 58 ++++++++----------------------- contrib/msggen/msggen/gen/rust.py | 33 +++++------------- contrib/msggen/msggen/model.py | 39 +++++++++++++++++++++ contrib/msggen/msggen/patch.py | 50 ++++++++++++++++++++++++++ 6 files changed, 136 insertions(+), 69 deletions(-) diff --git a/contrib/msggen/README.md b/contrib/msggen/README.md index 2c90f9f94..ea71e7a9c 100644 --- a/contrib/msggen/README.md +++ b/contrib/msggen/README.md @@ -21,3 +21,22 @@ digraph { "Rust JSON-RPC structs" -> "cln-rpc"; } ``` + +`msggen` will load the schemas in `doc/schemas` into memory, and then +use `Patches` to enrich the model before using it to generate the +bindings for the various languages as well as the converters from one +format to another. These patches can be found in `msggen/patch.py` and +perform a variety of operations: + + - Annotate the model with additional data from external sources, such + as the `.msggen.json` file at the repository root to track details + that can be derived but should remain constant (grpc field + numbering and versioning information) + - Aggregate common types with type overrides and omit fields that we + can't map currently. + - Infer optionality based on the versions a field was added or + deprecated, and the currently supported range of versions. + +If there is a field that is currently missing in the model, that is in +the schemas it is most likely because it has been marked as omitted in +the patch. diff --git a/contrib/msggen/msggen/__main__.py b/contrib/msggen/msggen/__main__.py index e8a5ee49a..20fcbc6f6 100644 --- a/contrib/msggen/msggen/__main__.py +++ b/contrib/msggen/msggen/__main__.py @@ -8,7 +8,7 @@ from msggen.gen.rust import RustGenerator from msggen.gen.generator import GeneratorChain from msggen.utils import load_jsonrpc_service import logging -from msggen.patch import VersionAnnotationPatch, OptionalPatch +from msggen.patch import VersionAnnotationPatch, OptionalPatch, OverridePatch from msggen.checks import VersioningCheck @@ -73,6 +73,10 @@ def run(rootdir: Path): p.apply(service) OptionalPatch().apply(service) + # Mark some fields we can't map as omitted, and for complex types + # we manually mapped, use those types instead. + OverridePatch().apply(service) + # Run the checks here, we should eventually split that out to a # separate subcommand VersioningCheck().check(service) diff --git a/contrib/msggen/msggen/gen/grpc.py b/contrib/msggen/msggen/gen/grpc.py index c479ed39c..2219097d1 100644 --- a/contrib/msggen/msggen/gen/grpc.py +++ b/contrib/msggen/msggen/gen/grpc.py @@ -33,30 +33,9 @@ typemap = { } -# Manual overrides for some of the auto-generated types for paths -overrides = { - # Truncate the tree here, it's a complex structure with identitcal - # types - 'ListPeers.peers[].channels[].state_changes[]': None, - 'ListPeers.peers[].channels[].htlcs[].state': None, - 'ListPeers.peers[].channels[].opener': "ChannelSide", - 'ListPeers.peers[].channels[].closer': "ChannelSide", - 'ListPeers.peers[].channels[].features[]': "string", - 'ListPeerChannels.channels[].state_changes[]': None, - 'ListPeerChannels.channels[].htlcs[].state': None, - 'ListPeerChannels.channels[].opener': "ChannelSide", - 'ListPeerChannels.channels[].closer': "ChannelSide", - 'ListPeerChannels.channels[].features[]': None, - 'ListPeerChannels.channels[].channel_type': None, - 'ListFunds.channels[].state': 'ChannelState', - 'ListTransactions.transactions[].type[]': None, - - 'ListClosedChannels.closedchannels[].closer': "ChannelSide", - 'ListClosedChannels.closedchannels[].opener': "ChannelSide", - 'ListClosedChannels.closedchannels[].channel_type': None, -} - - +# GRPC builds a stub with the methods declared in the protobuf file, +# but it also comes with its own methods, e.g., `connect` which can +# clash with the generated ones. So rename the ones we know clash. method_name_overrides = { "Connect": "ConnectPeer", } @@ -188,7 +167,7 @@ class GrpcGenerator(IGenerator): self.write(f"""{prefix}}}\n""", False) def generate_message(self, message: CompositeField): - if overrides.get(message.path, "") is None: + if message.omit(): return self.write(f""" @@ -197,33 +176,26 @@ class GrpcGenerator(IGenerator): # Declare enums inline so they are scoped correctly in C++ for _, f in enumerate(message.fields): - if isinstance(f, EnumField) and f.path not in overrides.keys(): + if isinstance(f, EnumField) and not f.override(): self.generate_enum(f, indent=1) for i, f in self.enumerate_fields(message.typename, message.fields): - if overrides.get(f.path, "") is None: + if f.omit(): continue opt = "optional " if f.optional else "" + if isinstance(f, ArrayField): - typename = typemap.get(f.itemtype.typename, f.itemtype.typename) - if f.path in overrides: - typename = overrides[f.path] + typename = f.override(typemap.get(f.itemtype.typename, f.itemtype.typename)) self.write(f"\trepeated {typename} {f.normalized()} = {i};\n", False) elif isinstance(f, PrimitiveField): - typename = typemap.get(f.typename, f.typename) - if f.path in overrides: - typename = overrides[f.path] + typename = f.override(typemap.get(f.typename, f.typename)) self.write(f"\t{opt}{typename} {f.normalized()} = {i};\n", False) elif isinstance(f, EnumField): - typename = f.typename - if f.path in overrides: - typename = overrides[f.path] + typename = f.override(f.typename) self.write(f"\t{opt}{typename} {f.normalized()} = {i};\n", False) elif isinstance(f, CompositeField): - typename = f.typename - if f.path in overrides: - typename = overrides[f.path] + typename = f.override(f.typename) self.write(f"\t{opt}{typename} {f.normalized()} = {i};\n", False) self.write(f"""}} @@ -263,7 +235,7 @@ class GrpcConverterGenerator(IGenerator): def generate_composite(self, prefix, field: CompositeField): """Generates the conversions from JSON-RPC to GRPC. """ - if overrides.get(field.path, "") is None: + if field.omit(): return # First pass: generate any sub-fields before we generate the @@ -284,7 +256,7 @@ class GrpcConverterGenerator(IGenerator): """) for f in field.fields: - if overrides.get(f.path, "") is None: + if f.omit(): continue name = f.normalized() @@ -423,7 +395,7 @@ class GrpcUnconverterGenerator(GrpcConverterGenerator): def generate_composite(self, prefix, field: CompositeField) -> None: # First pass: generate any sub-fields before we generate the # top-level field itself. - if overrides.get(field.path, "") is None: + if field.omit(): return for f in field.fields: @@ -443,7 +415,7 @@ class GrpcUnconverterGenerator(GrpcConverterGenerator): for f in field.fields: name = f.normalized() - if overrides.get(f.path, "") is None: + if f.omit(): continue if isinstance(f, ArrayField): diff --git a/contrib/msggen/msggen/gen/rust.py b/contrib/msggen/msggen/gen/rust.py index 26a5563e4..3f8ead1d1 100644 --- a/contrib/msggen/msggen/gen/rust.py +++ b/contrib/msggen/msggen/gen/rust.py @@ -15,25 +15,6 @@ logger = logging.getLogger(__name__) # built-in keywords. keywords = ["in", "type"] -# Manual overrides for some of the auto-generated types for paths -# Manual overrides for some of the auto-generated types for paths -overrides = { - 'ListPeers.peers[].channels[].state_changes[].old_state': "ChannelState", - 'ListPeers.peers[].channels[].state_changes[].new_state': "ChannelState", - 'ListPeers.peers[].channels[].state_changes[].cause': "ChannelStateChangeCause", - 'ListPeers.peers[].channels[].htlcs[].state': None, - 'ListPeers.peers[].channels[].opener': "ChannelSide", - 'ListPeers.peers[].channels[].closer': "ChannelSide", - 'ListPeers.peers[].channels[].features[]': "string", - 'ListFunds.channels[].state': 'ChannelState', - 'ListTransactions.transactions[].type[]': None, - 'Invoice.exposeprivatechannels': None, - - 'ListClosedChannels.closedchannels[].closer': "ChannelSide", - 'ListClosedChannels.closedchannels[].opener': "ChannelSide", - 'ListClosedChannels.closedchannels[].channel_type': None, -} - # A map of schema type to rust primitive types. typemap = { 'boolean': 'bool', @@ -44,7 +25,7 @@ typemap = { 'number': 'f64', 'pubkey': 'PublicKey', 'short_channel_id': 'ShortChannelId', - 'signature': 'String', + 'signature': 'Vec', 'string': 'String', 'txid': 'String', 'float': 'f32', @@ -79,6 +60,8 @@ def normalize_varname(field): def gen_field(field): + if field.omit(): + return ("", "") if isinstance(field, CompositeField): return gen_composite(field) elif isinstance(field, EnumField): @@ -94,7 +77,7 @@ def gen_field(field): def gen_enum(e): defi, decl = "", "" - if e.path in overrides and overrides[e.path] is None: + if e.omit(): return "", "" if e.description != "": @@ -132,9 +115,9 @@ def gen_enum(e): typename = e.typename - if e.path in overrides: + if e.override() is not None: decl = "" # No declaration if we have an override - typename = overrides[e.path] + typename = e.override() if not e.optional: defi = f" // Path `{e.path}`\n" @@ -176,9 +159,9 @@ def gen_array(a): logger.debug(f"Generating array field {a.name} -> {name} ({a.path})") _, decl = gen_field(a.itemtype) - if a.path in overrides: + if a.override(): decl = "" # No declaration if we have an override - itemtype = overrides[a.path] + itemtype = a.override() elif isinstance(a.itemtype, PrimitiveField): itemtype = a.itemtype.typename elif isinstance(a.itemtype, CompositeField): diff --git a/contrib/msggen/msggen/model.py b/contrib/msggen/msggen/model.py index d35a58f70..f1a781360 100644 --- a/contrib/msggen/msggen/model.py +++ b/contrib/msggen/msggen/model.py @@ -40,6 +40,14 @@ class Field: self.deprecated = deprecated self.required = False + # Are we going to omit this field when generating bindings? + # This usually means that the field either doesn't make sense + # to convert or that msggen cannot handle converting this + # field and its children yet. + self.omitted = False + + self.type_override: Optional[str] = None + @property def name(self): return FieldName(self.path.split(".")[-1]) @@ -53,6 +61,37 @@ class Field: def normalized(self): return self.name.normalized() + def capitalized(self): + return self.name.capitalized() + + def omit(self): + """Returns true if we should not consider this field in our model. + + This can be either because the field is redundant, or because + msggen cannot currently handle it. The field (and it's type if + it's composite) will not be materialized in the generated + bindings and converters. + + It is mainly switched on and off in the OverridePatch which is + the central location where we manage overrides and omissions. + + """ + return self.omitted + + def override(self, default: Optional[str] = None) -> Optional[str]: + """Provide a type that should be used instead of the inferred one. + + This is useful if for shared types that we don't want to + generate multiple times, and for enums that can result in + naming clashes in the grpc model (enum variantss must be + uniquely name in the top-level scope...). + + It is mainly switched on and off in the OverridePatch which is + the central location where we manage overrides and omissions. + + """ + return self.type_override if self.type_override else default + class Service: """Top level class that wraps all the RPC methods. diff --git a/contrib/msggen/msggen/patch.py b/contrib/msggen/msggen/patch.py index 293c88ff7..fea5a1260 100644 --- a/contrib/msggen/msggen/patch.py +++ b/contrib/msggen/msggen/patch.py @@ -133,3 +133,53 @@ class OptionalPatch(Patch): if f.deprecated and self.versions.index(f.deprecated) < idx[1]: f.optional = True + + +class OverridePatch(Patch): + """Allows omitting some fields and overriding the type of fields based on configuration. + + """ + omit = [ + 'Decode.invoice_paths[]', + 'Decode.invoice_paths[].payinfo', + 'Decode.offer_paths[].path[]', + 'Decode.offer_recurrence', + 'Decode.routes[][]', + 'Decode.unknown_invoice_request_tlvs[]', + 'Decode.unknown_invoice_tlvs[]', + 'Decode.unknown_offer_tlvs[]', + 'DecodePay.routes[][]', + 'DecodeRoutes.routes', + 'Invoice.exposeprivatechannels', + 'ListClosedChannels.closedchannels[].channel_type', + 'ListPeerChannels.channels[].channel_type', + 'ListPeerChannels.channels[].features[]', + 'ListPeerChannels.channels[].htlcs[].state', + 'ListPeerChannels.channels[].state_changes[]', + 'ListPeers.peers[].channels[].htlcs[].state', + 'ListPeers.peers[].channels[].state_changes[]', + 'ListTransactions.transactions[].type[]', + ] + + # Handcoded types to use instead of generating the types from the + # schema. Useful for repeated types, and types that have + # redundancies. + overrides = { + 'ListClosedChannels.closedchannels[].closer': "ChannelSide", + 'ListClosedChannels.closedchannels[].opener': "ChannelSide", + 'ListFunds.channels[].state': 'ChannelState', + 'ListPeerChannels.channels[].closer': "ChannelSide", + 'ListPeerChannels.channels[].opener': "ChannelSide", + 'ListPeers.peers[].channels[].closer': "ChannelSide", + 'ListPeers.peers[].channels[].features[]': "string", + 'ListPeers.peers[].channels[].opener': "ChannelSide", + 'ListPeers.peers[].channels[].state_changes[].cause': "ChannelStateChangeCause", + 'ListPeers.peers[].channels[].state_changes[].old_state': "ChannelState", + 'ListPeers.peers[].channels[].state_changes[].old_state': "ChannelState", + } + + def visit(self, f: model.Field) -> None: + """For now just skips the fields we can't convert. + """ + f.omitted = f.path in self.omit + f.type_override = self.overrides.get(f.path, None)