We use None to represent an object that has no source information
because it's a builtin. This complicates interface typing, since many
interfaces expect that there is an info object available to print errors
with.

Introduce a special QAPISourceInfo that represents these built-ins so
that if an error should so happen to occur relating to one of these
builtins that we will be able to print its information, and interface
typing becomes simpler: you will always have a source info object.

This object will evaluate as False, so "if info" is a valid idiomatic
construct.

NB: It was intentional to not allow empty constructors or similar to
create "empty" source info objects; callers must explicitly invoke
'builtin()' to pro-actively opt into using the sentinel. This should
prevent use-by-accident.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/source.py | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index d7a79a9b8aee..64af7318cb67 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -11,7 +11,12 @@
 
 import copy
 import sys
-from typing import List, Optional, TypeVar
+from typing import (
+    List,
+    Optional,
+    Type,
+    TypeVar,
+)
 
 
 class QAPISchemaPragma:
@@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
         self.defn_meta: Optional[str] = None
         self.defn_name: Optional[str] = None
 
+    @classmethod
+    def builtin(cls: Type[T]) -> T:
+        """
+        Create a SourceInfo object corresponding to a builtin definition.
+        """
+        return cls('', -1, None)
+
+    def __bool__(self) -> bool:
+        # "if info: ..." is false if info is the builtin sentinel.
+        return bool(self.fname)
+
     def set_defn(self, meta: str, name: str) -> None:
         self.defn_meta = meta
         self.defn_name = name
@@ -73,4 +89,6 @@ def include_path(self) -> str:
         return ret
 
     def __str__(self) -> str:
+        if not bool(self):
+            return '[builtin]'
         return self.include_path() + self.in_defn() + self.loc()
-- 
2.26.2

John Snow <jsnow@redhat.com> writes:

> We use None to represent an object that has no source information
> because it's a builtin. This complicates interface typing, since many
> interfaces expect that there is an info object available to print errors
> with.
>
> Introduce a special QAPISourceInfo that represents these built-ins so
> that if an error should so happen to occur relating to one of these
> builtins that we will be able to print its information, and interface
> typing becomes simpler: you will always have a source info object.

Two aspects mixed up:

1. Represent "no source info" as special QAPISourceInfo instead of
   None

   This is what de-complicates interface typing.

2. On error with "no source info", don't crash.

   I have my doubts on this one.

   Such an error means the QAPI code generator screwed up, at least in
   theory.  Crashing is only proper.  It gets the screwup fixed.

   In practice, errors due to interactions between built-in stuff and
   user-defined stuff could conceivably escape testing.  I can't
   remember such a case offhand.

   Will the "no source info" error be more useful than a crash?
   Possibly.  Will it get the screwup fixed?  Maybe not.

Can we separate the two aspects?

>
> This object will evaluate as False, so "if info" is a valid idiomatic
> construct.

Suggest s/is a valid/remains a valid/.

Not 100% sure we'll want to keep this idiom, but now is not the time to
worry about that.

>
> NB: It was intentional to not allow empty constructors or similar to
> create "empty" source info objects; callers must explicitly invoke
> 'builtin()' to pro-actively opt into using the sentinel. This should
> prevent use-by-accident.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/source.py | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index d7a79a9b8aee..64af7318cb67 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -11,7 +11,12 @@
>  
>  import copy
>  import sys
> -from typing import List, Optional, TypeVar
> +from typing import (
> +    List,
> +    Optional,
> +    Type,
> +    TypeVar,
> +)
>  
>  
>  class QAPISchemaPragma:
> @@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
>          self.defn_meta: Optional[str] = None
>          self.defn_name: Optional[str] = None
>  
> +    @classmethod
> +    def builtin(cls: Type[T]) -> T:
> +        """
> +        Create a SourceInfo object corresponding to a builtin definition.

Let's spell it built-in for consistency with existing comments.

Could perhaps shorten "a SourceInfo object" to "an instance".

> +        """
> +        return cls('', -1, None)

No users?  Peeking ahead... aha, they are in Patch 08.  Any particular
reason for putting PATCH 07 between the two?  Could PATCH 08 be squashed
into this one?

> +
> +    def __bool__(self) -> bool:
> +        # "if info: ..." is false if info is the builtin sentinel.
> +        return bool(self.fname)

Nitpicking...  "The builtin sentinel" suggests there is just one.  PATCH
08 creates several.  I don't mind, but let's say something like "if
@info corresponds to a built-in definition".

