[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
From: |
John Snow |
Subject: |
Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel |
Date: |
Fri, 18 Dec 2020 14:14:23 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 |
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
- Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond, (continued)
[PATCH 05/12] qapi/gen: use './builtin' for the built-in module name, John Snow, 2020/12/14
[PATCH 06/12] qapi/source: Add builtin null-object sentinel, John Snow, 2020/12/14
Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel, John Snow, 2020/12/16
Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel, Markus Armbruster, 2020/12/17
Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel, John Snow, 2020/12/18
[PATCH 07/12] qapi/gen: write _genc/_genh access shims, John Snow, 2020/12/14
[PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str, John Snow, 2020/12/14
[PATCH 10/12] tests/qapi-schema: Add quotes to module name in test output, John Snow, 2020/12/14
[PATCH 11/12] qapi/schema: Name the builtin module "" instead of None, John Snow, 2020/12/14