Re: [PATCH 12/19] qapi/schema: split "checked" field into "checking" and "checked"

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On Wed, Nov 22, 2023, 9:02 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Differentiate between "actively in the process of checking" and
>> > "checking has completed". This allows us to clean up the types of some
>> > internal fields such as QAPISchemaObjectType's members field which
>> > currently uses "None" as a canary for determining if check has
>> > completed.
>>
>> Certain members become valid only after .check().  Two ways to code
>> that:
>>
>> 1. Assign to such members only in .check().  If you try to use them
>> before .check(), AttributeError.  Nice.  Drawback: pylint is unhappy,
>> W0201 attribute-defined-outside-init.
>>
>
> Can be overcome by declaring the field in __init__, which satisfies both
> the linter and my developer usability sense (Classes should be easy to have
> their properties enumerated by looking in one well known place.)
>
>
>> 2. Assign None in .__init__(), and the real value in .check().  If you
>> try to use them before .check(), you get None, which hopefully leads to
>> an error.  Meh, but pylint is happy.
>>
>> I picked 2. because pylint's warning made me go "when in Rome..."
>>
>
> Yep, this is perfectly cromulent dynamically typed Python. It's not the
> Roman's fault I'm packing us up to go to another empire.
>
>
>> With type hints, we can declare in .__init__(), and assign in .check().
>> Gives me the AttributeError I like, and pylint remains happy.  What do
>> you think?
>>
>
> Sounds good to me in general, I already changed this for 2/3 of my other
> uses of @property.
>
> (I'm only reluctant because I don't really like that it's a "lie", but in
> this case, without potentially significant rewrites, it's a reasonable type
> band-aid. Once we're type checked, we can refactor more confidently if we
> so desire, to make certain patterns less prominent or landmine-y.)

The general problem is "attribute value is valid only after a state
transition" (here: .member is valid only after .check()).

We want to catch uses of the attribute before it becomes valid.

We want to keep pylint and mypy happy.

Solutions:

1. Initialize in .__init__() to some invalid value.  Set it to the valid
   value in .check().

1.a. Pick the "natural" invalid value: None

   How to catch: assert attribute value is valid (here: .members is not
   None).  Easy to forget.  Better: when the use will safely choke on
   the invalid value (here: elide for uses like for m in .members),
   catch is automatic.

   Pylint: fine.

   Mypy: adding None to the set of values changes the type from T to
   Optional[T].  Because of this, mypy commonly can't prove valid uses
   are valid.  Keeping it happy requires cluttering the code with
   assertions and such.  Meh.

   Note: catching invalid uses is a run time check.  Mypy won't.

1.b. Pick an invalid value of type T (here: [])

   How to catch: same as 1.a., except automatic catch is rare.  Meh.

   Pylint: fine.

   Mypy: fine.

2. Declare in .__init__() without initializing.  Initialize to valid
   value in .check()

   How to catch: always automatic.  Good, me want!

   Pylint: fine.

   Mypy: fine.

   Note: catching invalid uses is a run time check.  Mypy won't.

3. Express the state transition in the type system

   To catch invalid uses statically with mypy, we need to use different
   types before and after the state transition.  Feels possible.  Also
   feels ludicrously overengineered.

May I have 2., please?

>> Splitting .checked feels like a separate change, though.  I can't quite
>> see why we need it.  Help me out: what problem does it solve?
>>
>
> Mechanically, I wanted to eliminate the Optional type from the members
> field, but you have conditionals in the code that use the presence or
> absence of this field as a query to determine if we had run check or not
> yet.
>
> So I did the stupidest possible thing and added a "checked" variable to
> explicitly represent it.

If 2. complicates the existing "have we .check()ed?" code too much, then
adding such a variable may indeed be useful.

>> > This simplifies the typing from a cumbersome Optional[List[T]] to merely
>> > a List[T], which is more pythonic: it is safe to iterate over an empty
>> > list with "for x in []" whereas with an Optional[List[T]] you have to
>> > rely on the more cumbersome "if L: for x in L: ..."
>>
>> Yes, but when L is None, it's *invalid*, and for i in L *should* fail
>> when L is invalid.
>>
>
> Sure, but it's so invalid that it causes static typing errors.
>
> You can't write "for x in None" in a way that makes mypy happy, None is not
> iterable.

A variable that is declared, but not initialized (2. above) also not
iterable, and it does make mypy happy, doesn't it?

> If you want to preserve the behavior of "iterating an empty collection is
> an Assertion", you need a custom iterator that throws an exception when the
> collection is empty. I can do that, if you'd like, but I think it's
> actually fine to just allow the collection to be empty and to just catch
> the error in check() with either an assertion (oops, something went wrong
> and the list is empty, this should never happen) or a QAPISemError (oops,
> you didn't specify any members, which is illegal.)
>
> I'd prefer to catch this in check and just allow the iterator to permit
> empty iterators at the callsite, knowing it'll never happen.
>
>
>> I think the actual problem is something else.  By adding the type hint
>> Optional[List[T]], the valid uses of L become type errors.  We really
>> want L to be a List[T].  Doesn't mean we have to (or want to) make uses
>> of invalid L "work".
>>
>
> I didn't think I did allow for invalid uses to work - if the list should
> semantically never be empty, I think it's fine to enforce that in schema.py
> during construction of the schema object and to assume all uses of "for x
> in L: ..." are inherently valid.
>
>
>> > RFC: are we guaranteed to have members here? can we just use "if
>> > members" without adding the new field?
>>
>> I'm afraid I don't understand these questions.
>>
>
> I think you answered this one for me; I was asking if it was ever valid in
> any circumstance to have an empty members list, but I think you've laid out
> in your response that it isn't.
>
> And I think with that knowledge I can simplify this patch, but don't quite
> recall. (On my mobile again, please excuse my apparent laziness.)

Feel excused!

[...]