Re: [PATCH 02/22] qapi/source: [RFC] add "with_column" contextmanager

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is a silly one, but... it's important to have fun.
>
> This patch isn't *needed*, it's here as an RFC. In trying to experiment
> with different ways to solve the problem addressed by the previous
> commit, I kept getting confused at how the "source location" string with
> line and column number was built across two different classes.
>
> (i.e. QAPISourceError appends the column, but QAPISourceInfo does not
> track column information natively.)
>
> I was afraid to try and fully implement column number directly in
> QAPISourceInfo on the chance that it might have undesirable effects, so
> I came up with a quick "hack" to centralize the 'location' information
> generation.
>
> It's a little goofy, but it works :')
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/error.py  |  8 +++-----
>  scripts/qapi/source.py | 23 ++++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
> index e35e4ddb26a..6b04f56f8a2 100644
> --- a/scripts/qapi/error.py
> +++ b/scripts/qapi/error.py
> @@ -39,11 +39,9 @@ def __init__(self,
>  
>      def __str__(self) -> str:
>          assert self.info is not None
> -        loc = str(self.info)
> -        if self.col is not None:
> -            assert self.info.line is not None
> -            loc += ':%s' % self.col
> -        return loc + ': ' + self.msg
> +        with self.info.at_column(self.col):
> +            loc = str(self.info)
> +        return f"{loc}: {self.msg}"
>  
>  
>  class QAPISemError(QAPISourceError):
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index 1ade864d7b9..21090b9fe78 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -9,8 +9,14 @@
>  # This work is licensed under the terms of the GNU GPL, version 2.
>  # See the COPYING file in the top-level directory.
>  
> +from contextlib import contextmanager
>  import copy
> -from typing import List, Optional, TypeVar
> +from typing import (
> +    Iterator,
> +    List,
> +    Optional,
> +    TypeVar,
> +)
>  
>  
>  class QAPISchemaPragma:
> @@ -35,6 +41,7 @@ def __init__(self, fname: str, line: int,
>                   parent: Optional['QAPISourceInfo']):
>          self.fname = fname
>          self.line = line
> +        self._column: Optional[int] = None
>          self.parent = parent
>          self.pragma: QAPISchemaPragma = (
>              parent.pragma if parent else QAPISchemaPragma()
> @@ -52,9 +59,14 @@ def next_line(self: T) -> T:
>          return info
>  
>      def loc(self) -> str:
> +        # column cannot be provided meaningfully when line is absent.
> +        assert self.line or self._column is None
> +
>          ret = self.fname
>          if self.line is not None:
>              ret += ':%d' % self.line
> +        if self._column is not None:
> +            ret += ':%d' % self._column
>          return ret
>  
>      def in_defn(self) -> str:
> @@ -71,5 +83,14 @@ def include_path(self) -> str:
>              parent = parent.parent
>          return ret
>  
> +    @contextmanager
> +    def at_column(self, column: Optional[int]) -> Iterator[None]:
> +        current_column = self._column
> +        try:
> +            self._column = column
> +            yield
> +        finally:
> +            self._column = current_column
> +
>      def __str__(self) -> str:
>          return self.include_path() + self.in_defn() + self.loc()

Uh...

We create one QAPISourceInfo instance per source line.  First line in
QAPISchemaParser.__init__()

        self.info = QAPISourceInfo(fname, 1, incl_info)

and subsequent ones in .accept()

                self.info = self.info.next_line()

These instances get shared by everything on their line.

Your patch adds a _column attribute to these objects.  Because it
applies to everything that shares the object, setting it is kind of
wrong.  You therefore only ever set it temporarily, in
QAPISourceError.__str__().

This works in the absence of concurrency (which means it just works,
this being Python), but *ugh*!

The obvious extension of QAPISourceInfo to columns would create one
instance per source character.  Too egregiously wasteful for my taste.

Let's start over with the *why*: what is it exactly that bothers you in
the code before this patch?  Is it the spatial distance between the
formatting of "file:line:col: msg" in QAPISourceError.__str_() and the
formatting of the file:line part in QAPISourceInfo.loc()?