Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory

2021-01-13 Thread John Snow

On 1/13/21 7:29 PM, Eduardo Habkost wrote:

On Wed, Jan 13, 2021 at 06:04:29PM -0500, John Snow wrote:

On 1/13/21 11:12 AM, Markus Armbruster wrote:

John Snow  writes:


...



I think we need to, yes; or we probably really, really want to. Making the
info parameter optional really adds a lot of unidiomatic type-checking
confetti when we go to use info, and in many more contexts than just this
sorta-built-in-enum; it will creep badly.


Which methods would require unidiomatic type-checking because of
None/Optional?



Virtually everything that accepts a QAPISourceInfo has to do something 
like this:


def foo(info: Optional[QAPISourceInfo]):
if info is None:
raise Exception("Something very bad has happened!")
...
...


Since the great majority of cases *will* have an info, and we take 
careful pains to make sure this info is preserved, it's less invasive to 
just assert that info isn't Optional.


This is especially true for the QAPISchemaMember initializer, which 
several other classes inherit -- if this is allowed to be 
Optional[QAPISourceInfo], any and all users of a QAPISchemaMember or any 
derived classes will have to check -- on every access -- to see if 
'member.info' is set or not.


Since we expect it to be set 100% of the time for all user-defined code, 
it's a lot less "if member.info is not None" checks everywhere.


Adding a special "built-in" source info object to make this claim helps 
avoid making confusing type signatures for the visitors; i.e.


def visit_thing(info: Optional[QAPISourceInfo]): ...

is misleading, because we actually expect thing to *always* have a 
SourceInfo. To make the documentation be a little more ... statistically 
truthful, it's easier to bend the rules in the other direction and just 
fill in the scant few cases where we don't have a QAPISourceInfo.


--js




Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory

2021-01-13 Thread Eduardo Habkost
On Wed, Jan 13, 2021 at 06:04:29PM -0500, John Snow wrote:
> On 1/13/21 11:12 AM, Markus Armbruster wrote:
> > John Snow  writes:
> > 
> > > Signed-off-by: John Snow 
> > > 
> > > ---
> > > 
> > > The event_enum_members change might become irrelevant after a
> > > forthcoming (?) patch by Markus, but didn't have it in-hand at time of
> > > publishing.
> > 
> > It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
> > which includes parts of your v1.  The parts that are new should be
> > injected into your series so they replace your "[PATCH v2 09/12]
> > qapi/gen: move write method to QAPIGenC, make fname a str".  Holler if
> > you need help.
> > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   scripts/qapi/events.py |  2 +-
> > >   scripts/qapi/schema.py | 25 ++---
> > >   scripts/qapi/types.py  |  9 +
> > >   scripts/qapi/visit.py  |  6 +++---
> > >   4 files changed, 23 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > > index 9851653b9d1..9ba4f109028 100644
> > > --- a/scripts/qapi/events.py
> > > +++ b/scripts/qapi/events.py
> > > @@ -225,7 +225,7 @@ def visit_event(self,
> > > self._event_emit_name))
> > >   # Note: we generate the enum member regardless of @ifcond, to
> > >   # keep the enumeration usable in target-independent code.
> > > -self._event_enum_members.append(QAPISchemaEnumMember(name, None))
> > > +self._event_enum_members.append(QAPISchemaEnumMember(name, info))
> > 
> > This "enum" is not supposed to be erroneous.  If it is, it's a bug.
> > 
> > Your patch changes how the code behaves should such a bug bite here.
> > Before, we crash.  Afterwards, we report the bug using @info, which I'd
> > expect to produce utterly confusing error messages.
> > 
> 
> It doesn't change the behavior *here*, though. It changes it whenever this
> info object is used in another context. ... and who knows when or where or
> why it is being used, or by whom.
> 
> I'll have to play with this. I'm not sure there's any way to coax a bug to
> happen here that I am aware of right away. Can you think of how to will one
> into existence?
> 
> > My comments on PATCH 06 apply: how the code should behave here is a
> > design issue that should be kept out of this patch series.
> > 
> > If you need to pass a value other than None to help with static typing,
> > then pass a suitable poison info that will crash right where None
> > crashes now.

I don't understand what would be the point of inventing something
that behaves exactly like None but makes type checking less
useful.

