diff mbox series

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

Message ID 20210511220601.2110055-2-jsnow@redhat.com
State New
Headers show
Series qapi: static typing conversion, pt5a | expand

Commit Message

John Snow May 11, 2021, 10:05 p.m. UTC
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.
- 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.
- 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.


The long version:

The error message string here is incorrect (since 52a474180a):

> 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'.

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>
---
 scripts/qapi/parser.py         | 18 +++++++++---------
 scripts/qapi/schema.py         | 11 +++++++++--
 scripts/qapi/source.py         |  3 ---
 tests/qapi-schema/test-qapi.py |  3 ---
 4 files changed, 18 insertions(+), 17 deletions(-)

Comments

Markus Armbruster May 18, 2021, 9:28 a.m. UTC | #1
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 :)

> - 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?

My best guess is the last one.  Its rationale is actually "drop code
handling the variant of QAPISourceInfo being removed in this patch".

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):
>
>> 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"?

>
> 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>
> ---
>  scripts/qapi/parser.py         | 18 +++++++++---------
>  scripts/qapi/schema.py         | 11 +++++++++--
>  scripts/qapi/source.py         |  3 ---
>  tests/qapi-schema/test-qapi.py |  3 ---
>  4 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index ca5e8e18e00..a53b735e7de 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -40,15 +40,9 @@ def __init__(self, fname, previously_included=None, incl_info=None):
>          previously_included = previously_included or set()
>          previously_included.add(os.path.abspath(fname))
>  
> -        try:
> -            fp = open(fname, 'r', encoding='utf-8')
> +        # May raise OSError; allow the caller to handle it.
> +        with open(fname, 'r', encoding='utf-8') as fp:
>              self.src = fp.read()
> -        except IOError as e:
> -            raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
> -                               "can't read %s file '%s': %s"
> -                               % ("include" if incl_info else "schema",
> -                                  fname,
> -                                  e.strerror))
>  
>          if self.src == '' or self.src[-1] != '\n':
>              self.src += '\n'
> @@ -129,7 +123,13 @@ def _include(self, include, info, incl_fname, previously_included):
>          if incl_abs_fname in previously_included:
>              return None
>  
> -        return QAPISchemaParser(incl_fname, previously_included, info)
> +        try:
> +            return QAPISchemaParser(incl_fname, previously_included, info)
> +        except OSError as err:
> +            raise QAPISemError(
> +                info,
> +                f"can't read include file '{incl_fname}': {err.strerror}"
> +            ) from err
>  
>      def _check_pragma_list_of_str(self, name, value, info):
>          if (not isinstance(value, list)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 3a4172fb749..d1d27ff7ee8 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -20,7 +20,7 @@
>  from typing import Optional
>  
>  from .common import POINTER_SUFFIX, c_name
> -from .error import QAPISemError, QAPISourceError
> +from .error import QAPIError, QAPISemError, QAPISourceError
>  from .expr import check_exprs
>  from .parser import QAPISchemaParser
>  
> @@ -849,7 +849,14 @@ def visit(self, visitor):
>  class QAPISchema:
>      def __init__(self, fname):
>          self.fname = fname
> -        parser = QAPISchemaParser(fname)
> +
> +        try:
> +            parser = QAPISchemaParser(fname)
> +        except OSError as err:
> +            raise QAPIError(
> +                f"can't read schema file '{fname}': {err.strerror}"
> +            ) from err
> +
>          exprs = check_exprs(parser.exprs)
>          self.docs = parser.docs
>          self._entity_list = []
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index 03b6ede0828..1ade864d7b9 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -10,7 +10,6 @@
>  # See the COPYING file in the top-level directory.
>  
>  import copy
> -import sys
>  from typing import List, Optional, TypeVar
>  
>  
> @@ -53,8 +52,6 @@ def next_line(self: T) -> T:
>          return info
>  
>      def loc(self) -> str:
> -        if self.fname is None:
> -            return sys.argv[0]
>          ret = self.fname
>          if self.line is not None:
>              ret += ':%d' % self.line
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index e8db9d09d91..f1c4deb9a51 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -128,9 +128,6 @@ def test_and_diff(test_name, dir_name, update):
>      try:
>          test_frontend(os.path.join(dir_name, test_name + '.json'))
>      except QAPIError as err:
> -        if err.info.fname is None:
> -            print("%s" % err, file=sys.stderr)
> -            return 2
>          errstr = str(err) + '\n'
>          if dir_name:
>              errstr = errstr.replace(dir_name + '/', '')

Patch looks good to me.
John Snow May 18, 2021, 1:14 p.m. UTC | #2
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 ;)

