Re: [PATCH 13/19] qapi/schema: fix typing for QAPISchemaVariants.tag_member

[John Snow]

On Wed, Jan 17, 2024 at 5:53 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Still more...
>

Would you hate me if I suggested that we punt this to after type
checking is applied? i.e. let's do the stupid @property thing for now,
and we'll rebase this proposal and put it in right afterwards.

(Admittedly, it's just easier for me to review the impact on the
typing work if I already have that baseline to work from...)

> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 658c288f8f..4a2e62d919 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -328,7 +328,8 @@ def visit_object_type(self, name, info, ifcond, features,
>                        + self._nodes_for_sections(doc)
>                        + self._nodes_for_if_section(ifcond))
>
> -    def visit_alternate_type(self, name, info, ifcond, features, variants):
> +    def visit_alternate_type(self, name, info, ifcond, features,
> +                             alternatives):
>          doc = self._cur_doc
>          self._add_doc('Alternate',
>                        self._nodes_for_members(doc, 'Members')
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index c38df61a6d..e5cea8004e 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -26,6 +26,7 @@
>  from .gen import QAPISchemaMonolithicCVisitor
>  from .schema import (
>      QAPISchema,
> +    QAPISchemaAlternatives,
>      QAPISchemaArrayType,
>      QAPISchemaBuiltinType,
>      QAPISchemaEntity,
> @@ -343,12 +344,12 @@ def visit_object_type_flat(self, name: str, info: 
> Optional[QAPISourceInfo],
>      def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>                               ifcond: QAPISchemaIfCond,
>                               features: List[QAPISchemaFeature],
> -                             variants: QAPISchemaVariants) -> None:
> +                             alternatives: QAPISchemaAlternatives) -> None:
>          self._gen_tree(
>              name, 'alternate',
>              {'members': [Annotated({'type': self._use_type(m.type)},
>                                     m.ifcond)
> -                         for m in variants.variants]},
> +                         for m in alternatives.variants]},
>              ifcond, features
>          )
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 0d9a70ab4c..f18aac7199 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -187,7 +187,8 @@ def visit_object_type_flat(self, name, info, ifcond, 
> features,
>                                 members, variants):
>          pass
>
> -    def visit_alternate_type(self, name, info, ifcond, features, variants):
> +    def visit_alternate_type(self, name, info, ifcond, features,
> +                             alternatives):
>          pass
>
>      def visit_command(self, name, info, ifcond, features,
> @@ -563,8 +564,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>
>      def __init__(self, name, info, doc, ifcond, features, variants):
>          super().__init__(name, info, doc, ifcond, features)
> -        assert isinstance(variants, QAPISchemaVariants)
> -        assert variants.tag_member
> +        assert isinstance(variants, QAPISchemaAlternatives)
>          variants.set_defined_in(name)
>          variants.tag_member.set_defined_in(self.name)
>          self.variants = variants
> @@ -625,19 +625,12 @@ def visit(self, visitor):
>              self.name, self.info, self.ifcond, self.features, self.variants)
>
>
> -class QAPISchemaVariants:
> -    def __init__(self, tag_name, info, tag_member, variants):
> -        # Unions pass tag_name but not tag_member.
> -        # Alternates pass tag_member but not tag_name.
> -        # After check(), tag_member is always set.
> -        assert bool(tag_member) != bool(tag_name)
> -        assert (isinstance(tag_name, str) or
> -                isinstance(tag_member, QAPISchemaObjectTypeMember))
> +class QAPISchemaVariantsBase:
> +    def __init__(self, info, variants):
>          for v in variants:
>              assert isinstance(v, QAPISchemaVariant)
> -        self._tag_name = tag_name
>          self.info = info
> -        self.tag_member = tag_member
> +        self.tag_member = None
>          self.variants = variants
>
>      def set_defined_in(self, name):
> @@ -645,66 +638,68 @@ def set_defined_in(self, name):
>              v.set_defined_in(name)
>
>      def check(self, schema, seen):
> -        if self._tag_name:      # union
> -            self.tag_member = seen.get(c_name(self._tag_name))
> -            base = "'base'"
> -            # Pointing to the base type when not implicit would be
> -            # nice, but we don't know it here
> -            if not self.tag_member or self._tag_name != self.tag_member.name:
> -                raise QAPISemError(
> -                    self.info,
> -                    "discriminator '%s' is not a member of %s"
> -                    % (self._tag_name, base))
> -            # Here we do:
> -            base_type = schema.resolve_type(self.tag_member.defined_in)
> -            if not base_type.is_implicit():
> -                base = "base type '%s'" % self.tag_member.defined_in
> -            if not isinstance(self.tag_member.type, QAPISchemaEnumType):
> -                raise QAPISemError(
> -                    self.info,
> -                    "discriminator member '%s' of %s must be of enum type"
> -                    % (self._tag_name, base))
> -            if self.tag_member.optional:
> -                raise QAPISemError(
> -                    self.info,
> -                    "discriminator member '%s' of %s must not be optional"
> -                    % (self._tag_name, base))
> -            if self.tag_member.ifcond.is_present():
> -                raise QAPISemError(
> -                    self.info,
> -                    "discriminator member '%s' of %s must not be conditional"
> -                    % (self._tag_name, base))
> -        else:                   # alternate
> -            assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> -            assert not self.tag_member.optional
> -            assert not self.tag_member.ifcond.is_present()
> -        if self._tag_name:      # union
> -            # branches that are not explicitly covered get an empty type
> -            cases = {v.name for v in self.variants}
> -            for m in self.tag_member.type.members:
> -                if m.name not in cases:
> -                    v = QAPISchemaVariant(m.name, self.info,
> -                                          'q_empty', m.ifcond)
> -                    v.set_defined_in(self.tag_member.defined_in)
> -                    self.variants.append(v)
> -        if not self.variants:
> -            raise QAPISemError(self.info, "union has no branches")
>          for v in self.variants:
>              v.check(schema)
> -            # Union names must match enum values; alternate names are
> -            # checked separately. Use 'seen' to tell the two apart.
> -            if seen:
> -                if v.name not in self.tag_member.type.member_names():
> -                    raise QAPISemError(
> -                        self.info,
> -                        "branch '%s' is not a value of %s"
> -                        % (v.name, self.tag_member.type.describe()))
> -                if not isinstance(v.type, QAPISchemaObjectType):
> -                    raise QAPISemError(
> -                        self.info,
> -                        "%s cannot use %s"
> -                        % (v.describe(self.info), v.type.describe()))
> -                v.type.check(schema)
> +
> +
> +class QAPISchemaVariants(QAPISchemaVariantsBase):
> +    def __init__(self, info, variants, tag_name):
> +        assert isinstance(tag_name, str)
> +        super().__init__(info, variants)
> +        self._tag_name = tag_name
> +
> +    def check(self, schema, seen):
> +        self.tag_member = seen.get(c_name(self._tag_name))
> +        base = "'base'"
> +        # Pointing to the base type when not implicit would be
> +        # nice, but we don't know it here
> +        if not self.tag_member or self._tag_name != self.tag_member.name:
> +            raise QAPISemError(
> +                self.info,
> +                "discriminator '%s' is not a member of %s"
> +                % (self._tag_name, base))
> +        # Here we do:
> +        base_type = schema.resolve_type(self.tag_member.defined_in)
> +        if not base_type.is_implicit():
> +            base = "base type '%s'" % self.tag_member.defined_in
> +        if not isinstance(self.tag_member.type, QAPISchemaEnumType):
> +            raise QAPISemError(
> +                self.info,
> +                "discriminator member '%s' of %s must be of enum type"
> +                % (self._tag_name, base))
> +        if self.tag_member.optional:
> +            raise QAPISemError(
> +                self.info,
> +                "discriminator member '%s' of %s must not be optional"
> +                % (self._tag_name, base))
> +        if self.tag_member.ifcond.is_present():
> +            raise QAPISemError(
> +                self.info,
> +                "discriminator member '%s' of %s must not be conditional"
> +                % (self._tag_name, base))
> +        # branches that are not explicitly covered get an empty type
> +        cases = {v.name for v in self.variants}
> +        for m in self.tag_member.type.members:
> +            if m.name not in cases:
> +                v = QAPISchemaVariant(m.name, self.info,
> +                                      'q_empty', m.ifcond)
> +                v.set_defined_in(self.tag_member.defined_in)
> +                self.variants.append(v)
> +        if not self.variants:
> +            raise QAPISemError(self.info, "union has no branches")
> +        super().check(schema, seen)
> +        for v in self.variants:
> +            if v.name not in self.tag_member.type.member_names():
> +                raise QAPISemError(
> +                    self.info,
> +                    "branch '%s' is not a value of %s"
> +                    % (v.name, self.tag_member.type.describe()))
> +            if not isinstance(v.type, QAPISchemaObjectType):
> +                raise QAPISemError(
> +                    self.info,
> +                    "%s cannot use %s"
> +                    % (v.describe(self.info), v.type.describe()))
> +            v.type.check(schema)
>
>      def check_clash(self, info, seen):
>          for v in self.variants:
> @@ -713,6 +708,19 @@ def check_clash(self, info, seen):
>              v.type.check_clash(info, dict(seen))
>
>
> +class QAPISchemaAlternatives(QAPISchemaVariantsBase):
> +    def __init__(self, info, variants, tag_member):
> +        assert isinstance(tag_member, QAPISchemaObjectTypeMember)
> +        super().__init__(info, variants)
> +        self.tag_member = tag_member
> +
> +    def check(self, schema, seen):
> +        super().check(schema, seen)
> +        assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> +        assert not self.tag_member.optional
> +        assert not self.tag_member.ifcond.is_present()
> +
> +
>  class QAPISchemaMember:
>      """ Represents object members, enum members and features """
>      role = 'member'
> @@ -1184,7 +1192,7 @@ def _def_union_type(self, expr: QAPIExpression):
>              QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
>                                   base, members,
>                                   QAPISchemaVariants(
> -                                     tag_name, info, None, variants)))
> +                                     info, variants, tag_name)))
>
>      def _def_alternate_type(self, expr: QAPIExpression):
>          name = expr['alternate']
> @@ -1202,7 +1210,7 @@ def _def_alternate_type(self, expr: QAPIExpression):
>          self._def_definition(
>              QAPISchemaAlternateType(
>                  name, info, expr.doc, ifcond, features,
> -                QAPISchemaVariants(None, info, tag_member, variants)))
> +                QAPISchemaAlternatives(info, variants, tag_member)))
>
>      def _def_command(self, expr: QAPIExpression):
>          name = expr['command']
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index c39d054d2c..05da30b855 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -23,6 +23,7 @@
>  )
>  from .schema import (
>      QAPISchema,
> +    QAPISchemaAlternatives,
>      QAPISchemaEnumMember,
>      QAPISchemaFeature,
>      QAPISchemaIfCond,
> @@ -369,11 +370,11 @@ def visit_alternate_type(self,
>                               info: Optional[QAPISourceInfo],
>                               ifcond: QAPISchemaIfCond,
>                               features: List[QAPISchemaFeature],
> -                             variants: QAPISchemaVariants) -> None:
> +                             alternatives: QAPISchemaAlternatives) -> None:
>          with ifcontext(ifcond, self._genh):
>              self._genh.preamble_add(gen_fwd_object_or_array(name))
>          self._genh.add(gen_object(name, ifcond, None,
> -                                  [variants.tag_member], variants))
> +                                  [alternatives.tag_member], alternatives))
>          with ifcontext(ifcond, self._genh, self._genc):
>              self._gen_type_cleanup(name)
>
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index c56ea4d724..725bfcef50 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -28,6 +28,7 @@
>  )
>  from .schema import (
>      QAPISchema,
> +    QAPISchemaAlternatives,
>      QAPISchemaEnumMember,
>      QAPISchemaEnumType,
>      QAPISchemaFeature,
> @@ -222,7 +223,8 @@ def gen_visit_enum(name: str) -> str:
>                   c_name=c_name(name))
>
>
> -def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
> +def gen_visit_alternate(name: str,
> +                        alternatives: QAPISchemaAlternatives) -> str:
>      ret = mcgen('''
>
>  bool visit_type_%(c_name)s(Visitor *v, const char *name,
> @@ -244,7 +246,7 @@ def gen_visit_alternate(name: str, variants: 
> QAPISchemaVariants) -> str:
>  ''',
>                  c_name=c_name(name))
>
> -    for var in variants.variants:
> +    for var in alternatives.variants:
>          ret += var.ifcond.gen_if()
>          ret += mcgen('''
>      case %(case)s:
> @@ -414,10 +416,10 @@ def visit_alternate_type(self,
>                               info: Optional[QAPISourceInfo],
>                               ifcond: QAPISchemaIfCond,
>                               features: List[QAPISchemaFeature],
> -                             variants: QAPISchemaVariants) -> None:
> +                             alternatives: QAPISchemaAlternatives) -> None:
>          with ifcontext(ifcond, self._genh, self._genc):
>              self._genh.add(gen_visit_decl(name))
> -            self._genc.add(gen_visit_alternate(name, variants))
> +            self._genc.add(gen_visit_alternate(name, alternatives))
>
>
>  def gen_visit(schema: QAPISchema,
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 14f7b62a44..b66ceb81b8 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -61,9 +61,10 @@ def visit_object_type(self, name, info, ifcond, features,
>          self._print_if(ifcond)
>          self._print_features(features)
>
> -    def visit_alternate_type(self, name, info, ifcond, features, variants):
> +    def visit_alternate_type(self, name, info, ifcond, features,
> +                             alternatives):
>          print('alternate %s' % name)
> -        self._print_variants(variants)
> +        self._print_variants(alternatives)
>          self._print_if(ifcond)
>          self._print_features(features)
>
>