With None/Optional, mypy gives us a way to be 100% sure the
object isn't going to invalid.  With a poison value, mypy can't
tell us anymore if the code risks crashing at runtime.

> > 
> 
> I think we need to, yes; or we probably really, really want to. Making the
> info parameter optional really adds a lot of unidiomatic type-checking
> confetti when we go to use info, and in many more contexts than just this
> sorta-built-in-enum; it will creep badly.

Which methods would require unidiomatic type-checking because of
None/Optional?

> 
> So, I gotta pass ...something here. but what? You want poison, but I think
> it's not right to fundamentally poison all built-ins.
> 
> Mmm. Maybe per-instance poison can be done? We actually share info
> objects, but I can make poisoned copies. Using next_line() as a basis:
> 
> def poison(self: T) -> T:
> info = copy.copy(self)
> info.poisoned = True
> return info
> 
> probably almost anything I do is not going to make a lot of sense unless I
> can actually replicate and test the different error scenarios to prove that
> we didn't make the error spaghetti unknowably worse. I see it as
> functionally inevitable that I have to audit this and make sure we get good
> error messages anyway, so ... maybe I just ought to do that now anyway.
> 
> > >   def gen_events(schema: QAPISchema,
> > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > > index 720449feee4..0449771dfe5 100644
> > > --- a/scripts/qapi/schema.py
> > > +++ b/scripts/qapi/schema.py
> > > @@ -23,6 +23,7 @@
> > >   from .error import QAPIError, QAPISemError
> > >   from .expr import check_exprs
> > >   from .parser import QAPISchemaParser
> > > +from .source import QAPISourceInfo
> > >   class QAPISchemaEntity:
> > > @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, 
> > > features=None):
> > >   self.name = name
> > >   self._module = None
> > >   # For explicitly defined entities, info points to the (explicit)
> > > -# definition.  For builtins (and their arrays), info is None.
> > > -# For implicitly defined entities, info points to a place that
> > > -# triggered the implicit definition (there may be more than one
> > > -# such place).
> > > + 

Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory

2021-01-13 Thread John Snow

On 1/13/21 11:12 AM, Markus Armbruster wrote:

John Snow  writes:


Signed-off-by: John Snow 

---

The event_enum_members change might become irrelevant after a
forthcoming (?) patch by Markus, but didn't have it in-hand at time of
publishing.


It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
which includes parts of your v1.  The parts that are new should be
injected into your series so they replace your "[PATCH v2 09/12]
qapi/gen: move write method to QAPIGenC, make fname a str".  Holler if
you need help.


Signed-off-by: John Snow 
---
  scripts/qapi/events.py |  2 +-
  scripts/qapi/schema.py | 25 ++---
  scripts/qapi/types.py  |  9 +
  scripts/qapi/visit.py  |  6 +++---
  4 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d1..9ba4f109028 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -225,7 +225,7 @@ def visit_event(self,
self._event_emit_name))
  # Note: we generate the enum member regardless of @ifcond, to
  # keep the enumeration usable in target-independent code.
-self._event_enum_members.append(QAPISchemaEnumMember(name, None))
+self._event_enum_members.append(QAPISchemaEnumMember(name, info))


This "enum" is not supposed to be erroneous.  If it is, it's a bug.

Your patch changes how the code behaves should such a bug bite here.
Before, we crash.  Afterwards, we report the bug using @info, which I'd
expect to produce utterly confusing error messages.



It doesn't change the behavior *here*, though. It changes it whenever 
this info object is used in another context. ... and who knows when or 
where or why it is being used, or by whom.


I'll have to play with this. I'm not sure there's any way to coax a bug 
to happen here that I am aware of right away. Can you think of how to 
will one into existence?



My comments on PATCH 06 apply: how the code should behave here is a
design issue that should be kept out of this patch series.

If you need to pass a value other than None to help with static typing,
then pass a suitable poison info that will crash right where None
crashes now.



I think we need to, yes; or we probably really, really want to. Making 
the info parameter optional really adds a lot of unidiomatic 
type-checking confetti when we go to use info, and in many more contexts 
than just this sorta-built-in-enum; it will creep badly.


So, I gotta pass ...something here. but what? You want poison, but I 
think it's not right to fundamentally poison all built-ins.


Mmm. Maybe per-instance poison can be done? We actually share info 
objects, but I can make poisoned copies. Using next_line() as a basis:


