-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-141004: Add a CI job ensuring that new C APIs include documentation #142102
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
Open
ZeroIntensity
wants to merge
12
commits into
python:main
Choose a base branch
from
ZeroIntensity:c-api-docs-check
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+304
−0
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
77d1750
Add a tool for finding undocumented C API
ZeroIntensity e219136
Add a CI job.
ZeroIntensity 8c88402
General improvements.
ZeroIntensity 0f687cf
Add myself as a codeowner.
ZeroIntensity ec03845
Run black.
ZeroIntensity 2510476
Eliminate some duplication.
ZeroIntensity 51b2747
Apply suggestions from code review
ZeroIntensity cb9a68d
Fix alphabetical order.
ZeroIntensity 681962d
Change plurality in the message depending on the size of the list.
ZeroIntensity 1d0c8f6
Fix indentation on the output.
ZeroIntensity f487d6e
Update .github/workflows/build.yml
ZeroIntensity 02d2f07
Update Tools/check-c-api-docs/main.py
ZeroIntensity File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| # descrobject.h | ||
| PyClassMethodDescr_Type | ||
| PyDictProxy_Type | ||
| PyGetSetDescr_Type | ||
| PyMemberDescr_Type | ||
| PyMethodDescr_Type | ||
| PyWrapperDescr_Type | ||
| # pydtrace_probes.h | ||
| PyDTrace_AUDIT | ||
| PyDTrace_FUNCTION_ENTRY | ||
| PyDTrace_FUNCTION_RETURN | ||
| PyDTrace_GC_DONE | ||
| PyDTrace_GC_START | ||
| PyDTrace_IMPORT_FIND_LOAD_DONE | ||
| PyDTrace_IMPORT_FIND_LOAD_START | ||
| PyDTrace_INSTANCE_DELETE_DONE | ||
| PyDTrace_INSTANCE_DELETE_START | ||
| PyDTrace_INSTANCE_NEW_DONE | ||
| PyDTrace_INSTANCE_NEW_START | ||
| PyDTrace_LINE | ||
| # fileobject.h | ||
| Py_FileSystemDefaultEncodeErrors | ||
| Py_FileSystemDefaultEncoding | ||
| Py_HasFileSystemDefaultEncoding | ||
| Py_UTF8Mode | ||
| # pyhash.h | ||
| Py_HASH_EXTERNAL | ||
| # exports.h | ||
| PyAPI_DATA | ||
| Py_EXPORTED_SYMBOL | ||
| Py_IMPORTED_SYMBOL | ||
| Py_LOCAL_SYMBOL | ||
| # modsupport.h | ||
| PyABIInfo_FREETHREADING_AGNOSTIC | ||
| # moduleobject.h | ||
| PyModuleDef_Type | ||
| # object.h | ||
| Py_INVALID_SIZE | ||
| Py_TPFLAGS_HAVE_VERSION_TAG | ||
| Py_TPFLAGS_INLINE_VALUES | ||
| Py_TPFLAGS_IS_ABSTRACT | ||
| # pyexpat.h | ||
| PyExpat_CAPI_MAGIC | ||
| PyExpat_CAPSULE_NAME | ||
| # pyport.h | ||
| Py_ALIGNED | ||
| Py_ARITHMETIC_RIGHT_SHIFT | ||
| Py_CAN_START_THREADS | ||
| Py_FORCE_EXPANSION | ||
| Py_GCC_ATTRIBUTE | ||
| Py_LL | ||
| Py_SAFE_DOWNCAST | ||
| Py_ULL | ||
| Py_VA_COPY | ||
| # unicodeobject.h | ||
| Py_UNICODE_SIZE | ||
| # cpython/methodobject.h | ||
| PyCFunction_GET_CLASS | ||
| # cpython/compile.h | ||
| PyCF_ALLOW_INCOMPLETE_INPUT | ||
| PyCF_COMPILE_MASK | ||
| PyCF_DONT_IMPLY_DEDENT | ||
| PyCF_IGNORE_COOKIE | ||
| PyCF_MASK | ||
| PyCF_MASK_OBSOLETE | ||
| PyCF_SOURCE_IS_UTF8 | ||
| # cpython/descrobject.h | ||
| PyDescr_COMMON | ||
| PyDescr_NAME | ||
| PyDescr_TYPE | ||
| PyWrapperFlag_KEYWORDS | ||
| # cpython/fileobject.h | ||
| PyFile_NewStdPrinter | ||
| PyStdPrinter_Type | ||
| Py_UniversalNewlineFgets | ||
| # cpython/setobject.h | ||
| PySet_MINSIZE | ||
| # cpython/ceval.h | ||
| PyUnstable_CopyPerfMapFile | ||
| PyUnstable_PerfTrampoline_CompileCode | ||
| PyUnstable_PerfTrampoline_SetPersistAfterFork | ||
| # cpython/genobject.h | ||
| PyAsyncGenASend_CheckExact | ||
| # cpython/longintrepr.h | ||
| PyLong_BASE | ||
| PyLong_MASK | ||
| PyLong_SHIFT | ||
| # cpython/pyerrors.h | ||
| PyException_HEAD | ||
| # cpython/pyframe.h | ||
| PyUnstable_EXECUTABLE_KINDS | ||
| PyUnstable_EXECUTABLE_KIND_BUILTIN_FUNCTION | ||
| PyUnstable_EXECUTABLE_KIND_METHOD_DESCRIPTOR | ||
| PyUnstable_EXECUTABLE_KIND_PY_FUNCTION | ||
| PyUnstable_EXECUTABLE_KIND_SKIP | ||
| # cpython/pylifecycle.h | ||
| Py_FrozenMain | ||
| # cpython/unicodeobject.h | ||
| PyUnicode_IS_COMPACT | ||
| PyUnicode_IS_COMPACT_ASCII |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| import re | ||
| from pathlib import Path | ||
| import sys | ||
| import _colorize | ||
| import textwrap | ||
|
|
||
| SIMPLE_FUNCTION_REGEX = re.compile(r"PyAPI_FUNC(.+) (\w+)\(") | ||
| SIMPLE_MACRO_REGEX = re.compile(r"# *define *(\w+)(\(.+\))? ") | ||
| SIMPLE_INLINE_REGEX = re.compile(r"static inline .+( |\n)(\w+)") | ||
| SIMPLE_DATA_REGEX = re.compile(r"PyAPI_DATA\(.+\) (\w+)") | ||
|
|
||
| CPYTHON = Path(__file__).parent.parent.parent | ||
| INCLUDE = CPYTHON / "Include" | ||
| C_API_DOCS = CPYTHON / "Doc" / "c-api" | ||
| IGNORED = ( | ||
| (CPYTHON / "Tools" / "check-c-api-docs" / "ignored_c_api.txt") | ||
| .read_text() | ||
| .split("\n") | ||
| ) | ||
|
|
||
| for index, line in enumerate(IGNORED): | ||
| if line.startswith("#"): | ||
| IGNORED.pop(index) | ||
|
|
||
| MISTAKE = """ | ||
| If this is a mistake and this script should not be failing, create an | ||
| issue and tag Peter (@ZeroIntensity) on it.\ | ||
| """ | ||
|
|
||
|
|
||
| def found_undocumented(singular: bool) -> str: | ||
| some = "an" if singular else "some" | ||
| s = "" if singular else "s" | ||
| these = "this" if singular else "these" | ||
| them = "it" if singular else "them" | ||
| were = "was" if singular else "were" | ||
|
|
||
| return ( | ||
| textwrap.dedent( | ||
| f""" | ||
| Found {some} undocumented C API{s}! | ||
|
|
||
| Python requires documentation on all public C API symbols, macros, and types. | ||
| If {these} API{s} {were} not meant to be public, prefix {them} with a | ||
| leading underscore (_PySomething_API) or move {them} to the internal C API | ||
| (pycore_*.h files). | ||
|
|
||
| In exceptional cases, certain APIs can be ignored by adding them to | ||
| Tools/c-api-docs-check/ignored_c_api.txt | ||
| """ | ||
| ) | ||
| + MISTAKE | ||
| ) | ||
|
|
||
|
|
||
| def found_ignored_documented(singular: bool) -> str: | ||
| some = "a" if singular else "some" | ||
| s = "" if singular else "s" | ||
| them = "it" if singular else "them" | ||
| were = "was" if singular else "were" | ||
| they = "it" if singular else "they" | ||
|
|
||
| return ( | ||
| textwrap.dedent( | ||
| f""" | ||
| Found {some} C API{s} listed in Tools/c-api-docs-check/ignored_c_api.txt, but | ||
| {they} {were} found in the documentation. To fix this, remove {them} from | ||
| ignored_c_api.txt. | ||
| """ | ||
| ) | ||
| + MISTAKE | ||
| ) | ||
|
|
||
|
|
||
| def is_documented(name: str) -> bool: | ||
| """ | ||
| Is a name present in the C API documentation? | ||
| """ | ||
| for path in C_API_DOCS.iterdir(): | ||
| if path.is_dir(): | ||
| continue | ||
| if path.suffix != ".rst": | ||
| continue | ||
|
|
||
| text = path.read_text(encoding="utf-8") | ||
| if name in text: | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
|
|
||
| def scan_file_for_docs(filename: str, text: str) -> tuple[list[str], list[str]]: | ||
| """ | ||
| Scan a header file for C API functions. | ||
| """ | ||
| undocumented: list[str] = [] | ||
| documented_ignored: list[str] = [] | ||
| colors = _colorize.get_colors() | ||
|
|
||
| def check_for_name(name: str) -> None: | ||
| documented = is_documented(name) | ||
| if documented and (name in IGNORED): | ||
| documented_ignored.append(name) | ||
| elif not documented and (name not in IGNORED): | ||
| undocumented.append(name) | ||
|
|
||
| for function in SIMPLE_FUNCTION_REGEX.finditer(text): | ||
| name = function.group(2) | ||
| if not name.startswith("Py"): | ||
| continue | ||
|
|
||
| check_for_name(name) | ||
|
|
||
| for macro in SIMPLE_MACRO_REGEX.finditer(text): | ||
| name = macro.group(1) | ||
| if not name.startswith("Py"): | ||
| continue | ||
|
|
||
| if "(" in name: | ||
| name = name[: name.index("(")] | ||
|
|
||
| check_for_name(name) | ||
|
|
||
| for inline in SIMPLE_INLINE_REGEX.finditer(text): | ||
| name = inline.group(2) | ||
| if not name.startswith("Py"): | ||
| continue | ||
|
|
||
| check_for_name(name) | ||
|
|
||
| for data in SIMPLE_DATA_REGEX.finditer(text): | ||
| name = data.group(1) | ||
| if not name.startswith("Py"): | ||
| continue | ||
|
|
||
| check_for_name(name) | ||
|
|
||
| # Remove duplicates and sort alphabetically to keep the output deterministic | ||
| undocumented = list(set(undocumented)) | ||
| undocumented.sort() | ||
|
|
||
| if undocumented or documented_ignored: | ||
| print(f"{filename} {colors.RED}BAD{colors.RESET}") | ||
| for name in undocumented: | ||
| print(f"{colors.BOLD_RED}UNDOCUMENTED:{colors.RESET} {name}") | ||
| for name in documented_ignored: | ||
| print(f"{colors.BOLD_YELLOW}DOCUMENTED BUT IGNORED:{colors.RESET} {name}") | ||
| else: | ||
| print(f"{filename} {colors.GREEN}OK{colors.RESET}") | ||
|
|
||
| return undocumented, documented_ignored | ||
|
|
||
|
|
||
| def main() -> None: | ||
| print("Scanning for undocumented C API functions...") | ||
| files = [*INCLUDE.iterdir(), *(INCLUDE / "cpython").iterdir()] | ||
| all_missing: list[str] = [] | ||
| all_found_ignored: list[str] = [] | ||
|
|
||
| for file in files: | ||
| if file.is_dir(): | ||
| continue | ||
| assert file.exists() | ||
| text = file.read_text(encoding="utf-8") | ||
| missing, ignored = scan_file_for_docs(str(file.relative_to(INCLUDE)), text) | ||
| all_found_ignored += ignored | ||
| all_missing += missing | ||
|
|
||
| fail = False | ||
| to_check = [ | ||
| (all_missing, "missing", found_undocumented(len(all_missing) == 1)), | ||
| ( | ||
| all_found_ignored, | ||
| "documented but ignored", | ||
| found_ignored_documented(len(all_found_ignored) == 1), | ||
| ), | ||
| ] | ||
| for name_list, what, message in to_check: | ||
| if not name_list: | ||
| continue | ||
|
|
||
| s = "s" if len(name_list) != 1 else "" | ||
hugovk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| print(f"-- {len(name_list)} {what} C API{s} --") | ||
| for name in name_list: | ||
| print(f" - {name}") | ||
| print(message) | ||
| fail = True | ||
|
|
||
| sys.exit(1 if fail else 0) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() | ||
Oops, something went wrong.
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.
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.
I think
is_documentedis a little different from ispresent. Why not improve the check to include verifying it's a doc declaration, i.e. if it's in the text check it's on the same line as thec:func::etc.?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.
I thought about that, but I decided that it was a case of "practicality beats purity". Running Sphinx or using a regular expression will make this significantly slower, and probably won't save us much in practice.
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.
As a compromise, we could go ever simpler, just checking if
::and..are in the same line as the API name.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.
Hm, that may work, but the list in #141004 was created using a full-text search, not a search for
::or.., so there are probably some APIs that still need work. Let's finish the list in #141004 first, and then we can look into finding more edge cases.