Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-104050: Run mypy on clinic.py in CI #104421

Merged
merged 16 commits into from
May 15, 2023
Merged
7 changes: 7 additions & 0 deletions .github/dependabot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,10 @@ updates:
update-types:
- "version-update:semver-minor"
- "version-update:semver-patch"
- package-ecosystem: "pip"
directory: "/Tools/clinic/"
schedule:
interval: "monthly"
labels:
- "skip issue"
- "skip news"
39 changes: 39 additions & 0 deletions .github/workflows/mypy.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Workflow to run mypy on select parts of the CPython repo
name: mypy

on:
push:
branches:
- main
pull_request:
paths:
- "Tools/clinic/**"
- ".github/workflows/mypy.yml"
workflow_dispatch:

permissions:
contents: read

env:
PIP_DISABLE_PIP_VERSION_CHECK: 1
FORCE_COLOR: 1
TERM: xterm-256color # needed for FORCE_COLOR to work on mypy on Ubuntu, see https://github.com/python/mypy/issues/13817

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
cancel-in-progress: true

jobs:
mypy:
name: Run mypy on Tools/clinic/
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
with:
python-version: "3.x"
cache: pip
cache-dependency-path: Tools/clinic/requirements-dev.txt
- run: pip install -r Tools/clinic/requirements-dev.txt
- run: mypy --config-file Tools/clinic/mypy.ini
40 changes: 26 additions & 14 deletions Tools/clinic/clinic.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import abc
import ast
import builtins as bltns
import collections
import contextlib
import copy
Expand All @@ -26,7 +27,9 @@
import traceback
import types

from collections.abc import Callable
from types import *
hugovk marked this conversation as resolved.
Show resolved Hide resolved
from typing import Any, NamedTuple

# TODO:
#
Expand Down Expand Up @@ -78,19 +81,26 @@ def __repr__(self):

sig_end_marker = '--'

Appender = Callable[[str], None]
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
Outputter = Callable[[None], str]

_text_accumulator_nt = collections.namedtuple("_text_accumulator", "text append output")
class _TextAccumulator(NamedTuple):
text: list[str]
append: Appender
output: Outputter

def _text_accumulator():
text = []
def output():
s = ''.join(text)
text.clear()
return s
return _text_accumulator_nt(text, text.append, output)
return _TextAccumulator(text, text.append, output)


text_accumulator_nt = collections.namedtuple("text_accumulator", "text append")
class TextAccumulator(NamedTuple):
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
text: list[str]
append: Appender

def text_accumulator():
"""
Expand All @@ -104,7 +114,7 @@ def text_accumulator():
empties the accumulator.
"""
text, append, output = _text_accumulator()
return text_accumulator_nt(append, output)
return TextAccumulator(append, output)


def warn_or_fail(fail=False, *args, filename=None, line_number=None):
Expand Down Expand Up @@ -1925,8 +1935,10 @@ def dump(self):
# maps strings to Language objects.
# "languages" maps the name of the language ("C", "Python").
# "extensions" maps the file extension ("c", "py").
LangDict = dict[str, Callable[[str], Language]]

languages = { 'C': CLanguage, 'Python': PythonLanguage }
extensions = { name: CLanguage for name in "c cc cpp cxx h hh hpp hxx".split() }
extensions: LangDict = { name: CLanguage for name in "c cc cpp cxx h hh hpp hxx".split() }
extensions['py'] = PythonLanguage


