Movatterモバイル変換

Add type annotations to clinic.py#104050

Closed

erlend-aasland added the skip news label

Copy link

ContributorAuthor

erlend-aasland commentedJul 3, 2023•
edited
Loading

My basal typing skills are no longer sufficient; I need help! 😱 cc.@AlexWaygood

Tools/clinic/clinic.py:4593: error: Incompatible types in assignment (expression has type "str | None", variable has type "str")  [assignment]Tools/clinic/clinic.py:4617: error: Keywords must be strings  [misc]Tools/clinic/clinic.py:4645: error: Incompatible types in assignment (expression has type "Function", variable has type "None")  [assignment]Tools/clinic/clinic.py:4646: error: Argument "return_converter" to "Function" has incompatible type "CReturnConverter"; expected "Callable[..., CReturnConverter]"  [arg-type]Tools/clinic/clinic.py:4654: error: "None" has no attribute "self_converter"  [attr-defined]Tools/clinic/clinic.py:4654: error: Keywords must be strings  [misc]Tools/clinic/clinic.py:4655: error: Argument 1 to "Parameter" has incompatible type "str | None"; expected "str"  [arg-type]Tools/clinic/clinic.py:4655: error: Argument 2 to "Parameter" has incompatible type "Literal[_ParameterKind.POSITIONAL_ONLY]"; expected "str"  [arg-type]Tools/clinic/clinic.py:4655: error: Argument "function" to "Parameter" has incompatible type "None"; expected "Function"  [arg-type]Tools/clinic/clinic.py:4656: error: "None" has no attribute "parameters"  [attr-defined]Tools/clinic/clinic.py:4659: error: Argument 1 to "next" of "DSLParser" has incompatible type "Callable[[str], None]"; expected "Callable[[str | None], None]"  [arg-type]Found 11 errors in 1 file (checked 2 source files)

These ghosts are now haunting me:

Line 4314 ind65b783

self.function=None

I'm tempted to use aFunction sentinel value, or just rewrite this passage. Perhaps there is a more idiomatic way in the typing world.

Lines 4591 to 4593 ind65b783

	full_name,_,c_basename=line.partition(' as ')
	full_name=full_name.strip()
	c_basename=c_basename.strip()orNone

Unless there's a more typing idiomatic way, I'm inclined to simply do something like this:

diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.pyindex b0e5142efa..c730688fbf 100755--- a/Tools/clinic/clinic.py+++ b/Tools/clinic/clinic.py@@ -4588,9 +4588,9 @@ def state_modulename_name(self, line: str | None) -> None:          line, _, returns = line.partition('->')-        full_name, _, c_basename = line.partition(' as ')-        full_name = full_name.strip()-        c_basename = c_basename.strip() or None+        left, _, right = line.partition(' as ')+        full_name = left.strip()+        c_basename = right.strip() or None          if not is_legal_py_identifier(full_name):             fail("Illegal function name:", full_name)

erlend-aasland added3 commits

July 3, 2023 09:35

Fix shadowing post partition

4744bd9

Tighten up parse_converter() return annotation

7df949b

Loosen up state_parameters_start()

7667704

AlexWaygood reviewed

Tools/clinic/clinic.py OutdatedShow resolvedHide resolved

erlend-aasland added6 commits

July 3, 2023 09:48

ast.expr is not str

0480ed2

Annotate Parameter 'kind' param as inspect._ParameterKind

ec7c1d8

Annotate DSLParser 'function' attr as 'Function | None'

ec38c62

Make sure Parameter() gets name as str

b1bd72a

Function param self_converter is CConverter or None

3f1a0c0

Make sure str is used as key in parameter dict

d694f04

AlexWaygood reviewed

Tools/clinic/clinic.py OutdatedShow resolvedHide resolved

AlexWaygood reviewed

Tools/clinic/clinic.py OutdatedShow resolvedHide resolved

Copy link

Member

AlexWaygood commentedJul 3, 2023

Tools/clinic/clinic.py:4593: error: Incompatible types in assignment(expression has type "str | None", variable has type "str")  [assignment]            c_basename = right.strip() or None

This error is because the code does something equivalent to this:

condition:boolifcondition:foo="foo"else:foo=None

Iff you don't have a type annotation for a variable, then a type checker will by default infer the most specific type possible for an assignment. That means that in the first branch, mypy infers that the type offoo isstr, and in the second branch, it infers that the type offoo isNone. It then realises that it's inferring two different types for the two different branches, and squawks at you, as in any given function, any given variable should have the same type across the whole function.

The solution is to give an explicit type annotation before the variable has been assigned in either branch, to force mypy to infer a broader type in both branches instead of inferring the most specific type possible:

condition:boolfoo:str|Noneifcondition:foo="foo"else:foo=None

Copy link

Member

AlexWaygood commentedJul 3, 2023

Tools/clinic/clinic.py:5039: error: Incompatible return value type (got"tuple[str, bool, dict[str | None, Any]]", expected"tuple[str, bool, dict[str, Any]]")  [return-value]                    return name, False, kwargs                           ^~~~~~~~~~~~~~~~~~~

I think mypy is flagging a real bug in the code here. The issue is this block of code here:

Lines 5033 to 5039 ind694f04

	caseast.Call(func=ast.Name(name)):
	symbols=globals()
	kwargs= {
	node.arg:eval_ast_expr(node.value,symbols)
	fornodeinannotation.keywords
	}
	returnname,False,kwargs

