From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CC69AC433FE for ; Tue, 8 Dec 2020 00:23:13 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1341A23A04 for ; Tue, 8 Dec 2020 00:23:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1341A23A04 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:46760 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kmQmd-0000mO-D2 for qemu-devel@archiver.kernel.org; Mon, 07 Dec 2020 19:23:11 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:33598) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kmQl6-0000IR-Bf for qemu-devel@nongnu.org; Mon, 07 Dec 2020 19:21:36 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:59799) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.90_1) (envelope-from ) id 1kmQl2-0004JM-KD for qemu-devel@nongnu.org; Mon, 07 Dec 2020 19:21:35 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1607386891; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=b200TLGAgfznP8G5dj/Zz/GfdFGRQTLP/th9gnezFI8=; b=cOkXWhq3c/cWLh9iR34vtMrheIUJVsq/kAmrrhRES7dvmD9Osl+NLeSv4cHslpjo2QV13k szbLAhKm4bEU3Wt8L5EW8XRxbf6VcvXltOK4zCgNcKk7vsXZ8mFZuV9hG3MKF1iyd7Yn+q mCl5vO2gOcRLYXoz0L7YHL0N6tRHT40= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-189-pF5tEtUuOrirb8RdvdfHrA-1; Mon, 07 Dec 2020 19:21:29 -0500 X-MC-Unique: pF5tEtUuOrirb8RdvdfHrA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1C08B802B56 for ; Tue, 8 Dec 2020 00:21:28 +0000 (UTC) Received: from [10.10.116.117] (ovpn-116-117.rdu2.redhat.com [10.10.116.117]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4FFD15D719; Tue, 8 Dec 2020 00:21:27 +0000 (UTC) Subject: Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure To: Markus Armbruster References: <20201026194251.11075-1-jsnow@redhat.com> <20201026194251.11075-10-jsnow@redhat.com> <87y2j1zk35.fsf@dusky.pond.sub.org> From: John Snow Message-ID: Date: Mon, 7 Dec 2020 19:21:26 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <87y2j1zk35.fsf@dusky.pond.sub.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=jsnow@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=216.205.24.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Cleber Rosa , qemu-devel@nongnu.org, Eduardo Habkost Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 11/16/20 5:12 AM, Markus Armbruster wrote: > John Snow writes: > >> This replaces _make_tree with Annotated(). By creating it as a generic >> container, we can more accurately describe the exact nature of this >> particular value. i.e., each Annotated object is actually an >> Annotated, describing its contained value. >> >> This adds stricter typing to Annotated nodes and extra annotated >> information. > > Inhowfar? > The Generic[T] trick lets us express the type of the annotated node itself, which is more specific than Tuple[_something, ...etc...] and this type can be preserved when we peel the annotations off. It's not super crucial, but like you say, the big benefit is the field names and strict types for the special-purpose structure. >> It also replaces a check of "isinstance tuple" with the >> much more explicit "isinstance Annotated" which is guaranteed not to >> break if a tuple is accidentally introduced into the type tree. (Perhaps >> as a result of a bad conversion from a list.) > > Sure this is worth writing home about? Such accidents seem quite > unlikely. > We all have our phobias. I find "isinstance(x, extremely_common_stdlib_type)" to be extremely fragile and likely to frustrate. Maybe what's unlikely is anyone editing this code ever again. You've mentioned wanting to look into changing how the schema information is stored in QEMU before, so a lot of this might not matter for too much longer, who knows. > For me, the commit's benefit is making the structure of the annotated > tree node more explicit (your first paragraph, I guess). It's a bit of > a pattern in developing Python code: we start with a Tuple because it's > terse and easy, then things get more complex, terse becomes too terse, > and we're replacing the Tuple with a class. > Yep. >> Signed-off-by: John Snow >> --- >> scripts/qapi/introspect.py | 97 +++++++++++++++++++------------------- >> 1 file changed, 48 insertions(+), 49 deletions(-) >> >> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >> index a0978cb3adb..a261e402d69 100644 >> --- a/scripts/qapi/introspect.py >> +++ b/scripts/qapi/introspect.py >> @@ -13,12 +13,13 @@ >> from typing import ( >> Any, >> Dict, >> + Generic, >> + Iterable, >> List, >> Optional, >> Sequence, >> - Tuple, >> + TypeVar, >> Union, >> - cast, >> ) >> >> from .common import ( >> @@ -63,50 +64,48 @@ >> _scalar = Union[str, bool, None] >> _nonscalar = Union[Dict[str, _stub], List[_stub]] >> _value = Union[_scalar, _nonscalar] >> -TreeValue = Union[_value, 'Annotated'] >> +TreeValue = Union[_value, 'Annotated[_value]'] >> >> # This is just an alias for an object in the structure described above: >> _DObject = Dict[str, object] >> >> -# Represents the annotations themselves: >> -Annotations = Dict[str, object] >> >> -# Represents an annotated node (of some kind). >> -Annotated = Tuple[_value, Annotations] >> +_AnnoType = TypeVar('_AnnoType', bound=TreeValue) >> >> >> -def _make_tree(obj: Union[_DObject, str], ifcond: List[str], >> - comment: Optional[str] = None) -> Annotated: >> - extra: Annotations = { >> - 'if': ifcond, >> - 'comment': comment, >> - } >> - return (obj, extra) >> +class Annotated(Generic[_AnnoType]): >> + """ >> + Annotated generally contains a SchemaInfo-like type (as a dict), >> + But it also used to wrap comments/ifconds around scalar leaf values, >> + for the benefit of features and enums. >> + """ >> + # Remove after 3.7 adds @dataclass: >> + # pylint: disable=too-few-public-methods >> + def __init__(self, value: _AnnoType, ifcond: Iterable[str], >> + comment: Optional[str] = None): >> + self.value = value >> + self.comment: Optional[str] = comment >> + self.ifcond: Sequence[str] = tuple(ifcond) >> >> >> -def _tree_to_qlit(obj: TreeValue, >> - level: int = 0, >> +def _tree_to_qlit(obj: TreeValue, level: int = 0, >> suppress_first_indent: bool = False) -> str: >> >> def indent(level: int) -> str: >> return level * 4 * ' ' >> >> - if isinstance(obj, tuple): >> - ifobj, extra = obj >> - ifcond = cast(Optional[Sequence[str]], extra.get('if')) >> - comment = extra.get('comment') >> - >> + if isinstance(obj, Annotated): >> msg = "Comments and Conditionals not implemented for dict values" >> - assert not (suppress_first_indent and (ifcond or comment)), msg >> + assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg >> >> ret = '' >> - if comment: >> - ret += indent(level) + '/* %s */\n' % comment >> - if ifcond: >> - ret += gen_if(ifcond) >> - ret += _tree_to_qlit(ifobj, level, suppress_first_indent) >> - if ifcond: >> - ret += '\n' + gen_endif(ifcond) >> + if obj.comment: >> + ret += indent(level) + '/* %s */\n' % obj.comment >> + if obj.ifcond: >> + ret += gen_if(obj.ifcond) >> + ret += _tree_to_qlit(obj.value, level, suppress_first_indent) >> + if obj.ifcond: >> + ret += '\n' + gen_endif(obj.ifcond) >> return ret >> >> ret = '' >> @@ -153,7 +152,7 @@ def __init__(self, prefix: str, unmask: bool): >> ' * QAPI/QMP schema introspection', __doc__) >> self._unmask = unmask >> self._schema: Optional[QAPISchema] = None >> - self._trees: List[Annotated] = [] >> + self._trees: List[Annotated[_DObject]] = [] >> self._used_types: List[QAPISchemaType] = [] >> self._name_map: Dict[str, str] = {} >> self._genc.add(mcgen(''' >> @@ -219,10 +218,9 @@ def _use_type(self, typ: QAPISchemaType) -> str: >> return self._name(typ.name) >> >> @classmethod >> - def _gen_features(cls, >> - features: List[QAPISchemaFeature] >> - ) -> List[Annotated]: >> - return [_make_tree(f.name, f.ifcond) for f in features] >> + def _gen_features( >> + cls, features: List[QAPISchemaFeature]) -> List[Annotated[str]]: > > Indent this way from the start for lesser churn. > OK >> + return [Annotated(f.name, f.ifcond) for f in features] >> >> def _gen_tree(self, name: str, mtype: str, obj: _DObject, >> ifcond: List[str], >> @@ -238,10 +236,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, >> obj['meta-type'] = mtype >> if features: >> obj['features'] = self._gen_features(features) >> - self._trees.append(_make_tree(obj, ifcond, comment)) >> + self._trees.append(Annotated(obj, ifcond, comment)) >> >> def _gen_member(self, >> - member: QAPISchemaObjectTypeMember) -> Annotated: >> + member: QAPISchemaObjectTypeMember) -> Annotated[_DObject]: > > Long line. Ty hanging indent. > OK. Admittedly, I hate hanging the return argument, I think it looks bad. Worst part of python types. :( >> obj: _DObject = { >> 'name': member.name, >> 'type': self._use_type(member.type) >> @@ -250,19 +248,19 @@ def _gen_member(self, >> obj['default'] = None >> if member.features: >> obj['features'] = self._gen_features(member.features) >> - return _make_tree(obj, member.ifcond) >> + return Annotated(obj, member.ifcond) >> >> def _gen_variants(self, tag_name: str, >> variants: List[QAPISchemaVariant]) -> _DObject: >> return {'tag': tag_name, >> 'variants': [self._gen_variant(v) for v in variants]} >> >> - def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated: >> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]: >> obj: _DObject = { >> 'case': variant.name, >> 'type': self._use_type(variant.type) >> } >> - return _make_tree(obj, variant.ifcond) >> + return Annotated(obj, variant.ifcond) >> >> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo], >> json_type: str) -> None: >> @@ -272,10 +270,11 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo, >> ifcond: List[str], features: List[QAPISchemaFeature], >> members: List[QAPISchemaEnumMember], >> prefix: Optional[str]) -> None: >> - self._gen_tree(name, 'enum', >> - {'values': [_make_tree(m.name, m.ifcond, None) >> - for m in members]}, >> - ifcond, features) >> + self._gen_tree( >> + name, 'enum', >> + {'values': [Annotated(m.name, m.ifcond) for m in members]}, >> + ifcond, features >> + ) >> >> def visit_array_type(self, name: str, info: Optional[QAPISourceInfo], >> ifcond: List[str], >> @@ -300,12 +299,12 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo, >> ifcond: List[str], >> features: List[QAPISchemaFeature], >> variants: QAPISchemaVariants) -> None: >> - self._gen_tree(name, 'alternate', >> - {'members': [ >> - _make_tree({'type': self._use_type(m.type)}, >> - m.ifcond, None) >> - for m in variants.variants]}, >> - ifcond, features) >> + self._gen_tree( >> + name, 'alternate', >> + {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond) > > Long line. Try breaking the line before m.ifcond, or before Annotated. > OK. >> + for m in variants.variants]}, >> + ifcond, features >> + ) >> >> def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str], >> features: List[QAPISchemaFeature],