[libclang/python] Do not rely on ctypes' errcheck

[DeinAlptraum]

Call conversion functions directly instead of using them for type conversion on library function calls via ctypes' errcheck functionality.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jannick Kremer (DeinAlptraum)

Changes

Call conversion functions directly instead of using them for type conversion on library function calls via ctypes' errcheck functionality.

---

Patch is 35.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105490.diff

1 Files Affected:

• (modified) clang/bindings/python/clang/cindex.py (+168-185)

diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index 4da99e899e7f7c..f8a20a1e224724 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -133,7 +133,7 @@ def from_param(cls, param: str | bytes | None) -> c_interop_string:
         )
 
     @staticmethod
-    def to_python_string(x: c_interop_string, *args: Any) -> str | None:
+    def to_python_string(x: c_interop_string) -> str | None:
         return x.value
 
 
@@ -241,9 +241,9 @@ def __del__(self) -> None:
         conf.lib.clang_disposeString(self)
 
     @staticmethod
-    def from_result(res: _CXString, fn: Any = None, args: Any = None) -> str:
+    def from_result(res: _CXString) -> str:
         assert isinstance(res, _CXString)
-        pystr: str | None = conf.lib.clang_getCString(res)
+        pystr = c_interop_string.to_python_string(conf.lib.clang_getCString(res))
         if pystr is None:
             return ""
         return pystr
@@ -424,7 +424,7 @@ def location(self):
 
     @property
     def spelling(self):
-        return conf.lib.clang_getDiagnosticSpelling(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getDiagnosticSpelling(self))
 
     @property
     def ranges(self) -> NoSliceSequence[SourceRange]:
@@ -453,7 +453,9 @@ def __len__(self) -> int:
 
             def __getitem__(self, key: int) -> FixIt:
                 range = SourceRange()
-                value = conf.lib.clang_getDiagnosticFixIt(self.diag, key, byref(range))
+                value = _CXString.from_result(
+                    conf.lib.clang_getDiagnosticFixIt(self.diag, key, byref(range))
+                )
                 if len(value) == 0:
                     raise IndexError
 
@@ -486,12 +488,12 @@ def category_number(self):
     @property
     def category_name(self):
         """The string name of the category for this diagnostic."""
-        return conf.lib.clang_getDiagnosticCategoryText(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getDiagnosticCategoryText(self))
 
     @property
     def option(self):
         """The command-line option that enables this diagnostic."""
-        return conf.lib.clang_getDiagnosticOption(self, None)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getDiagnosticOption(self, None))
 
     @property
     def disable_option(self):
@@ -511,7 +513,7 @@ def format(self, options=None):
             options = conf.lib.clang_defaultDiagnosticDisplayOptions()
         if options & ~Diagnostic._FormatOptionsMask:
             raise ValueError("Invalid format options")