def poison(self: T) -> T:
info = copy.copy(self)
info.poisoned = True
return info

probably almost anything I do is not going to make a lot of sense unless 
I can actually replicate and test the different error scenarios to prove 
that we didn't make the error spaghetti unknowably worse. I see it as 
functionally inevitable that I have to audit this and make sure we get 
good error messages anyway, so ... maybe I just ought to do that now anyway.



  def gen_events(schema: QAPISchema,
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..0449771dfe5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -23,6 +23,7 @@
  from .error import QAPIError, QAPISemError
  from .expr import check_exprs
  from .parser import QAPISchemaParser
+from .source import QAPISourceInfo
  
  
  class QAPISchemaEntity:

@@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, 
features=None):
  self.name = name
  self._module = None
  # For explicitly defined entities, info points to the (explicit)
-# definition.  For builtins (and their arrays), info is None.
-# For implicitly defined entities, info points to a place that
-# triggered the implicit definition (there may be more than one
-# such place).
+# definition. For built-in types (and their arrays), info is a
+# special object that evaluates to False. For implicitly defined
+# entities, info points to a place that triggered the implicit
+# definition (there may be more than one such place).
  self.info = info
  self.doc = doc
  self._ifcond = ifcond or []
@@ -68,7 +69,7 @@ def check_doc(self):
  
  def _set_module(self, schema, info):

  assert self._checked
-self._module = schema.module_by_fname(info and info.fname)
+self._module = schema.module_by_fname(info.fname if info else None)


Looks unrelated.



Hmm, it sorta is. I have apparently edited this patch since I sent it, 
but there was some tomfoolery over how "x and y" statements behave and 
this edit was necessary at the time.


"info and info.fname" 

Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory

2021-01-13 Thread Markus Armbruster
John Snow  writes:

> Signed-off-by: John Snow 
>
> ---
>
> The event_enum_members change might become irrelevant after a
> forthcoming (?) patch by Markus, but didn't have it in-hand at time of
> publishing.

It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
which includes parts of your v1.  The parts that are new should be
injected into your series so they replace your "[PATCH v2 09/12]
qapi/gen: move write method to QAPIGenC, make fname a str".  Holler if
you need help.

> Signed-off-by: John Snow 
> ---
>  scripts/qapi/events.py |  2 +-
>  scripts/qapi/schema.py | 25 ++---
>  scripts/qapi/types.py  |  9 +
>  scripts/qapi/visit.py  |  6 +++---
>  4 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 9851653b9d1..9ba4f109028 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -225,7 +225,7 @@ def visit_event(self,
>self._event_emit_name))
>  # Note: we generate the enum member regardless of @ifcond, to
>  # keep the enumeration usable in target-independent code.
> -self._event_enum_members.append(QAPISchemaEnumMember(name, None))
> +self._event_enum_members.append(QAPISchemaEnumMember(name, info))

This "enum" is not supposed to be erroneous.  If it is, it's a bug.

Your patch changes how the code behaves should such a bug bite here.
Before, we crash.  Afterwards, we report the bug using @info, which I'd
expect to produce utterly confusing error messages.

My comments on PATCH 06 apply: how the code should behave here is a
design issue that should be kept out of this patch series.

If you need to pass a value other than None to help with static typing,
then pass a suitable poison info that will crash right where None
crashes now.

>  def gen_events(schema: QAPISchema,
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee4..0449771dfe5 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -23,6 +23,7 @@
>  from .error import QAPIError, QAPISemError
>  from .expr import check_exprs
>  from .parser import QAPISchemaParser
> +from .source import QAPISourceInfo
>  
>  
>  class QAPISchemaEntity:
> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, 
> features=None):
>  self.name = name
>  self._module = None
>  # For explicitly defined entities, info points to the (explicit)
> -# definition.  For builtins (and their arrays), info is None.
> -# For implicitly defined entities, info points to a place that
> -# triggered the implicit definition (there may be more than one
> -# such place).
> +# definition. For built-in types (and their arrays), info is a
> +# special object that evaluates to False. For implicitly defined
> +# entities, info points to a place that triggered the implicit
> +# definition (there may be more than one such place).
>  self.info = info
>  self.doc = doc
>  self._ifcond = ifcond or []
> @@ -68,7 +69,7 @@ def check_doc(self):
>  
>  def _set_module(self, schema, info):
>  assert self._checked
> -self._module = schema.module_by_fname(info and info.fname)
> +self._module = schema.module_by_fname(info.fname if info else None)

