Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[John Snow <jsnow@redhat.com>]

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