Re: [PATCH 03/12] qapi/main: handle theoretical None-return from re.match()

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Mypy cannot understand that this match can never be None, so help it
> along.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/main.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 42517210b805..271d9e84da94 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -23,7 +23,8 @@
>  
>  def invalid_prefix_char(prefix: str) -> Optional[str]:
>      match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)

@match can't be None because the regexp always matches a prefix,
possibly the empty prefix.

> -    if match.end() != len(prefix):
> +    # match cannot be None, but mypy cannot infer that.
> +    if match and match.end() != len(prefix):
>          return prefix[match.end()]
>      return None

High-level logic:

       if there is an invalid prefix character:
           return it
       return None

The actual code takes the return None when the match fails.  If this
could happen, it would be wrong.  I can't, so it doesn't matter, but I
dislike it anyway.

Alternative 1: turn "match cannot be None" from comment to code

       match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
  +    assert match
       if match.end() != len(prefix):
           return prefix[match.end()]
       return None

Alternative 2: turn empty prefix into a special case

  -    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
  +    match = re.match(r'[A-Za-z_.-][A-Za-z0-9_.-]*', prefix)
  +    if not match:
  +        return prefix[0]
       if match.end() != len(prefix):
           return prefix[match.end()]
       return None

I'd prefer alternative 1.