> +
>      def set_defn(self, meta: str, name: str) -> None:
>          self.defn_meta = meta
>          self.defn_name = name
> @@ -73,4 +89,6 @@ def include_path(self) -> str:
>          return ret
>  
>      def __str__(self) -> str:
> +        if not bool(self):
> +            return '[builtin]'
>          return self.include_path() + self.in_defn() + self.loc()

Looks like we can separate the two aspects easily:

       def __str__(self) -> str:
  +        assert not bool(self)
           return self.include_path() + self.in_defn() + self.loc()

On 12/16/20 4:22 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> We use None to represent an object that has no source information
>> because it's a builtin. This complicates interface typing, since many
>> interfaces expect that there is an info object available to print errors
>> with.
>>
>> Introduce a special QAPISourceInfo that represents these built-ins so
>> that if an error should so happen to occur relating to one of these
>> builtins that we will be able to print its information, and interface
>> typing becomes simpler: you will always have a source info object.
> 
> Two aspects mixed up:
> 
> 1. Represent "no source info" as special QAPISourceInfo instead of
>     None
> 
>     This is what de-complicates interface typing.
> 

Yup.

> 2. On error with "no source info", don't crash.
> 
>     I have my doubts on this one.
> 
>     Such an error means the QAPI code generator screwed up, at least in
>     theory.  Crashing is only proper.  It gets the screwup fixed.
> 
>     In practice, errors due to interactions between built-in stuff and
>     user-defined stuff could conceivably escape testing.  I can't
>     remember such a case offhand.
> 
>     Will the "no source info" error be more useful than a crash?
>     Possibly.  Will it get the screwup fixed?  Maybe not.
> 
> Can we separate the two aspects?
> 

We can add an intentional assertion, if you'd like, that makes such 
cases obvious -- but if we are already in the error printer, QAPI is 
likely already in the process of crashing and printing an error.

So, Is this really an issue?

>>
>> This object will evaluate as False, so "if info" is a valid idiomatic
>> construct.
> 
> Suggest s/is a valid/remains a valid/.
> 
> Not 100% sure we'll want to keep this idiom, but now is not the time to
> worry about that.
> 

OK.

>>
>> NB: It was intentional to not allow empty constructors or similar to
>> create "empty" source info objects; callers must explicitly invoke
>> 'builtin()' to pro-actively opt into using the sentinel. This should
>> prevent use-by-accident.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/source.py | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>> index d7a79a9b8aee..64af7318cb67 100644
>> --- a/scripts/qapi/source.py
>> +++ b/scripts/qapi/source.py
>> @@ -11,7 +11,12 @@
>>   
>>   import copy
>>   import sys
>> -from typing import List, Optional, TypeVar
>> +from typing import (
>> +    List,
>> +    Optional,
>> +    Type,
>> +    TypeVar,
>> +)
>>   
>>   
>>   class QAPISchemaPragma:
>> @@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
>>           self.defn_meta: Optional[str] = None
>>           self.defn_name: Optional[str] = None
>>   
>> +    @classmethod
>> +    def builtin(cls: Type[T]) -> T:
>> +        """
>> +        Create a SourceInfo object corresponding to a builtin definition.
> 
> Let's spell it built-in for consistency with existing comments.
> 
> Could perhaps shorten "a SourceInfo object" to "an instance".
> 

OK.

>> +        """
>> +        return cls('', -1, None)
> 
> No users?  Peeking ahead... aha, they are in Patch 08.  Any particular
> reason for putting PATCH 07 between the two?  Could PATCH 08 be squashed
> into this one?
> 

Too much soup in one pot: this patch highlights the "trick" and the 
subsequent patch shows the adoption of it. Seemed safe.

Goofy ordering, though. I've pushed the genc/genh patch downwards 
instead; you can squash them on commit if you'd like.

>> +
>> +    def __bool__(self) -> bool:
>> +        # "if info: ..." is false if info is the builtin sentinel.
>> +        return bool(self.fname)
> 
> Nitpicking...  "The builtin sentinel" suggests there is just one.  PATCH
> 08 creates several.  I don't mind, but let's say something like "if
> @info corresponds to a built-in definition".
> 

Fair enough. I don't mind nitpicks on comments and docstrings so much if 
it helps make things clearer for more people.