The function is annotated as returntuple[str, bool, KwargDict], andKwargDict is a type alias fordict[str, Any]. It's important that the dictionary that's the third element of the tuple only has strings as keys. If it doesn't, then this will fail:

Line 4617 ind694f04

return_converter=return_converters[name](**kwargs)

The code as written, however, doesn't guarantee that all the keys in thekwargs dictionary will be strings. In the dictionary comprehension, we can see thatannotation is anast.Call instance. That means thatannotation.keywords is of typelist[ast.keyword] -- we can see this from typeshed's stubs for theast module (which are an invaluable reference if you're working with ASTs in Python!):https://github.com/python/typeshed/blob/18d45d62aabe68fce78965c4920cbdeddb4b54db/stdlib/_ast.pyi#L324-L329. Ifannotation.keywords is of typelist[ast.keyword], that means that thenode variable in the dictionary comprehension is of typekeyword, which means (again using typeshed'sast stubs), thatnode.arg is of typestr | None:https://github.com/python/typeshed/blob/18d45d62aabe68fce78965c4920cbdeddb4b54db/stdlib/_ast.pyi#L516-L520. AKA, the keys in this dictionary are not always guaranteed to be strings -- there's a latent bug in this code!

Merge branch 'main' into clinic/even-more-typing

f116de8

Copy link

Member

AlexWaygood commentedJul 3, 2023•
edited
Loading

AKA, the keys in this dictionary are not always guaranteed to be strings -- there's a latent bug in this code!

Here's two possible ways of fixing this latent bug:

(1)

--- a/Tools/clinic/clinic.py+++ b/Tools/clinic/clinic.py@@ -5048,6 +5048,7 @@ def parse_converter(annotation: ast.expr | None) -> tuple[str, bool, KwargDict]:                 kwargs = {                     node.arg: eval_ast_expr(node.value, symbols)                     for node in annotation.keywords+                    if isinstance(node.arg, str)                 }                 return name, False, kwargs             case _:

(2)

--- a/Tools/clinic/clinic.py+++ b/Tools/clinic/clinic.py@@ -5045,10 +5045,11 @@ def parse_converter(annotation: ast.expr | None) -> tuple[str, bool, KwargDict]:                 return name, False, {}             case ast.Call(func=ast.Name(name)):                 symbols = globals()-                kwargs = {-                    node.arg: eval_ast_expr(node.value, symbols)-                    for node in annotation.keywords-                }+                kwargs: dict[str, Any] = {}+                for node in annotation.keywords:+                    if not isinstance(node.arg, str):+                        raise TypeError("Unexpectedly found a keyword node with a non-str arg!")+                    kwargs[node.arg] = eval_ast_expr(node.value, symbols)                 return name, False, kwargs             case _:                 fail(

You tell me which would be more correct here (or if another solution would be better than either of these)!

Copy link

Member

AlexWaygood commentedJul 3, 2023

Tools/clinic/clinic.py:4659: error: Argument "return_converter" to "Function"has incompatible type "CReturnConverter"; expected"Callable[..., CReturnConverter]"  [arg-type]    ...                                return_converter=return_converter, kin...                                                        ^~~~~~~~~~~~~~~~

It looks to me like the annotation for thereturn_converter attribute inFunction.__init__ is just incorrect. The following change should fix this error:

--- a/Tools/clinic/clinic.py+++ b/Tools/clinic/clinic.py@@ -2448,7 +2448,7 @@ def __init__(             cls: Class | None = None,             c_basename: str | None = None,             full_name: str | None = None,-            return_converter: ReturnConverterType,+            return_converter: "CReturnConverter",             return_annotation = inspect.Signature.empty,             docstring: str | None = None,             kind: str = CALLABLE,

erlend-aasland added2 commits

July 3, 2023 13:07

Annotation for c_basename

e587ba4

More accurate type for self.self_converter

cd5d675

Copy link

ContributorAuthor

erlend-aasland commentedJul 3, 2023•
edited
Loading

[...] there's a latent bug in this code!

Good sleuthing! Let's fix bugs separately from adding annotations. I'll create an issue.

EDIT: Createdgh-106359

erlend-aasland mentioned this pull request

Argument Clinic: DSLParser.parse_converter() can return an incorrect kwargs dict#106359

Closed

erlend-aasland added4 commits

July 3, 2023 13:17

Address review: fix return_converter type

f25dd99

from __future__ imports must occur at the beginning of the file!

dcd5d32

Pull in main

4c3844e

Pull in main

89ba85a

erlend-aasland marked this pull request as ready for review

July 3, 2023 13:18

bedevere-bot added the awaiting core review label

Wrap touched lines at 79 chars

140bafc

AlexWaygood reviewed

Copy link

Member

AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good, just two questions

Tools/clinic/clinic.py OutdatedShow resolvedHide resolved

Apply suggestions from code review

4a7edf9

AlexWaygood approved these changes

Copy link

Member

AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM!

bedevere-bot added awaiting merge and removed awaiting core review labels

erlend-aaslandenabled auto-merge (squash)

July 3, 2023 13:58

erlend-aasland merged commit2e2daac intopython:main

bedevere-bot removed the awaiting merge label

carljm added a commit to carljm/cpython that referenced this pull request