Expand Down Expand Up @@ -2558,15 +2570,15 @@ class CConverter(metaclass=CConverterAutoRegister):
"""

# The C name to use for this variable.
name = None
name: str | None = None
Copy link
Contributor

@erlend-aasland erlend-aasland May 16, 2023

Choose a reason for hiding this comment

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

So, these str | None are now starting to cause frustrations, as I gradually add typing to other parts of clinic.py. Since there are no guards anywhere, I obviously get errors and warnings about str plus None, etc. We could change the defaults to the empty string, but that can also create a lot of havoc.


# The Python name to use for this variable.
py_name = None
py_name: str | None = None

# The C type to use for this variable.
# 'type' should be a Python string specifying the type, e.g. "int".
# If this is a pointer type, the type string should end with ' *'.
type = None
type: str | None = None

# The Python default value for this parameter, as a Python value.
# Or the magic value "unspecified" if there is no default.
Expand All @@ -2577,15 +2589,15 @@ class CConverter(metaclass=CConverterAutoRegister):

# If not None, default must be isinstance() of this type.
# (You can also specify a tuple of types.)
default_type = None
default_type: bltns.type[Any] | tuple[bltns.type[Any], ...] | None = None

# "default" converted into a C value, as a string.
# Or None if there is no default.
c_default = None
c_default: str | None = None

# "default" converted into a Python value, as a string.
# Or None if there is no default.
py_default = None
py_default: str | None = None

# The default value used to initialize the C variable when
# there is no default, but not specifying a default may
Expand All @@ -2597,14 +2609,14 @@ class CConverter(metaclass=CConverterAutoRegister):
#
# This value is specified as a string.
# Every non-abstract subclass should supply a valid value.
c_ignored_default = 'NULL'
c_ignored_default: str = 'NULL'

# If true, wrap with Py_UNUSED.
unused = False

# The C converter *function* to be used, if any.
# (If this is not None, format_unit must be 'O&'.)
converter = None
converter: str | None = None

# Should Argument Clinic add a '&' before the name of
# the variable when passing it into the _impl function?
Expand Down Expand Up @@ -3432,7 +3444,7 @@ class robuffer: pass
def str_converter_key(types, encoding, zeroes):
return (frozenset(types), bool(encoding), bool(zeroes))

str_converter_argument_map = {}
str_converter_argument_map: dict[str, str] = {}

class str_converter(CConverter):
type = 'const char *'
Expand Down
26 changes: 16 additions & 10 deletions Tools/clinic/cpp.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import re
import sys
from collections.abc import Callable

def negate(condition):

TokenAndCondition = tuple[str, str]
TokenStack = list[TokenAndCondition]

def negate(condition: str) -> str:
"""
Returns a CPP conditional that is the opposite of the conditional passed in.
"""
Expand All @@ -22,28 +27,29 @@ class Monitor:
Anyway this implementation seems to work well enough for the CPython sources.
"""

is_a_simple_defined: Callable[[str], re.Match[str] | None]
is_a_simple_defined = re.compile(r'^defined\s*\(\s*[A-Za-z0-9_]+\s*\)$').match

def __init__(self, filename=None, *, verbose=False):
self.stack = []
def __init__(self, filename=None, *, verbose: bool = False):
self.stack: TokenStack = []
self.in_comment = False
self.continuation = None
self.continuation: str | None = None
self.line_number = 0
self.filename = filename
self.verbose = verbose

def __repr__(self):
def __repr__(self) -> str:
return ''.join((
'<Monitor ',
str(id(self)),
" line=", str(self.line_number),
" condition=", repr(self.condition()),
">"))

def status(self):
def status(self) -> str:
return str(self.line_number).rjust(4) + ": " + self.condition()

def condition(self):
def condition(self) -> str:
"""
Returns the current preprocessor state, as a single #if condition.
"""
Expand All @@ -62,15 +68,15 @@ def close(self):
if self.stack:
self.fail("Ended file while still in a preprocessor conditional block!")

def write(self, s):
def write(self, s: str) -> None:
for line in s.split("\n"):
self.writeline(line)

def writeline(self, line):
def writeline(self, line: str) -> None:
self.line_number += 1
line = line.strip()

def pop_stack():
def pop_stack() -> TokenAndCondition:
if not self.stack:
self.fail("#" + token + " without matching #if / #ifdef / #ifndef!")
return self.stack.pop()
Expand Down
11 changes: 11 additions & 0 deletions Tools/clinic/mypy.ini
Copy link
Member Author

Choose a reason for hiding this comment

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

I usually put my mypy config in a pyproject.toml file these days, but a mypy.ini file feels like it makes more "sense" unless we're planning on adding other linters/formatters to check clinic.py in the future

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[mypy]
# make sure clinic can still be run on Python 3.10
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
python_version = 3.10
pretty = True
enable_error_code = ignore-without-code
disallow_any_generics = True
strict_concatenate = True
warn_redundant_casts = True
warn_unused_ignores = True
warn_unused_configs = True
files = Tools/clinic/
2 changes: 2 additions & 0 deletions Tools/clinic/requirements-dev.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Requirements file for external linters and checks we run on Tools/clinic/ in CI
mypy==1.2.0
Copy link
Member Author

Choose a reason for hiding this comment

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

We're deliberately pinning to an older version here, in order to test that dependabot updates are working on this file, as per @hugovk's suggestion in #104421 (comment)