GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-104050: Add more type hints to Argument Clinic DSLParser() by erlend-aasland · Pull Request #106354 · python/cpython

Conversation

erlend-aasland commented Jul 3, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Add basic annotations to state_modulename_name()

Issue: Add type annotations to clinic.py #104050

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-104050: Add more type hints to Argument Clinic DSLParser()

15d4b03

Add basic annotations to state_modulename_name()

erlend-aasland requested a review from AlexWaygood

July 3, 2023 07:24

bedevere-bot mentioned this pull request

Jul 3, 2023

Add type annotations to clinic.py #104050

Closed

erlend-aasland added the skip news label

Jul 3, 2023

erlend-aasland commented Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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:

cpython/Tools/clinic/clinic.py

Line 4314 in d65b783

self.function = None

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

cpython/Tools/clinic/clinic.py

Lines 4591 to 4593 in d65b783

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

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

Details

diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py
index 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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 3 commits

July 3, 2023 09:35


          Fix shadowing post partition

4744bd9


          Tighten up parse_converter() return annotation

7df949b


          Loosen up state_parameters_start()

AlexWaygood reviewed

Jul 3, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 6 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

Jul 3, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood reviewed

Jul 3, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood commented Jul 3, 2023

Copy link

Copy Markdown

Member

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: bool

if condition:
    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 of foo is str, and in the second branch, it infers that the type of foo is None. 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: bool
foo: str | None

if condition:
    foo = "foo"
else:
    foo = None

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood commented Jul 3, 2023

Copy link

Copy Markdown

Member

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:

cpython/Tools/clinic/clinic.py

Lines 5033 to 5039 in d694f04

	case ast.Call(func=ast.Name(name)):
	symbols = globals()
	kwargs = {
	node.arg: eval_ast_expr(node.value, symbols)
	for node in annotation.keywords
	}
	return name, False, kwargs

The function is annotated as return tuple[str, bool, KwargDict], and KwargDict is a type alias for dict[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:

cpython/Tools/clinic/clinic.py

Line 4617 in d694f04

return_converter = return_converters[name](**kwargs)

The code as written, however, doesn't guarantee that all the keys in the kwargs dictionary will be strings. In the dictionary comprehension, we can see that annotation is an ast.Call instance. That means that annotation.keywords is of type list[ast.keyword] -- we can see this from typeshed's stubs for the ast 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. If annotation.keywords is of type list[ast.keyword], that means that the node variable in the dictionary comprehension is of type keyword, which means (again using typeshed's ast stubs), that node.arg is of type str | 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!

erlend-aasland reacted with rocket emoji

All reactions

🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


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

f116de8

AlexWaygood commented Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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)!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood commented Jul 3, 2023

Copy link

Copy Markdown

Member

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 the return_converter attribute in Function.__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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 2 commits

July 3, 2023 13:07


          Annotation for c_basename

e587ba4


          More accurate type for self.self_converter

cd5d675

erlend-aasland commented Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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

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

EDIT: Created gh-106359

AlexWaygood reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland mentioned this pull request

Jul 3, 2023

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

Closed

erlend-aasland added 4 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

Jul 3, 2023


          Wrap touched lines at 79 chars

140bafc

AlexWaygood reviewed

Jul 3, 2023

View reviewed changes

AlexWaygood left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Apply suggestions from code review

4a7edf9

AlexWaygood approved these changes

Jul 3, 2023

View reviewed changes

AlexWaygood left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji

All reactions

🎉 1 reaction

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

Jul 3, 2023

erlend-aasland enabled auto-merge (squash)

July 3, 2023 13:58

erlend-aasland merged commit 2e2daac into python:main

Jul 3, 2023

bedevere-bot removed the awaiting merge label

Jul 3, 2023

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

Jul 3, 2023


          Merge branch 'main' into modulelocals

ac11740

* main: (167 commits)
  pythongh-91053: make func watcher tests resilient to other func watchers (python#106286)
  pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106354)
  pythongh-106359: Fix corner case bugs in Argument Clinic converter parser (python#106361)
  pythongh-104146: Remove unused attr 'parameter_indent' from clinic.DLParser (python#106358)
  pythongh-106320: Remove private _PyErr C API functions (python#106356)
  pythongh-104050: Annotate Argument Clinic DSLParser attributes (python#106357)
  pythongh-106320: Create pycore_modsupport.h header file (python#106355)
  pythongh-106320: Move _PyUnicodeWriter to the internal C API (python#106342)
  pythongh-61215: New mock to wait for multi-threaded events to happen (python#16094)
  Document PYTHONSAFEPATH along side -P (python#106122)
  Replace the esoteric term 'datum' when describing dict comprehensions (python#106119)
  pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106343)
  pythongh-106320: _testcapi avoids private _PyUnicode_EqualToASCIIString() (python#106341)
  pythongh-106320: Add pycore_complexobject.h header file (python#106339)
  pythongh-106078: Move DecimalException to _decimal state (python#106301)
  pythongh-106320: Use _PyInterpreterState_GET() (python#106336)
  pythongh-106320: Remove private _PyInterpreterState functions (python#106335)
  pythongh-104922: Doc: add note about PY_SSIZE_T_CLEAN (python#106314)
  pythongh-106217: Truncate the issue body size of `new-bugs-announce-notifier` (python#106329)
  pythongh-104922: remove PY_SSIZE_T_CLEAN (python#106315)
  ...

erlend-aasland deleted the clinic/even-more-typing branch

July 4, 2023 22:45

Reviewers

AlexWaygood

AlexWaygood approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

erlend-aasland commented Jul 3, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood commented Jul 3, 2023

Uh oh!

AlexWaygood commented Jul 3, 2023

Uh oh!

AlexWaygood commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood commented Jul 3, 2023

Uh oh!

erlend-aasland commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erlend-aasland commented Jul 3, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood commented Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.