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

[John Snow <jsnow@redhat.com>]

On 12/16/20 3:23 AM, Markus Armbruster wrote:
> 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.
> 

OK, no problem.