(And they don't cause me rebase pain as much as other nitpicks ;)

>> +
>>       def set_defn(self, meta: str, name: str) -> None:
>>           self.defn_meta = meta
>>           self.defn_name = name
>> @@ -73,4 +89,6 @@ def include_path(self) -> str:
>>           return ret
>>   
>>       def __str__(self) -> str:
>> +        if not bool(self):
>> +            return '[builtin]'
>>           return self.include_path() + self.in_defn() + self.loc()
> 
> Looks like we can separate the two aspects easily:
> 
>         def __str__(self) -> str:
>    +        assert not bool(self)
>             return self.include_path() + self.in_defn() + self.loc()
> 

Feels like abusing __str__ to prevent application logic we don't like 
elsewhere and unrelated to this class; I am still leaning on "If we are 
printing this, it's likely we're already crashing" unless you have news 
to the contrary for me.

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> We use None to represent an object that has no source information
>>> because it's a builtin. This complicates interface typing, since many
>>> interfaces expect that there is an info object available to print errors
>>> with.
>>>
>>> Introduce a special QAPISourceInfo that represents these built-ins so
>>> that if an error should so happen to occur relating to one of these
>>> builtins that we will be able to print its information, and interface
>>> typing becomes simpler: you will always have a source info object.
>> 
>> Two aspects mixed up:
>> 
>> 1. Represent "no source info" as special QAPISourceInfo instead of
>>     None
>> 
>>     This is what de-complicates interface typing.
>> 
>
> Yup.
>
>> 2. On error with "no source info", don't crash.
>> 
>>     I have my doubts on this one.
>> 
>>     Such an error means the QAPI code generator screwed up, at least in
>>     theory.  Crashing is only proper.  It gets the screwup fixed.
>> 
>>     In practice, errors due to interactions between built-in stuff and
>>     user-defined stuff could conceivably escape testing.  I can't
>>     remember such a case offhand.
>> 
>>     Will the "no source info" error be more useful than a crash?
>>     Possibly.  Will it get the screwup fixed?  Maybe not.
>> 
>> Can we separate the two aspects?
>> 
>
> We can add an intentional assertion, if you'd like, that makes such 
> cases obvious -- but if we are already in the error printer, QAPI is 
> likely already in the process of crashing and printing an error.
>
> So, Is this really an issue?

A patch limited to the first aspect merely tweaks an implementation
detail.

As soon as we include the second aspect, we get to debate how to handle
programming errors, and maybe whether any of the errors involving a
QAPISourceInfo.builtin() are *not* programming errors.

I'd prefer this series to remain focused on "enabling strict optional
checking in mypy for everything we have typed so far".

>>>
>>> This object will evaluate as False, so "if info" is a valid idiomatic
>>> construct.
>> 
>> Suggest s/is a valid/remains a valid/.
>> 
>> Not 100% sure we'll want to keep this idiom, but now is not the time to
>> worry about that.
>> 
>
> OK.
>
>>>
>>> NB: It was intentional to not allow empty constructors or similar to
>>> create "empty" source info objects; callers must explicitly invoke
>>> 'builtin()' to pro-actively opt into using the sentinel. This should
>>> prevent use-by-accident.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/source.py | 20 +++++++++++++++++++-
>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>>> index d7a79a9b8aee..64af7318cb67 100644
>>> --- a/scripts/qapi/source.py
>>> +++ b/scripts/qapi/source.py
>>> @@ -11,7 +11,12 @@
>>>   
>>>   import copy
>>>   import sys
>>> -from typing import List, Optional, TypeVar
>>> +from typing import (
>>> +    List,
>>> +    Optional,
>>> +    Type,
>>> +    TypeVar,
>>> +)
>>>   
>>>   
>>>   class QAPISchemaPragma:
>>> @@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
>>>           self.defn_meta: Optional[str] = None
>>>           self.defn_name: Optional[str] = None
>>>   
>>> +    @classmethod
>>> +    def builtin(cls: Type[T]) -> T:
>>> +        """
>>> +        Create a SourceInfo object corresponding to a builtin definition.
>> 
>> Let's spell it built-in for consistency with existing comments.
>> 
>> Could perhaps shorten "a SourceInfo object" to "an instance".
>> 
>
> OK.
>
>>> +        """
>>> +        return cls('', -1, None)
>> 
>> No users?  Peeking ahead... aha, they are in Patch 08.  Any particular
>> reason for putting PATCH 07 between the two?  Could PATCH 08 be squashed
>> into this one?
>> 
>
> Too much soup in one pot: this patch highlights the "trick" and the 
> subsequent patch shows the adoption of it. Seemed safe.
>
> Goofy ordering, though. I've pushed the genc/genh patch downwards 
> instead; you can squash them on commit if you'd like.
>
>>> +
>>> +    def __bool__(self) -> bool:
>>> +        # "if info: ..." is false if info is the builtin sentinel.
>>> +        return bool(self.fname)
>> 
>> Nitpicking...  "The builtin sentinel" suggests there is just one.  PATCH
>> 08 creates several.  I don't mind, but let's say something like "if
>> @info corresponds to a built-in definition".
>> 
>
> Fair enough. I don't mind nitpicks on comments and docstrings so much if 
> it helps make things clearer for more people.
>
> (And they don't cause me rebase pain as much as other nitpicks ;)
>
>>> +
>>>       def set_defn(self, meta: str, name: str) -> None:
>>>           self.defn_meta = meta
>>>           self.defn_name = name
>>> @@ -73,4 +89,6 @@ def include_path(self) -> str:
>>>           return ret
>>>   
>>>       def __str__(self) -> str:
>>> +        if not bool(self):
>>> +            return '[builtin]'
>>>           return self.include_path() + self.in_defn() + self.loc()
>> 
>> Looks like we can separate the two aspects easily:
>> 
>>         def __str__(self) -> str:
>>    +        assert not bool(self)
>>             return self.include_path() + self.in_defn() + self.loc()
>> 
>
> Feels like abusing __str__ to prevent application logic we don't like 
> elsewhere and unrelated to this class; I am still leaning on "If we are 
> printing this, it's likely we're already crashing" unless you have news 
> to the contrary for me.

You're right, making __str__() fail is not nice.  It has other uses,
e.g. when messing around interactively.

Ways out:

1. Avoid abuse of __str__() by naming the thing differently.

2. Lift the assertion into the relevant caller(s).  Unfortunately, the
   relevant caller is another __str__(): QAPIError's.  Next level up:
   the except suite in main() and also test-qapi.py's test_and_diff().
   Keeping the stack backtrace useful might require care.

3. Find a simpler way to signal "oops, programming error".

   For a simple batch program like this one, crashing is a perfectly
   fine reaction to programming errors.  Most of the time, it's also the
   simplest one.  Simple is *good*.  *Especially* when it's something
   that should never happen.

   If reporting an error is genuinely simpler than crashing: simple is
   good.  But the error message should clearly express "this is a bug,
   please report it", like a crash does.

   Drawbacks: we'd have to relax our rule "no error without a test
   case", and we'd lose the stack backtrace.

On 12/17/20 7:33 AM, Markus Armbruster wrote:
> A patch limited to the first aspect merely tweaks an implementation
> detail.
> 
> As soon as we include the second aspect, we get to debate how to handle
> programming errors, and maybe whether any of the errors involving a
> QAPISourceInfo.builtin() are*not*  programming errors.
> 
> I'd prefer this series to remain focused on "enabling strict optional
> checking in mypy for everything we have typed so far".

That is still the focus -- I believe adding a poison pill as you suggest 
actually causes more errors instead of "maintaining" them; it doesn't 
maintain a status quo.

Now, I understand the concern that this is opening a chance to allow a 
new class of errors to slip by undetected, but I don't believe it does 
in a meaningful way.

I didn't intend for this info object to be a poison; it wasn't designed 
as one, nor should it be. It's merely a SourceInfo that represents the 
degenerate case that something-or-other does not have an explicit user 
source.

You're right that it probably shouldn't be seen in normal conditions, 
but if it has; my take is that something *else* has already gone 
horribly wrong and we aren't just sweeping dust under the mat, actually.


Here's a recap of my thinking process in identifying the problem and my 
fix for it:

- Many functions expect that QAPISourceInfo *will* be present. Largely, 
they are correct in expecting this, but very few of them check or assert 
this. This is perfectly normal for untyped Python.

- Taxonomically, however, this is untrue, because the built-in entities 
do not need to specify one.

- This can be addressed in typed python by adding assertions everywhere 
("assert info is not None"), or it can be addressed by making the core 
assumption true ("QAPISourceInfo is not Optional").

I opted to make the core assumption true; which does introduce a new 
ambiguity that may not have been present before:

"If we have an info object, is it a real user-source info object, or is 
it the built-in sentinel?"

I *think* this is where you start to get worried; we have "lost" the 
ability to know, in an "obvious" way if this object is "valid" for a 
given context. The assumption is that the code will explode violently if 
we are passed None instead of a valid source info.

I don't think this assumption is true, though. mypy informs me that we 
were never checking to see if it was valid anyway, so we already have 
this ambiguity in the code today. Given that the 'info' object is, in 
most cases, not used until *after we have detected another error*, 
changing the info object to be non-none does not suppress any errors 
that didn't already exist in these contexts.

What about places where QAPISourceInfo is used outside of an 
error-handling context to gain contextual information about an entity? 
Do we open ourselves up to new cases where we may have had an implicit 
error, but now we do not?


Explicit usages of QAPISourceInfo are here:

1. QAPISchemaEntity._set_module(), which uses the info fname to find the 
QAPISchemaModule object. This already checks for "None" info now, and 
will check for the "./builtin" info after the series.

2. QAPISchemaEntity.is_implicit(), which uses the falsey nature of info 
to intuit that it is a built-in definition. No change in behavior.

3. QAPISchemaArrayType.check(), which uses the falsey nature of 
self.info to conditionally pass self.info.defn_meta to 
QAPISchema.resolve_type().

Found a bug (previously fixed in part 6 prior to enabling type checking 
on schema.py, which is why it went undetected here. I am moving the fix 
forward into this series):

info and info.defn_meta will evaluate to the sentinel QAPISourceInfo 
instead of an empty string or None, but it can be changed simply to just 
"info.defn_meta" to take the None value from the builtin source info's 
defn_meta field; easy.

4. QAPISchemaMember.describe(), which uses info.defn_name. This is only 
ever called in error-handling contexts, so we aren't reducing total 
error coverage here, either.

5. QAPISchemaCommand.check(), which uses info.pragma.returns_whitelist. 
This might legitimately create a new avenue where we succeed where we 
used to fail, if we were to create a built-in command. Whether or not 
this is desirable depends on how you want pragma directives to work 
long-term; per-file or per-schema. (Do you want to let user-defined 
schemas dictate how built-in commands work?)

I don't think this is an issue.

6. QAPISchema._def_entity(), which asserts that info must be true-ish, 
or we are in the predefining state. This logic doesn't change.

7. QAPISchema._def_entity(), which uses the true-ish nature of 
other_ent.info to print a different error message. This logic doesn't 
change.

8. expr.py:check_type(), which uses info.pragma.returns_whitelist. This 
theoretically adds slack, but parser.py always gives us real info objects.

9. expr.py:check_exprs(), which calls info.set_defn(meta, name). 
Obviously this must not be None because parser.py does not create 
"built-in" source objects; we know with certainty here that we cannot 
get such a thing.

10. expr.py:check_exprs() again, checking info.pragma.doc_required. As 
above, we know this is always a true source info.

11. QAPIError's str() method itself will call str(info), but if info is 
None, it doesn't crash -- you just get "None". Now you'll get 
"[builtin]". Doesn't permit any new error cases.



Conclusions:

- There are users that use info to decide on behavior outside of an 
error context, but they are already cared for.

- There are some pre-schema.py uses of info that may cause less problems 
than they used to, but would require someone to add the placeholder 
object into parser.py. We can throw a single assertion in expr.py to 
make sure it never happens by accident.

- There are no, and I doubt there ever will be, users who use str(info) 
to decide on behavior. There are only exception handlers that use this 
method. There is no value to making str(info) in particular a poison pill.

- There might be some limited benefit to poisoning info.pragma, depending.


Ultimately, I consider this the standard idiomatic usage of info:

```
def some_func(info):
     if (error):
         raise QAPISemError(info, "you forgot to defrobulate")
```

When we go to print the error message to the user, the exception handler 
is going to fault because str(info) is poisoned. Is it useful to poison 
the exception handler here?

I don't think so; especially considering that even "None" here will work 
just fine!


So I suppose I would prefer not to add poison, because I don't think 
it's necessary, or improves anything materially.

--js

On 12/16/20 4:22 AM, Markus Armbruster wrote:
> 2. On error with "no source info", don't crash.
> 
>     I have my doubts on this one.
> 
>     Such an error means the QAPI code generator screwed up, at least in
>     theory.  Crashing is only proper.  It gets the screwup fixed.
> 

QAPISemError and friends will also halt the generator and don't produce 
output and will fail tests. They aren't less visible or more ignorable 
somehow.

>     In practice, errors due to interactions between built-in stuff and
>     user-defined stuff could conceivably escape testing.  I can't
>     remember such a case offhand.
> 
>     Will the "no source info" error be more useful than a crash?
>     Possibly.  Will it get the screwup fixed?  Maybe not.

I don't understand this; if it's an error -- there's no QAPI, there's no 
QEMU. It's definitely getting fixed.

If QAPISourceInfo is primarily used for printing error information, we 
are already in a situation where the generator is hosed and has wandered 
itself into a problem that can't be ignored.

There's no additional value in having python crash twice per every crash 
because we have bad types in our error reporting functions.

--js

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>> 2. On error with "no source info", don't crash.
>>     I have my doubts on this one.
>>     Such an error means the QAPI code generator screwed up, at least
>> in
>>     theory.  Crashing is only proper.  It gets the screwup fixed.
>> 
>
> QAPISemError and friends will also halt the generator and don't
> produce output and will fail tests. They aren't less visible or more
> ignorable somehow.
>
>>     In practice, errors due to interactions between built-in stuff and
>>     user-defined stuff could conceivably escape testing.  I can't
>>     remember such a case offhand.
>>     Will the "no source info" error be more useful than a crash?
>>     Possibly.  Will it get the screwup fixed?  Maybe not.
>
> I don't understand this; if it's an error -- there's no QAPI, there's
> no QEMU. It's definitely getting fixed.
>
> If QAPISourceInfo is primarily used for printing error information, we
> are already in a situation where the generator is hosed and has
> wandered itself into a problem that can't be ignored.
>
> There's no additional value in having python crash twice per every
> crash because we have bad types in our error reporting functions.

Consider the following scenario.  The average developer knows just
enough about QAPI to be dangerous.  That's okay; if you had to be a QAPI
expert to modify the QAPI schema, we would have failed.  Now meet Joe
Average.  He's a good guy.  Today his job happens to require extending
the QAPI schema.  In a hurry, as usual.  So Joe brings his well-honed
voodoo programming skills to bear, and writes something that looks
plausible to him.  His build fails.  He's not surprised; he's voodoo-
programming after all.  However, the error message is less clear than
usual.  Something about a '[builtin]' file.  There is no '[builtin]'
file.  What to do?  Obvious!  If a bit of voodoo doesn't get you over
the finish line, use more: twiddle the schema until it works.

If his build failed with a Python backtrace instead, Joe would
immediately know that he ran into a bug in our tooling he should report.

Again, I don't mean to criticize Joe.  I've walked in his shoes myself.

On 12/17/20 6:56 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>>> 2. On error with "no source info", don't crash.
>>>      I have my doubts on this one.
>>>      Such an error means the QAPI code generator screwed up, at least
>>> in
>>>      theory.  Crashing is only proper.  It gets the screwup fixed.
>>>
>>
>> QAPISemError and friends will also halt the generator and don't
>> produce output and will fail tests. They aren't less visible or more
>> ignorable somehow.
>>
>>>      In practice, errors due to interactions between built-in stuff and
>>>      user-defined stuff could conceivably escape testing.  I can't
>>>      remember such a case offhand.
>>>      Will the "no source info" error be more useful than a crash?
>>>      Possibly.  Will it get the screwup fixed?  Maybe not.
>>
>> I don't understand this; if it's an error -- there's no QAPI, there's
>> no QEMU. It's definitely getting fixed.
>>
>> If QAPISourceInfo is primarily used for printing error information, we
>> are already in a situation where the generator is hosed and has
>> wandered itself into a problem that can't be ignored.
>>
>> There's no additional value in having python crash twice per every
>> crash because we have bad types in our error reporting functions.
> 
> Consider the following scenario.  The average developer knows just
> enough about QAPI to be dangerous.  That's okay; if you had to be a QAPI
> expert to modify the QAPI schema, we would have failed.  Now meet Joe
> Average.  He's a good guy.  Today his job happens to require extending
> the QAPI schema.  In a hurry, as usual.  So Joe brings his well-honed
> voodoo programming skills to bear, and writes something that looks
> plausible to him.  His build fails.  He's not surprised; he's voodoo-
> programming after all.  However, the error message is less clear than
> usual.  Something about a '[builtin]' file.  There is no '[builtin]'
> file.  What to do?  Obvious!  If a bit of voodoo doesn't get you over
> the finish line, use more: twiddle the schema until it works.
> 
> If his build failed with a Python backtrace instead, Joe would
> immediately know that he ran into a bug in our tooling he should report.
> 
> Again, I don't mean to criticize Joe.  I've walked in his shoes myself.
> 
> 

Worse, Joe was going to see an error about the "None" file or an error 
about the "[builtin]" file.

I understand, though: don't suppress crash messages with opaque crap 
that doesn't help spot the error. It's just that I don't think that's 
what is happening here.

See lengthy response in other email.

--js