On Thu, Feb 3, 2022 at 5:40 AM Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> On Wed, Jan 26, 2022 at 01:58:19PM -0500, John Snow wrote:
> > (1) QAPI types the return of many commands as an empty object. That's
> > literally indeed what happens on the wire, and it makes sense in that
> > if these commands were ever to return anything, it is a "compatible
> > evolution" to include new fields in such an object. In Python, this
> > does not make much sense, though; as this is somewhat hard to
> > annotate:
> >
> > async def stop() -> Literal[{}]: ...
> >
> > The more pythonic signature is:
> >
> > async def stop() -> None: ...
> >
> > I feel like it's spiritually equivalent, but I am aware it is a
> > distinct choice that is being made. It could theoretically interfere
> > with a choice made in QAPI later to explicitly return Null. I don't
> > think we'd do that, but it's still a choice of abstraction that
> > reduces the resolution of distinct return signatures.
>
> As you mention though, bear in mind that a command returning
> nothing today, might return something tomorrow. IOW, we'd be
> going from a empty dict to a non-empty dict. If you use "None"
> then it'd be gonig from None to a non-empty dict.
>
> I think you can argue both of these approaches are backwards
> compatible. The python app is not likely to be looking at
> the return type at all initially - unlike C, errors get
> raised as exceptions, so you don't need to look at return
> type to distinguish succes from failure.
>
> So I'd consider it merely a documentation problem to say
> that a "None" return type might later change to a dict.
> Dunno how you represent that in python type annotations,
> but I presume there's a way.I don't think type hints offer a temporal dimension to them yet :) I started writing a much lengthier response, but the subject of compatibility is really complex and I am not prepared to give a comprehensive response to some of the issues you raise ... so instead I will say "Wow, good points!" and I will get back to you on some of it. A lot of things will only make sense if we are talking about a very specific type of project, with very specific goals that are clearly established. I don't really have that ready, here; I am just experimenting to learn where some of the pain points are, still. So... I'll get back to you on this. > We do allow fields to be deleted, which is a *non-compatible* > evolution, but they MUST have been marked as deprecated for > 2 cycles first. Good point. > I'd say sorting required vs optional arguments is doomed as > a strategy. Stuff that is mandatory today can be made optional > tomorrow and I think that's reasonable to want todo as we > evolve an API design. Also a good point. Python requires all mandatory arguments precede all optional ones, so you're probably right that in order to maximize cross-version compatibility, keyword-only arguments for *all* arguments both mandatory and optional is the only real way to fly. I think this might cause problems for Marc-Andre in rust/dbus land, but it's tractable in Python. I am unclear on the ramifications for golang. (Victor, Marc-Andre, any input here?) [...] > So I think however you express API params in python needs > to cope with this scenario, which means not sorting > args based on optional vs required. Effectively need > to assume that all args are potentially optional on a > long enough timeframe. We still have to sort them to fulfill Python grammar requirements, but if they are *all* keyword-only, then the order the programmer uses to actually invoke the function isn't important. > I don't think you have any choice - they must all be keyword > only if you want protection from future schema changes. You're right, it's simply more robust. > It sounds like you need a wrapper type. This StrOrNull scenario > is QMP's "alternate" type IIUC, but you're trying to avoid > expressing the existance fo the "alternate" type in the API Yes. This is a very clean way to type it, but it is a little more laborious for the user to have to remember to wrap certain strings in a special constructor first. Still, this is a good trick that I hadn't considered. I'll keep it in mind for future experiments. > I think it is not unreasonable to expose the struct names > on introspection though, and just accept that it ties our > hands a little more to avoid renaming structs. I don't > think we rename frequently enough that this matters. I feel like I don't have a real stake in this issue yet. Maybe we can discuss bolstering the introspection data if we decide that we really, really would like the ability to generate bindings dynamically on the client side. I'm not sure *I* even want that enough to really push for this change yet. (Again, I think I need to come forward with something more concrete than an experiment before I dive too deeply into this issue. I'll get back to you.) I wouldn't mind hearing from Markus on what he believes the value of anonymizing the types names is. My current understanding is: "The type names aren't necessary to speak QMP, so they aren't necessary to reveal. We operate on a need-to-know basis, and nobody has needed to know." (The most tangible story I can think of is that we don't want clients to do things like assume they can index the introspection data in a hashmap and rely on looking up specific type names.) > A "NORETURN" flag sounds like a reasonable idea. Markus has gently pointed out that we do have this information in the schema, but it isn't revealed over introspection data *and* we do not have introspection for QGA anyway. We /could/ remove success-response and add a 'NORETURN' feature flag, modifying the generator to use that flag (instead of success-response), and then we'd get away with not having to modify the introspection schema. But we'd still have to add introspection in general to QGA, which rather sounds like where the bulk of the work is anyway. > > Regards, > Daniel Thanks! I've got a lot to think about. --js