>> - 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"

> 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?)

>>
>>> 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.

>>
>> 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>
>> ---
>>   scripts/qapi/parser.py         | 18 +++++++++---------
>>   scripts/qapi/schema.py         | 11 +++++++++--
>>   scripts/qapi/source.py         |  3 ---
>>   tests/qapi-schema/test-qapi.py |  3 ---
>>   4 files changed, 18 insertions(+), 17 deletions(-)
>>
>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> index ca5e8e18e00..a53b735e7de 100644
>> --- a/scripts/qapi/parser.py
>> +++ b/scripts/qapi/parser.py
>> @@ -40,15 +40,9 @@ def __init__(self, fname, previously_included=None, incl_info=None):
>>           previously_included = previously_included or set()
>>           previously_included.add(os.path.abspath(fname))
>>   
>> -        try:
>> -            fp = open(fname, 'r', encoding='utf-8')
>> +        # May raise OSError; allow the caller to handle it.
>> +        with open(fname, 'r', encoding='utf-8') as fp:
>>               self.src = fp.read()
>> -        except IOError as e:
>> -            raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
>> -                               "can't read %s file '%s': %s"
>> -                               % ("include" if incl_info else "schema",
>> -                                  fname,
>> -                                  e.strerror))
>>   
>>           if self.src == '' or self.src[-1] != '\n':
>>               self.src += '\n'
>> @@ -129,7 +123,13 @@ def _include(self, include, info, incl_fname, previously_included):
>>           if incl_abs_fname in previously_included:
>>               return None
>>   
>> -        return QAPISchemaParser(incl_fname, previously_included, info)
>> +        try:
>> +            return QAPISchemaParser(incl_fname, previously_included, info)
>> +        except OSError as err:
>> +            raise QAPISemError(
>> +                info,
>> +                f"can't read include file '{incl_fname}': {err.strerror}"
>> +            ) from err
>>   
>>       def _check_pragma_list_of_str(self, name, value, info):
>>           if (not isinstance(value, list)
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 3a4172fb749..d1d27ff7ee8 100644
>> --- a/scripts/qapi/schema.py
>> +++ b/scripts/qapi/schema.py
>> @@ -20,7 +20,7 @@
>>   from typing import Optional
>>   
>>   from .common import POINTER_SUFFIX, c_name
>> -from .error import QAPISemError, QAPISourceError
>> +from .error import QAPIError, QAPISemError, QAPISourceError
>>   from .expr import check_exprs
>>   from .parser import QAPISchemaParser
>>   
>> @@ -849,7 +849,14 @@ def visit(self, visitor):
>>   class QAPISchema:
>>       def __init__(self, fname):
>>           self.fname = fname
>> -        parser = QAPISchemaParser(fname)
>> +
>> +        try:
>> +            parser = QAPISchemaParser(fname)
>> +        except OSError as err:
>> +            raise QAPIError(
>> +                f"can't read schema file '{fname}': {err.strerror}"
>> +            ) from err
>> +
>>           exprs = check_exprs(parser.exprs)
>>           self.docs = parser.docs
>>           self._entity_list = []
>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>> index 03b6ede0828..1ade864d7b9 100644
>> --- a/scripts/qapi/source.py
>> +++ b/scripts/qapi/source.py
>> @@ -10,7 +10,6 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import copy
>> -import sys
>>   from typing import List, Optional, TypeVar
>>   
>>   
>> @@ -53,8 +52,6 @@ def next_line(self: T) -> T:
>>           return info
>>   
>>       def loc(self) -> str:
>> -        if self.fname is None:
>> -            return sys.argv[0]
>>           ret = self.fname
>>           if self.line is not None:
>>               ret += ':%d' % self.line
>> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
>> index e8db9d09d91..f1c4deb9a51 100755
>> --- a/tests/qapi-schema/test-qapi.py
>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -128,9 +128,6 @@ def test_and_diff(test_name, dir_name, update):
>>       try:
>>           test_frontend(os.path.join(dir_name, test_name + '.json'))
>>       except QAPIError as err:
>> -        if err.info.fname is None:
>> -            print("%s" % err, file=sys.stderr)
>> -            return 2
>>           errstr = str(err) + '\n'
>>           if dir_name:
>>               errstr = errstr.replace(dir_name + '/', '')
> 
> Patch looks good to me.
> 

Well, that's good ;)
John Snow May 18, 2021, 7:01 p.m. UTC | #3
On 5/18/21 5:28 AM, Markus Armbruster wrote:
> QAPIError not having .info don't actually exist before this patch.

It's defined by QAPISourceError now, I just missed this spot in 
test-qapi. It isn't used in practice until now, however.

--js
Markus Armbruster May 19, 2021, 6:51 a.m. UTC | #4
John Snow <jsnow@redhat.com> writes:

> On 5/18/21 5:28 AM, Markus Armbruster wrote:
>> QAPIError not having .info don't actually exist before this patch.
>
> It's defined by QAPISourceError now, I just missed this spot in
> test-qapi. It isn't used in practice until now, however.

I had QAPIError mentally filed under abstract types / didn't bother to
formally make it one with decorators.  Just as well, because it's not
staying abstract: this patch creates instances.
Markus Armbruster May 19, 2021, 7:01 a.m. UTC | #5
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 ;)
John Snow May 19, 2021, 5:17 p.m. UTC | #6
On 5/19/21 3:01 AM, Markus Armbruster wrote:
> In commit f5d4361cda "qapi/source.py: add type hint annotations", I
> believe.
> 
> Hmm, this commit actually fixes incorrect typing, doesn't it?
> 