-        return conf.lib.clang_formatDiagnostic(self, options)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_formatDiagnostic(self, options))
 
     def __repr__(self):
         return "<Diagnostic severity %r, location %r, spelling %r>" % (
@@ -1734,7 +1736,7 @@ def get_definition(self):
         """
         # TODO: Should probably check that this is either a reference or
         # declaration prior to issuing the lookup.
-        return conf.lib.clang_getCursorDefinition(self)  # type: ignore [no-any-return]
+        return Cursor.from_result(conf.lib.clang_getCursorDefinition(self), self)
 
     def get_usr(self):
         """Return the Unified Symbol Resolution (USR) for the entity referenced
@@ -1745,13 +1747,13 @@ def get_usr(self):
         program. USRs can be compared across translation units to determine,
         e.g., when references in one translation refer to an entity defined in
         another translation unit."""
-        return conf.lib.clang_getCursorUSR(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getCursorUSR(self))
 
     def get_included_file(self):
         """Returns the File that is included by the current inclusion cursor."""
         assert self.kind == CursorKind.INCLUSION_DIRECTIVE
 
-        return conf.lib.clang_getIncludedFile(self)  # type: ignore [no-any-return]
+        return File.from_result(conf.lib.clang_getIncludedFile(self), self)
 
     @property
     def kind(self):
@@ -1762,7 +1764,9 @@ def kind(self):
     def spelling(self):
         """Return the spelling of the entity pointed at by the cursor."""
         if not hasattr(self, "_spelling"):
-            self._spelling = conf.lib.clang_getCursorSpelling(self)
+            self._spelling = _CXString.from_result(
+                conf.lib.clang_getCursorSpelling(self)
+            )
 
         return self._spelling
 
@@ -1776,7 +1780,9 @@ def displayname(self):
         arguments of a class template specialization.
         """
         if not hasattr(self, "_displayname"):
-            self._displayname = conf.lib.clang_getCursorDisplayName(self)
+            self._displayname = _CXString.from_result(
+                conf.lib.clang_getCursorDisplayName(self)
+            )
 
         return self._displayname
 
@@ -1784,7 +1790,9 @@ def displayname(self):
     def mangled_name(self):
         """Return the mangled name for the entity referenced by this cursor."""
         if not hasattr(self, "_mangled_name"):
-            self._mangled_name = conf.lib.clang_Cursor_getMangling(self)
+            self._mangled_name = _CXString.from_result(
+                conf.lib.clang_Cursor_getMangling(self)
+            )
 
         return self._mangled_name
 
@@ -1876,7 +1884,7 @@ def type(self):
         Retrieve the Type (if any) of the entity pointed at by the cursor.
         """
         if not hasattr(self, "_type"):
-            self._type = conf.lib.clang_getCursorType(self)
+            self._type = Type.from_result(conf.lib.clang_getCursorType(self), (self,))
 
         return self._type
 
@@ -1890,7 +1898,9 @@ def canonical(self):
         declarations will be identical.
         """
         if not hasattr(self, "_canonical"):
-            self._canonical = conf.lib.clang_getCanonicalCursor(self)
+            self._canonical = Cursor.from_cursor_result(
+                conf.lib.clang_getCanonicalCursor(self), self
+            )
 
         return self._canonical
 
@@ -1898,7 +1908,9 @@ def canonical(self):
     def result_type(self):
         """Retrieve the Type of the result for this Cursor."""
         if not hasattr(self, "_result_type"):
-            self._result_type = conf.lib.clang_getCursorResultType(self)
+            self._result_type = Type.from_result(
+                conf.lib.clang_getCursorResultType(self), (self,)
+            )
 
         return self._result_type
 
@@ -1925,7 +1937,9 @@ def underlying_typedef_type(self):
         """
         if not hasattr(self, "_underlying_type"):
             assert self.kind.is_declaration()
-            self._underlying_type = conf.lib.clang_getTypedefDeclUnderlyingType(self)
+            self._underlying_type = Type.from_result(
+                conf.lib.clang_getTypedefDeclUnderlyingType(self), (self,)
+            )
 
         return self._underlying_type
 
@@ -1938,7 +1952,9 @@ def enum_type(self):
         """
         if not hasattr(self, "_enum_type"):
             assert self.kind == CursorKind.ENUM_DECL
-            self._enum_type = conf.lib.clang_getEnumDeclIntegerType(self)
+            self._enum_type = Type.from_result(
+                conf.lib.clang_getEnumDeclIntegerType(self), (self,)
+            )
 
         return self._enum_type
 
@@ -1972,7 +1988,9 @@ def enum_value(self):
     def objc_type_encoding(self):
         """Return the Objective-C type encoding as a str."""
         if not hasattr(self, "_objc_type_encoding"):
-            self._objc_type_encoding = conf.lib.clang_getDeclObjCTypeEncoding(self)
+            self._objc_type_encoding = _CXString.from_result(
+                conf.lib.clang_getDeclObjCTypeEncoding(self)
+            )
 
         return self._objc_type_encoding
 
@@ -1988,7 +2006,9 @@ def hash(self):
     def semantic_parent(self):
         """Return the semantic parent for this cursor."""
         if not hasattr(self, "_semantic_parent"):
-            self._semantic_parent = conf.lib.clang_getCursorSemanticParent(self)
+            self._semantic_parent = Cursor.from_cursor_result(
+                conf.lib.clang_getCursorSemanticParent(self), self
+            )
 
         return self._semantic_parent
 
@@ -1996,7 +2016,9 @@ def semantic_parent(self):
     def lexical_parent(self):
         """Return the lexical parent for this cursor."""
         if not hasattr(self, "_lexical_parent"):
-            self._lexical_parent = conf.lib.clang_getCursorLexicalParent(self)
+            self._lexical_parent = Cursor.from_cursor_result(
+                conf.lib.clang_getCursorLexicalParent(self), self
+            )
 
         return self._lexical_parent
 
@@ -2014,25 +2036,27 @@ def referenced(self):
         representing the entity that it references.
         """
         if not hasattr(self, "_referenced"):
-            self._referenced = conf.lib.clang_getCursorReferenced(self)
+            self._referenced = Cursor.from_result(
+                conf.lib.clang_getCursorReferenced(self), self
+            )
 
         return self._referenced
 
     @property
     def brief_comment(self):
         """Returns the brief comment text associated with that Cursor"""
-        return conf.lib.clang_Cursor_getBriefCommentText(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_Cursor_getBriefCommentText(self))
 
     @property
     def raw_comment(self):
         """Returns the raw comment text associated with that Cursor"""
-        return conf.lib.clang_Cursor_getRawCommentText(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_Cursor_getRawCommentText(self))
 
     def get_arguments(self):
         """Return an iterator for accessing the arguments of this cursor."""
         num_args = conf.lib.clang_Cursor_getNumArguments(self)
         for i in range(0, num_args):
-            yield conf.lib.clang_Cursor_getArgument(self, i)
+            yield Cursor.from_result(conf.lib.clang_Cursor_getArgument(self, i), self)
 
     def get_num_template_arguments(self):
         """Returns the number of template args associated with this cursor."""
@@ -2041,11 +2065,15 @@ def get_num_template_arguments(self):
     def get_template_argument_kind(self, num):
         """Returns the TemplateArgumentKind for the indicated template
         argument."""
-        return conf.lib.clang_Cursor_getTemplateArgumentKind(self, num)  # type: ignore [no-any-return]
+        return TemplateArgumentKind.from_id(
+            conf.lib.clang_Cursor_getTemplateArgumentKind(self, num)
+        )
 
     def get_template_argument_type(self, num):
         """Returns the CXType for the indicated template argument."""
-        return conf.lib.clang_Cursor_getTemplateArgumentType(self, num)  # type: ignore [no-any-return]
+        return Type.from_result(
+            conf.lib.clang_Cursor_getTemplateArgumentType(self, num), (self, num)
+        )
 
     def get_template_argument_value(self, num):
         """Returns the value of the indicated arg as a signed 64b integer."""
@@ -2116,7 +2144,7 @@ def get_bitfield_width(self):
         return conf.lib.clang_getFieldDeclBitWidth(self)  # type: ignore [no-any-return]
 
     @staticmethod
-    def from_result(res, fn, args):
+    def from_result(res, arg):
         assert isinstance(res, Cursor)
         # FIXME: There should just be an isNull method.
         if res == conf.lib.clang_getNullCursor():
@@ -2125,14 +2153,10 @@ def from_result(res, fn, args):
         # Store a reference to the TU in the Python object so it won't get GC'd
         # before the Cursor.
         tu = None
-        for arg in args:
-            if isinstance(arg, TranslationUnit):
-                tu = arg
-                break
-
-            if hasattr(arg, "translation_unit"):
-                tu = arg.translation_unit
-                break
+        if isinstance(arg, TranslationUnit):
+            tu = arg
+        elif hasattr(arg, "translation_unit"):
+            tu = arg.translation_unit
 
         assert tu is not None
 
@@ -2140,12 +2164,12 @@ def from_result(res, fn, args):
         return res
 
     @staticmethod
-    def from_cursor_result(res, fn, args):
+    def from_cursor_result(res, arg):
         assert isinstance(res, Cursor)
         if res == conf.lib.clang_getNullCursor():
             return None
 
-        res._tu = args[0]._tu
+        res._tu = arg._tu
         return res
 
 
@@ -2250,7 +2274,7 @@ class TypeKind(BaseEnumeration):
     @property
     def spelling(self):
         """Retrieve the spelling of this TypeKind."""
-        return conf.lib.clang_getTypeKindSpelling(self.value)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getTypeKindSpelling(self.value))
 
     INVALID = 0
     UNEXPOSED = 1
@@ -2438,7 +2462,9 @@ def __getitem__(self, key: int) -> Type:
                         "%d > %d" % (key, len(self))
                     )
 
-                result: Type = conf.lib.clang_getArgType(self.parent, key)
+                result = Type.from_result(
+                    conf.lib.clang_getArgType(self.parent, key), (self.parent, key)
+                )
                 if result.kind == TypeKind.INVALID:
                     raise IndexError("Argument could not be retrieved.")
 
@@ -2454,7 +2480,7 @@ def element_type(self):
         If accessed on a type that is not an array, complex, or vector type, an
         exception will be raised.
         """
-        result = conf.lib.clang_getElementType(self)
+        result = Type.from_result(conf.lib.clang_getElementType(self), (self,))
         if result.kind == TypeKind.INVALID:
             raise Exception("Element type not available on this type.")
 
@@ -2482,7 +2508,7 @@ def translation_unit(self):
         return self._tu
 
     @staticmethod
-    def from_result(res, fn, args):
+    def from_result(res, args):
         assert isinstance(res, Type)
 
         tu = None
@@ -2500,7 +2526,9 @@ def get_num_template_arguments(self):
         return conf.lib.clang_Type_getNumTemplateArguments(self)  # type: ignore [no-any-return]
 
     def get_template_argument_type(self, num):
-        return conf.lib.clang_Type_getTemplateArgumentAsType(self, num)  # type: ignore [no-any-return]
+        return Type.from_result(
+            conf.lib.clang_Type_getTemplateArgumentAsType(self, num), (self, num)
+        )
 
     def get_canonical(self):
         """
@@ -2512,7 +2540,7 @@ def get_canonical(self):
         example, if 'T' is a typedef for 'int', the canonical type for
         'T' would be 'int'.
         """
-        return conf.lib.clang_getCanonicalType(self)  # type: ignore [no-any-return]
+        return Type.from_result(conf.lib.clang_getCanonicalType(self), (self,))
 
     def is_const_qualified(self):
         """Determine whether a Type has the "const" qualifier set.
@@ -2548,7 +2576,7 @@ def get_address_space(self):
         return conf.lib.clang_getAddressSpace(self)  # type: ignore [no-any-return]
 
     def get_typedef_name(self):
-        return conf.lib.clang_getTypedefName(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getTypedefName(self))
 
     def is_pod(self):
         """Determine whether this Type represents plain old data (POD)."""
@@ -2558,25 +2586,25 @@ def get_pointee(self):
         """
         For pointer types, returns the type of the pointee.
         """
-        return conf.lib.clang_getPointeeType(self)  # type: ignore [no-any-return]
+        return Type.from_result(conf.lib.clang_getPointeeType(self), (self,))
 
     def get_declaration(self):
         """
         Return the cursor for the declaration of the given type.
         """
-        return conf.lib.clang_getTypeDeclaration(self)  # type: ignore [no-any-return]
+        return Cursor.from_result(conf.lib.clang_getTypeDeclaration(self), self)
 
     def get_result(self):
         """
         Retrieve the result type associated with a function type.
         """
-        return conf.lib.clang_getResultType(self)  # type: ignore [no-any-return]
+        return Type.from_result(conf.lib.clang_getResultType(self), (self,))
 
     def get_array_element_type(self):
         """
         Retrieve the type of the elements of the array type.
         """
-        return conf.lib.clang_getArrayElementType(self)  # type: ignore [no-any-return]
+        return Type.from_result(conf.lib.clang_getArrayElementType(self), (self,))
 
     def get_array_size(self):
         """
@@ -2588,13 +2616,13 @@ def get_class_type(self):
         """
         Retrieve the class type of the member pointer type.
         """
-        return conf.lib.clang_Type_getClassType(self)  # type: ignore [no-any-return]
+        return Type.from_result(conf.lib.clang_Type_getClassType(self), (self,))
 
     def get_named_type(self):
         """
         Retrieve the type named by the qualified-id.
         """
-        return conf.lib.clang_Type_getNamedType(self)  # type: ignore [no-any-return]
+        return Type.from_result(conf.lib.clang_Type_getNamedType(self), (self,))
 
     def get_align(self):
         """
@@ -2647,7 +2675,7 @@ def get_exception_specification_kind(self):
     @property
     def spelling(self):
         """Retrieve the spelling of this Type."""
-        return conf.lib.clang_getTypeSpelling(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getTypeSpelling(self))
 
     def __eq__(self, other):
         if type(other) != type(self):
@@ -2737,7 +2765,9 @@ def __repr__(self):
     def spelling(self):
         if self.__kindNumber in SpellingCache:
             return SpellingCache[self.__kindNumber]
-        return conf.lib.clang_getCompletionChunkText(self.cs, self.key)  # type: ignore [no-any-return]
+        return _CXString.from_result(
+            conf.lib.clang_getCompletionChunkText(self.cs, self.key)
+        )
 
     # We do not use @CachedProperty here, as the manual implementation is
     # apparently still significantly faster. Please profile carefully if you
@@ -2839,7 +2869,9 @@ def availability(self):
     @property
     def briefComment(self):
         if conf.function_exists("clang_getCompletionBriefComment"):
-            return conf.lib.clang_getCompletionBriefComment(self.obj)  # type: ignore [no-any-return]
+            return _CXString.from_result(
+                conf.lib.clang_getCompletionBriefComment(self.obj)
+            )
         return _CXString()
 
     def __repr__(self):
@@ -3125,12 +3157,12 @@ def __del__(self):
     @property
     def cursor(self):
         """Retrieve the cursor that represents the given translation unit."""
-        return conf.lib.clang_getTranslationUnitCursor(self)  # type: ignore [no-any-return]
+        return Cursor.from_result(conf.lib.clang_getTranslationUnitCursor(self), self)
 
     @property
     def spelling(self):
         """Get the original translation unit source file name."""
-        return conf.lib.clang_getTranslationUnitSpelling(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getTranslationUnitSpelling(self))
 
     def get_includes(self):
         """
@@ -3356,7 +3388,7 @@ def from_name(translation_unit, file_name):
     @property
     def name(self):
         """Return the complete file and path name of the file."""
-        return conf.lib.clang_getFileName(self)  # type: ignore [no-any-return]
+        return _CXString.from_result(conf.lib.clang_getFileName(self))
 
     @property
     def time(self):
@@ -3370,12 +3402,12 @@ def __repr__(self):
         return "<File: %s>" % (self.name)
 
     @staticmethod
-    def from_result(res, fn, args):
+    def from_result(res, arg):
         assert isinstance(res, c_object_p)
         res = File(res)
 
         # Copy a reference to the TranslationUnit to prevent premature GC.
-        res._tu = args[0]._tu
+        res._tu = arg._tu
         return res
 
 
@@ -3440,12 +3472,16 @@ def __init__(self, cmd, ccmds):
     @property
     def directory(self):
         """Get the working directory for this CompileCommand"""
-        return conf.lib.clang_CompileCommand_getDirectory(self.cmd)  # type: ignore [no-any-return]
+        return _CXString.from_res...
[truncated]

[DeinAlptraum]

@Endilll this removes the errcheck magic conversion stuff that we discussed the other day

[Endilll]

Any chance you were able to test those changes?

[DeinAlptraum]

@Endilll tested how exactly?
All tests still pass, obviously, but I just checked and there are six lines changed here, that are not currently covered by tests:
Diagnostic.format(), Cursor.get_definition(), Cursor.get_usr(), Type.get_result(), Type.get_class_type(), Type.get_named_type()

Should I add tests for all of these first, and then merge that into this PR?

[Endilll]

Should I add tests for all of these first, and then merge that into this PR?

That would be nice!

[DeinAlptraum]

@Endilll I rebased with the added tests from #109846 and checked with diff-cover that all changed lines are covered by tests