Looks unrelated.

>  self._module.add_entity(self)
>  
>  def set_module(self, schema):
[...]




[PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory

2020-12-16 Thread John Snow
Signed-off-by: John Snow 

---

The event_enum_members change might become irrelevant after a
forthcoming (?) patch by Markus, but didn't have it in-hand at time of
publishing.

Signed-off-by: John Snow 
---
 scripts/qapi/events.py |  2 +-
 scripts/qapi/schema.py | 25 ++---
 scripts/qapi/types.py  |  9 +
 scripts/qapi/visit.py  |  6 +++---
 4 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d1..9ba4f109028 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -225,7 +225,7 @@ def visit_event(self,
   self._event_emit_name))
 # Note: we generate the enum member regardless of @ifcond, to
 # keep the enumeration usable in target-independent code.
-self._event_enum_members.append(QAPISchemaEnumMember(name, None))
+self._event_enum_members.append(QAPISchemaEnumMember(name, info))
 
 
 def gen_events(schema: QAPISchema,
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..0449771dfe5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -23,6 +23,7 @@
 from .error import QAPIError, QAPISemError
 from .expr import check_exprs
 from .parser import QAPISchemaParser
+from .source import QAPISourceInfo
 
 
 class QAPISchemaEntity:
@@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, 
features=None):
 self.name = name
 self._module = None
 # For explicitly defined entities, info points to the (explicit)
-# definition.  For builtins (and their arrays), info is None.
-# For implicitly defined entities, info points to a place that
-# triggered the implicit definition (there may be more than one
-# such place).
+# definition. For built-in types (and their arrays), info is a
+# special object that evaluates to False. For implicitly defined
+# entities, info points to a place that triggered the implicit
+# definition (there may be more than one such place).
 self.info = info
 self.doc = doc
 self._ifcond = ifcond or []
@@ -68,7 +69,7 @@ def check_doc(self):
 
 def _set_module(self, schema, info):
 assert self._checked
-self._module = schema.module_by_fname(info and info.fname)
+self._module = schema.module_by_fname(info.fname if info else None)
 self._module.add_entity(self)
 
 def set_module(self, schema):
@@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
 meta = 'built-in'
 
 def __init__(self, name, json_type, c_type):
-super().__init__(name, None, None)
+super().__init__(name, QAPISourceInfo.builtin(), None)
 assert not c_type or isinstance(c_type, str)
 assert json_type in ('string', 'number', 'int', 'boolean', 'null',
  'value')
@@ -897,7 +898,7 @@ def _def_builtin_type(self, name, json_type, c_type):
 # be nice, but we can't as long as their generated code
 # (qapi-builtin-types.[ch]) may be shared by some other
 # schema.
-self._make_array_type(name, None)
+self._make_array_type(name, QAPISourceInfo.builtin())
 
 def _def_predefineds(self):
 for t in [('str','string',  'char' + POINTER_SUFFIX),
@@ -917,16 +918,18 @@ def _def_predefineds(self):
   ('null',   'null','QNull' + POINTER_SUFFIX)]:
 self._def_builtin_type(*t)
 self.the_empty_object_type = QAPISchemaObjectType(
-'q_empty', None, None, None, None, None, [], None)
+'q_empty', QAPISourceInfo.builtin(),
+None, None, None, None, [], None)
 self._def_entity(self.the_empty_object_type)
 
 qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
   'qbool']
 qtype_values = self._make_enum_members(
-[{'name': n} for n in qtypes], None)
+[{'name': n} for n in qtypes], QAPISourceInfo.builtin())
 
-self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
-qtype_values, 'QTYPE'))
+self._def_entity(QAPISchemaEnumType(
+'QType', QAPISourceInfo.builtin(), None,
+None, None, qtype_values, 'QTYPE'))
 
 def _make_features(self, features, info):
 if features is None:
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa1..a3a16284006 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -71,7 +71,8 @@ def gen_enum(name: str,
  members: List[QAPISchemaEnumMember],
  prefix: Optional[str] = None) -> str:
 # append automatically generated _MAX value
-enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
+enum_members = members + [
+QAPISchemaEnumMember('_MAX', QAPISourceInfo.builtin())]
 
 ret = mcgen('''
 
@@