Re: [PATCH v2 01/21] qapi/parser: Don't try to handle file errors

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 5/18/21 5:28 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Remove the try/except block that handles file-opening errors in
>>> QAPISchemaParser.__init__() and add one each to
>>> QAPISchemaParser._include() and QAPISchema.__init__() respectively.
>>>
>>>
>>> The short-ish version of what motivates this patch is:
>>>
>>> - It's hard to write a good error message in the init method,
>>>    because we need to determine the context of our caller to do so.
>>>    It's easier to just let the caller write the message.
>> 
>> I kind of disagree, but that's okay; it's your commit message :)
>> 
>
> I can nix the paragraph if you want, the primary purpose was to explain 
> to you what I was thinking anyway, and you already know ;)

Nah, keep it.

>>> - We don't want to allow QAPISourceInfo(None, None, None) to exist.
>>> - Errors made using such an object are currently incorrect.
>>> - It's not technically a semantic error if we cannot open the schema
>>> - There are various typing constraints that make mixing these two cases
>>>    undesirable for a single special case.
>>>
>>> Other considerations:
>>>
>>> - open() is moved to a 'with' block to ensure file pointers are
>>>    cleaned up deterministically.
>> 
>> Improvement over v1's leak claim.  Sold!
>> 
>>> - Python 3.3 deprecated IOError and made it a synonym for OSError.
>>>    Avoid the misleading perception these exception handlers are
>>>    narrower than they really are.
>>> - Not all QAPIError objects have an 'info' field, so remove that stanza
>>>    from test-qapi. Don't bother to replace the early exit on purpose
>>>    so that we can test its output in the next commit.
>> 
>> To which hunk exactly does the last item refer?
>> 
>
> Sigh, "Early return", not *exit* -- and I'm referring to the test-qapi hunk.
>
>> My best guess is the last one.  Its rationale is actually "drop code
>> handling the variant of QAPISourceInfo being removed in this patch".
>> 
>
> That too ... I just meant to say "It doesn't need to be replaced"

Can we the commit message clearer here?  Maybe:

    - test-qapi's code handling None fname is now dead.  Drop it.

Or just drop the item entirely.

>> QAPIError not having .info don't actually exist before this patch.
>> 
>> I'm afraid I don't get the second sentence.
>>  >>
>>>
>>> The long version:
>>>
>>> The error message string here is incorrect (since 52a474180a):
>> 
>> I think this reads slightly better:
>> 
>>    The error message here is incorrect (since commit 52a474180a):
>
> OK (If I need to respin I'll change it?)

Or I change it in my tree if we decide we don't need a full respin.

>>>> python3 qapi-gen.py 'fake.json'
>>> qapi-gen.py: qapi-gen.py: can't read schema file 'fake.json': No such file 
>>> or directory
>>>
>>> In pursuing it, we find that QAPISourceInfo has a special accommodation
>>> for when there's no filename. Meanwhile, the intended typing of
>>> QAPISourceInfo was (non-optional) 'str'.
>> 
>> Not sure about "intended".  When I wrote the code, I intended ".fname is
>> str means it's a position that file; None means it's not in a file".  I
>> didn't intend typing, because typing wasn't a concern back then.
>> 
>> Do you mean something like "we'd prefer to type .fname as (non-optional)
>> str"?
>> 
>
> Really, I meant *I* intended that typing. I just have a habit of using 
> "we" for F/OSS commit messages.
>
> What I am really saying: When I typed this field, I didn't realize it 
> could be None actually -- I see this as a way to fix the "intended 
> typing" that I established however many commits ago.

In commit f5d4361cda "qapi/source.py: add type hint annotations", I
believe.

Hmm, this commit actually fixes incorrect typing, doesn't it?

>>>
>>> To remove this, I'd want to avoid having a "fake" QAPISourceInfo
>>> object. We also don't want to explicitly begin accommodating
>>> QAPISourceInfo itself being None, because we actually want to eventually
>>> prove that this can never happen -- We don't want to confuse "The file
>>> isn't open yet" with "This error stems from a definition that wasn't
>>> defined in any file".
>>>
>>> (An earlier series tried to create a dummy info object, but it was tough
>>> to prove in review that it worked correctly without creating new
>>> regressions. This patch avoids that distraction. We would like to first
>>> prove that we never raise QAPISemError for any built-in object before we
>>> add "special" info objects. We aren't ready to do that yet.)
>>>
>>> So, which way out of the labyrinth?
>>>
>>> Here's one way: Don't try to handle errors at a level with "mixed"
>>> semantic contexts; i.e. don't mix inclusion errors (should report a
>>> source line where the include was triggered) and command line errors
>>> (where we specified a file we couldn't read).
>>>
>>> Remove the error handling from the initializer of the parser. Pythonic!
>>> Now it's the caller's job to figure out what to do about it. Handle the
>>> error in QAPISchemaParser._include() instead, where we can write a
>>> targeted error message where we are guaranteed to have an 'info' context
>>> to report with.
>>>
>>> The root level error can similarly move to QAPISchema.__init__(), where
>>> we know we'll never have an info context to report with, so we use a
>>> more abstract error type.
>>>
>>> Now the error looks sensible again:
>>>
>>>> python3 qapi-gen.py 'fake.json'
>>> qapi-gen.py: can't read schema file 'fake.json': No such file or directory
>>>
>>> With these error cases separated, QAPISourceInfo can be solidified as
>>> never having placeholder arguments that violate our desired types. Clean
>>> up test-qapi along similar lines.
>>>
>>> Fixes: 52a474180a
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>

[...]

>> Patch looks good to me.
>> 
>
> Well, that's good ;)