Yes.

It just wasn't caught because this file wasn't being checked yet. I 
tried to avoid this for a long time by making sure I tested my full 
conversion stack after every rebase, but it got too cumbersome.
John Snow May 19, 2021, 5:51 p.m. UTC | #7
On 5/19/21 3:01 AM, Markus Armbruster wrote:
> Hmm, this commit actually fixes incorrect typing, doesn't it?

Updated the commit message with *THREE* references to commits that this 
patch technically fixes.
diff mbox series

Patch

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ca5e8e18e00..a53b735e7de 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -40,15 +40,9 @@  def __init__(self, fname, previously_included=None, incl_info=None):
         previously_included = previously_included or set()
         previously_included.add(os.path.abspath(fname))
 
-        try:
-            fp = open(fname, 'r', encoding='utf-8')
+        # May raise OSError; allow the caller to handle it.
+        with open(fname, 'r', encoding='utf-8') as fp:
             self.src = fp.read()
-        except IOError as e:
-            raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
-                               "can't read %s file '%s': %s"
-                               % ("include" if incl_info else "schema",
-                                  fname,
-                                  e.strerror))
 
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
@@ -129,7 +123,13 @@  def _include(self, include, info, incl_fname, previously_included):
         if incl_abs_fname in previously_included:
             return None
 
-        return QAPISchemaParser(incl_fname, previously_included, info)
+        try:
+            return QAPISchemaParser(incl_fname, previously_included, info)
+        except OSError as err:
+            raise QAPISemError(
+                info,
+                f"can't read include file '{incl_fname}': {err.strerror}"
+            ) from err
 
     def _check_pragma_list_of_str(self, name, value, info):
         if (not isinstance(value, list)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 3a4172fb749..d1d27ff7ee8 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@ 
 from typing import Optional
 
 from .common import POINTER_SUFFIX, c_name
-from .error import QAPISemError, QAPISourceError
+from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
 from .parser import QAPISchemaParser
 
@@ -849,7 +849,14 @@  def visit(self, visitor):
 class QAPISchema:
     def __init__(self, fname):
         self.fname = fname
-        parser = QAPISchemaParser(fname)
+
+        try:
+            parser = QAPISchemaParser(fname)
+        except OSError as err:
+            raise QAPIError(
+                f"can't read schema file '{fname}': {err.strerror}"
+            ) from err
+
         exprs = check_exprs(parser.exprs)
         self.docs = parser.docs
         self._entity_list = []
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 03b6ede0828..1ade864d7b9 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -10,7 +10,6 @@ 
 # See the COPYING file in the top-level directory.
 
 import copy
-import sys
 from typing import List, Optional, TypeVar
 
 
@@ -53,8 +52,6 @@  def next_line(self: T) -> T:
         return info
 
     def loc(self) -> str:
-        if self.fname is None:
-            return sys.argv[0]
         ret = self.fname
         if self.line is not None:
             ret += ':%d' % self.line
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d91..f1c4deb9a51 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -128,9 +128,6 @@  def test_and_diff(test_name, dir_name, update):
     try:
         test_frontend(os.path.join(dir_name, test_name + '.json'))
     except QAPIError as err:
-        if err.info.fname is None:
-            print("%s" % err, file=sys.stderr)
-            return 2
         errstr = str(err) + '\n'
         if dir_name:
             errstr = errstr.replace(dir_name + '/', '')