Re: [PATCH 14/19] scripts/qapi: generate high-level Rust bindings

Paolo Bonzini <[email protected]> writes:

> From: Marc-André Lureau <[email protected]>
>
> Generate high-level native Rust declarations for the QAPI types.
>
> - char* is mapped to String, scalars to there corresponding Rust types
>
> - enums are simply aliased from FFI
>
> - has_foo/foo members are mapped to Option<T>
>
> - lists are represented as Vec<T>
>
> - structures have Rust versions, with To/From FFI conversions
>
> - alternate are represented as Rust enum
>
> - unions are represented in a similar way as in C: a struct S with a "u"
>   member (since S may have extra 'base' fields). However, the discriminant
>   isn't a member of S, since Rust enum already include it.
>
> Signed-off-by: Marc-André Lureau <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
>  meson.build              |   4 +-
>  scripts/qapi/backend.py  |  27 ++-
>  scripts/qapi/main.py     |   4 +-
>  scripts/qapi/rs.py       | 181 +++++++++++++++++++
>  scripts/qapi/rs_types.py | 365 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 577 insertions(+), 4 deletions(-)
>  create mode 100644 scripts/qapi/rs.py
>  create mode 100644 scripts/qapi/rs_types.py
>
> diff --git a/meson.build b/meson.build
> index afaefa01722..ce914217c52 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3571,12 +3571,14 @@ qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.py',
>                       meson.current_source_dir() / 'scripts/qapi/introspect.py',
>                       meson.current_source_dir() / 'scripts/qapi/main.py',
>                       meson.current_source_dir() / 'scripts/qapi/parser.py',
> +                     meson.current_source_dir() / 'scripts/qapi/rs_types.py',
>                       meson.current_source_dir() / 'scripts/qapi/schema.py',
>                       meson.current_source_dir() / 'scripts/qapi/source.py',
>                       meson.current_source_dir() / 'scripts/qapi/types.py',
>                       meson.current_source_dir() / 'scripts/qapi/features.py',
>                       meson.current_source_dir() / 'scripts/qapi/visit.py',
> -                     meson.current_source_dir() / 'scripts/qapi-gen.py'
> +                     meson.current_source_dir() / 'scripts/qapi-gen.py',
> +                     meson.current_source_dir() / 'scripts/qapi/rs.py',
>  ]
>  
>  tracetool = [
> diff --git a/scripts/qapi/backend.py b/scripts/qapi/backend.py
> index 49ae6ecdd33..305b62b514c 100644
> --- a/scripts/qapi/backend.py
> +++ b/scripts/qapi/backend.py
> @@ -7,6 +7,7 @@
>  from .events import gen_events
>  from .features import gen_features
>  from .introspect import gen_introspect
> +from .rs_types import gen_rs_types
>  from .schema import QAPISchema
>  from .types import gen_types
>  from .visit import gen_visit
> @@ -36,7 +37,7 @@ def generate(self,
>          """
>  
>  
> -class QAPICBackend(QAPIBackend):
> +class QAPICodeBackend(QAPIBackend):

Why this rename?

If we want it, separate commit, please.

>      # pylint: disable=too-few-public-methods
>  
>      def generate(self,
> @@ -63,3 +64,27 @@ def generate(self,
>          gen_commands(schema, output_dir, prefix, gen_tracing)
>          gen_events(schema, output_dir, prefix)
>          gen_introspect(schema, output_dir, prefix, unmask)
> +
> +
> +class QAPIRsBackend(QAPIBackend):
> +    # pylint: disable=too-few-public-methods
> +
> +    def generate(self,
> +                 schema: QAPISchema,
> +                 output_dir: str,
> +                 prefix: str,
> +                 unmask: bool,
> +                 builtins: bool,
> +                 gen_tracing: bool) -> None:
> +        """
> +        Generate Rust code for the given schema into the target directory.
> +
> +        :param schema_file: The primary QAPI schema file.
> +        :param output_dir: The output directory to store generated code.
> +        :param prefix: Optional C-code prefix for symbol names.
> +        :param unmask: Expose non-ABI names through introspection?
> +        :param builtins: Generate code for built-in types?
> +
> +        :raise QAPIError: On failures.
> +        """
> +        gen_rs_types(schema, output_dir, prefix, builtins)

As discussed in reply to the cover letter, this series uses the -B
plumbing for out-of-tree backends for generating Rust.  Fine for a
prototype.  This class is the glue between -B and Rust generation.

> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 0e2a6ae3f07..4ad75e213f5 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -12,7 +12,7 @@
>  import sys
>  from typing import Optional
>  
> -from .backend import QAPIBackend, QAPICBackend
> +from .backend import QAPIBackend, QAPICodeBackend
>  from .common import must_match
>  from .error import QAPIError
>  from .schema import QAPISchema
> @@ -27,7 +27,7 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
>  
>  def create_backend(path: str) -> QAPIBackend:
>      if path is None:
> -        return QAPICBackend()
> +        return QAPICodeBackend()
>  
>      module_path, dot, class_name = path.rpartition('.')
>      if not dot:
> diff --git a/scripts/qapi/rs.py b/scripts/qapi/rs.py
> new file mode 100644
> index 00000000000..2a9bbcb9f54
> --- /dev/null
> +++ b/scripts/qapi/rs.py
> @@ -0,0 +1,181 @@
> +# This work is licensed under the terms of the GNU GPL, version 2.
> +# See the COPYING file in the top-level directory.
> +"""
> +QAPI Rust generator
> +"""
> +
> +import os
> +import re
> +import subprocess
> +from typing import NamedTuple, Optional
> +
> +from .common import POINTER_SUFFIX
> +from .gen import QAPIGen
> +from .schema import QAPISchemaModule, QAPISchemaVisitor
> +
> +
> +# see to_upper_case()/to_lower_case() below
> +snake_case = re.compile(r'((?<=[a-z0-9])[A-Z]|(?!^)[A-Z](?=[a-z]))')
> +
> +
> +rs_name_trans = str.maketrans('.-', '__')
> +
> +
> +# Map @name to a valid Rust identifier.
> +# If @protect, avoid returning certain ticklish identifiers (like
> +# keywords) by prepending raw identifier prefix 'r#'.
> +def rs_name(name: str, protect: bool = True) -> str:
> +    name = name.translate(rs_name_trans)
> +    if name[0].isnumeric():
> +        name = '_' + name
> +    if not protect:
> +        return name
> +    # based from the list:
> +    # https://doc.rust-lang.org/reference/keywords.html
> +    if name in ('Self', 'abstract', 'as', 'async',
> +                'await', 'become', 'box', 'break',
> +                'const', 'continue', 'crate', 'do',
> +                'dyn', 'else', 'enum', 'extern',
> +                'false', 'final', 'fn', 'for',
> +                'if', 'impl', 'in', 'let',
> +                'loop', 'macro', 'match', 'mod',
> +                'move', 'mut', 'override', 'priv',
> +                'pub', 'ref', 'return', 'self',
> +                'static', 'struct', 'super', 'trait',
> +                'true', 'try', 'type', 'typeof',
> +                'union', 'unsafe', 'unsized', 'use',
> +                'virtual', 'where', 'while', 'yield'):
> +        name = 'r#' + name
> +    # avoid some clashes with the standard library
> +    if name in ('String',):
> +        name = 'Qapi' + name
> +
> +    return name

This is like common.c_name().  Differences:

1. Funny input characters

   c_name() returns a valid C identifier for any non-empty input.

   rs_name() requires its argument to contain only characters valid in
   Rust identifiers plus '.' and '-'.

   I think we better avoid this difference.

2. "Protected" identifiers

   When @protect, then certain "protected" identifiers are prefixed with
   'q_'.  We typically pass False to @protect when the output is used as
   part of an identifier.

   c_name() and rs_name() protect different identifiers.  Makes sense.

3. Input starting with a digit

   c_name() treats them just like protected identifiers, i.e. prefix
   with 'q_' when @protect.

   rs_name() prefixes with '_'.  Is this a good idea?  Hmm...  "The Rust
   Reference:

       Note

       Identifiers starting with an underscore are typically used to
       indicate an identifier that is intentionally unused, and will
       silence the unused warning in rustc.

       https://doc.rust-lang.org/reference/identifiers.html

   rs_name() prefixes always, not just when @protect.  Is this a good
   idea?  Remember, @protect is typically false when the output is used
   as part of an identifier.  Or do we use it differently for Rust?

4. Name clash avoidance

   c_name() treats names that are prone to clash as protected,
   i.e. prefix 'q_' unless @protect.

   rs_name() prefixes 'Qapi' instead.  Why?

> +
> +
> +def rs_type(c_type: str,
> +            qapi_ns: str = 'qapi::',
> +            optional: bool = False,
> +            box: bool = False) -> str:
> +    (is_pointer, _, is_list, c_type) = rs_ctype_parse(c_type)
> +    to_rs = {
> +        'QNull': '()',
> +        'QObject': 'QObject',
> +        'any': 'QObject',
> +        'bool': 'bool',
> +        'char': 'i8',
> +        'double': 'f64',
> +        'int': 'i64',
> +        'int16': 'i16',
> +        'int16_t': 'i16',
> +        'int32': 'i32',
> +        'int32_t': 'i32',
> +        'int64': 'i64',
> +        'int64_t': 'i64',
> +        'int8': 'i8',
> +        'int8_t': 'i8',
> +        'number': 'f64',
> +        'size': 'u64',
> +        'str': 'String',
> +        'uint16': 'u16',
> +        'uint16_t': 'u16',
> +        'uint32': 'u32',
> +        'uint32_t': 'u32',
> +        'uint64': 'u64',
> +        'uint64_t': 'u64',
> +        'uint8': 'u8',
> +        'uint8_t': 'u8',
> +        'String': 'QapiString',
> +    }

The argument name @c_type suggests it is a C type, but this map contains
a mix of C types, QAPI built-in types, and even a user-defined QAPI type
(String).  How come?

Why do we even have to map from C type to Rust type?  Why can't we map
from QAPI type to Rust type, like we map from QAPI type to C type?

> +    if is_pointer:
> +        to_rs.update({
> +            'char': 'String',
> +        })
> +
> +    if is_list:
> +        c_type = c_type[:-4]
> +
> +    ret = to_rs.get(c_type, qapi_ns + c_type)
> +    if is_list:
> +        ret = 'Vec<%s>' % ret
> +    elif is_pointer and c_type not in to_rs and box:
> +        ret = 'Box<%s>' % ret
> +    if optional:
> +        ret = 'Option<%s>' % ret
> +    return ret
> +
> +
> +class CType(NamedTuple):
> +    is_pointer: bool
> +    is_const: bool
> +    is_list: bool
> +    c_type: str
> +
> +
> +def rs_ctype_parse(c_type: str) -> CType:
> +    is_pointer = False
> +    if c_type.endswith(POINTER_SUFFIX):
> +        is_pointer = True
> +        c_type = c_type[:-len(POINTER_SUFFIX)]
> +    is_list = c_type.endswith('List')
> +    is_const = False
> +    if c_type.startswith('const '):
> +        is_const = True
> +        c_type = c_type[6:]
> +
> +    c_type = rs_name(c_type)
> +    return CType(is_pointer, is_const, is_list, c_type)

This feels a bit brittle.

> +
> +
> +def to_camel_case(value: str) -> str:
> +    # special case for last enum value
> +    if value == '_MAX':
> +        return value
> +    raw_id = False
> +    if value.startswith('r#'):
> +        raw_id = True
> +        value = value[2:]
> +    value = ''.join('_' + word if word[0].isdigit()
> +                    else word[:1].upper() + word[1:]
> +                    for word in filter(None, re.split("[-_]+", value)))
> +    if raw_id:
> +        return 'r#' + value
> +    return value
> +
> +
> +def to_upper_case(value: str) -> str:
> +    return snake_case.sub(r'_\1', value).upper()

This tackles the same problem as common.camel_to_upper().  Your code is
much simpler.  However, the two produce different output, e.g.

    input                       output
    QType                       QTYPE
                                Q_TYPE
    XDbgBlockGraphNodeType      XDBG_BLOCK_GRAPH_NODE_TYPE
                                X_DBG_BLOCK_GRAPH_NODE_TYPE
    QCryptoTLSCredsEndpoint     QCRYPTO_TLS_CREDS_ENDPOINT
                                Q_CRYPTO_TLS_CREDS_ENDPOINT

I doubt having two different mappings from CamelCase make sense.

See also commit 7b29353fdd9 (qapi: Smarter camel_to_upper() to reduce
need for 'prefix').

Aside: the examples in camel_to_upper()'s function comment are out of
date.  I'll take care of that.

> +
> +
> +def to_lower_case(value: str) -> str:
> +    return snake_case.sub(r'_\1', value).lower()
> +
> +
> +class QAPIGenRs(QAPIGen):
> +    pass

In my initial review of the generated code, I suggested a file comment.
Code for that would go here.  See QAPIGenC for an example.

> +
> +
> +class QAPISchemaRsVisitor(QAPISchemaVisitor):
> +
> +    def __init__(self, prefix: str, what: str):
> +        super().__init__()
> +        self._prefix = prefix
> +        self._what = what
> +        self._gen = QAPIGenRs(self._prefix + self._what + '.rs')
> +        self._main_module: Optional[str] = None
> +
> +    def visit_module(self, name: Optional[str]) -> None:
> +        if name is None:
> +            return
> +        if QAPISchemaModule.is_user_module(name):
> +            if self._main_module is None:
> +                self._main_module = name

._main_module appears to be unused.

> +
> +    def write(self, output_dir: str) -> None:
> +        self._gen.write(output_dir)
> +
> +        pathname = os.path.join(output_dir, self._gen.fname)

This duplicates ._gen.write()'s file name construction.  I think we
better make it a available from ._gen.

> +        try:
> +            subprocess.check_call(['rustfmt', pathname])

Interesting.  Worth mentioning in the commit message.

> +        except FileNotFoundError:
> +            pass

Huh?

Gotta run, rest left for later.

[...]