[libclang/python] Add some logical typing changes.

[DeinAlptraum]

This adds a few logical changes that might require discussion to the libclang/python typing project, as the next step towards #76664 .

Some documentation is also updated to reflect the recent changes in returned types.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jannick Kremer (DeinAlptraum)

Changes

This adds a few logical changes that might require some discussion to the libclang/python typing project, as the next step towards #76664

---

Full diff: https://github.com/llvm/llvm-project/pull/138074.diff

1 Files Affected:

• (modified) clang/bindings/python/clang/cindex.py (+26-11)

diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index 8dc79f28a090a..2cd8df6a8a3e9 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -71,8 +71,10 @@
 from typing import (
     Any,
     Callable,
+    cast as Tcast,
     Generic,
     Optional,
+    Sequence,
     Type as TType,
     TypeVar,
     TYPE_CHECKING,
@@ -314,6 +316,8 @@ def is_in_system_header(self):
         return conf.lib.clang_Location_isInSystemHeader(self)  # type: ignore [no-any-return]
 
     def __eq__(self, other):
+        if not isinstance(other, SourceLocation):
+            return False
         return conf.lib.clang_equalLocations(self, other)  # type: ignore [no-any-return]
 
     def __ne__(self, other):
@@ -372,6 +376,8 @@ def end(self):
         return conf.lib.clang_getRangeEnd(self)  # type: ignore [no-any-return]
 
     def __eq__(self, other):
+        if not isinstance(other, SourceRange):
+            return False
         return conf.lib.clang_equalRanges(self, other)  # type: ignore [no-any-return]
 
     def __ne__(self, other):
@@ -1556,6 +1562,8 @@ def from_location(tu, location):
         return cursor
 
     def __eq__(self, other):
+        if not isinstance(other, Cursor):
+            return False
         return conf.lib.clang_equalCursors(self, other)  # type: ignore [no-any-return]
 
     def __ne__(self, other):
@@ -1746,7 +1754,7 @@ def get_definition(self):
 
     def get_usr(self):
         """Return the Unified Symbol Resolution (USR) for the entity referenced
-        by the given cursor (or None).
+        by the given cursor.
 
         A Unified Symbol Resolution (USR) is a string that identifies a
         particular entity (function, class, variable, etc.) within a
@@ -2776,7 +2784,7 @@ def pretty_printed(self, policy):
         return _CXString.from_result(conf.lib.clang_getTypePrettyPrinted(self, policy))
 
     def __eq__(self, other):
-        if type(other) != type(self):
+        if not isinstance(other, Type):
             return False
 
         return conf.lib.clang_equalTypes(self, other)  # type: ignore [no-any-return]
@@ -2888,8 +2896,7 @@ def string(self):
 
         if res:
             return CompletionString(res)
-        else:
-            None
+        return None
 
     def isKindOptional(self):
         return self.__kindNumber == 0
@@ -2955,6 +2962,10 @@ def __getitem__(self, key):
             raise IndexError
         return CompletionChunk(self.obj, key)
 
+    def __iter__(self):
+        for i in range(len(self)):
+            yield self[i]
+
     @property
     def priority(self):
         return conf.lib.clang_getCompletionPriority(self.obj)  # type: ignore [no-any-return]
@@ -2970,7 +2981,7 @@ def briefComment(self):
             return _CXString.from_result(
                 conf.lib.clang_getCompletionBriefComment(self.obj)
             )
-        return _CXString()
+        return ""
 
     def __repr__(self):
         return (
@@ -3155,8 +3166,8 @@ def from_source(
         a list via args. These can be used to specify include paths, warnings,
         etc. e.g. ["-Wall", "-I/path/to/include"].
 
-        In-memory file content can be provided via unsaved_files. This is an
-        iterable of 2-tuples. The first element is the filename (str or
+        In-memory file content can be provided via unsaved_files. This is a
+        list of 2-tuples. The first element is the filename (str or
         PathLike). The second element defines the content. Content can be
         provided as str source code or as file objects (anything with a read()
         method). If a file object is being used, content will be read until EOF
@@ -3328,6 +3339,7 @@ def get_extent(self, filename, locations):
         start_location, end_location = locations
 
         if hasattr(start_location, "__len__"):
+            start_location = Tcast(Sequence[int], start_location)
             start_location = SourceLocation.from_position(
                 self, f, start_location[0], start_location[1]
             )
@@ -3335,6 +3347,7 @@ def get_extent(self, filename, locations):
             start_location = SourceLocation.from_offset(self, f, start_location)
 
         if hasattr(end_location, "__len__"):
+            end_location = Tcast(Sequence[int], end_location)
             end_location = SourceLocation.from_position(
                 self, f, end_location[0], end_location[1]
             )
@@ -3466,6 +3479,8 @@ def get_tokens(self, locations=None, extent=None):
         """
         if locations is not None:
             extent = SourceRange(start=locations[0], end=locations[1])
+        if extent is None:
+            raise TypeError("get_tokens() requires at least one argument")
 
         return TokenGroup.get_tokens(self, extent)
 
@@ -3502,11 +3517,11 @@ def __repr__(self):
     @staticmethod
     def from_result(res, arg):
         assert isinstance(res, c_object_p)
-        res = File(res)
+        file = File(res)
 
         # Copy a reference to the TranslationUnit to prevent premature GC.
-        res._tu = arg._tu
-        return res
+        file._tu = arg._tu
+        return file
 
 
 class FileInclusion:
@@ -3585,7 +3600,7 @@ def filename(self):
     def arguments(self):
         """
         Get an iterable object providing each argument in the
-        command line for the compiler invocation as a _CXString.
+        command line for the compiler invocation as a string.
 
         Invariant : the first argument is the compiler executable
         """

[DeinAlptraum]

Some notes on the changes here

[Endilll]

Thank you for working on this!
Looks good overall. I left some comments

[DeinAlptraum]

@Endilll thank you for the review, I've implemented your suggestions

[llvm-ci]

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-4 while building clang at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/19308

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'Clang-Unit :: ./AllClangUnitTests/8/48' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/Users/buildbot/buildbot-root/aarch64-darwin/build/tools/clang/unittests/./AllClangUnitTests-Clang-Unit-51874-8-48.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=48 GTEST_SHARD_INDEX=8 /Users/buildbot/buildbot-root/aarch64-darwin/build/tools/clang/unittests/./AllClangUnitTests
--

Script:
--
/Users/buildbot/buildbot-root/aarch64-darwin/build/tools/clang/unittests/./AllClangUnitTests --gtest_filter=TimeProfilerTest.ConstantEvaluationC99
--
/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/unittests/Support/TimeProfilerTest.cpp:347: Failure
Expected equality of these values:
  R"(
Frontend (test.c)
| ParseDeclarationOrFunctionDefinition (test.c:2:1)
| | isIntegerConstantExpr (<test.c:3:18>)
| | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)
| PerformPendingInstantiations
)"
    Which is: "\nFrontend (test.c)\n| ParseDeclarationOrFunctionDefinition (test.c:2:1)\n| | isIntegerConstantExpr (<test.c:3:18>)\n| | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)\n| PerformPendingInstantiations\n"
  buildTraceGraph(Json)
    Which is: "\nFrontend (test.c)\n| ParseDeclarationOrFunctionDefinition (test.c:2:1)\n| | isIntegerConstantExpr (<test.c:3:18>)\n| | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)\n| | PerformPendingInstantiations\n"
With diff:
@@ -4,3 +4,3 @@
 | | isIntegerConstantExpr (<test.c:3:18>)
 | | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)
-| PerformPendingInstantiations\n
+| | PerformPendingInstantiations\n



/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/unittests/Support/TimeProfilerTest.cpp:347
Expected equality of these values:
  R"(
Frontend (test.c)
| ParseDeclarationOrFunctionDefinition (test.c:2:1)
| | isIntegerConstantExpr (<test.c:3:18>)
| | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)
| PerformPendingInstantiations
)"
    Which is: "\nFrontend (test.c)\n| ParseDeclarationOrFunctionDefinition (test.c:2:1)\n| | isIntegerConstantExpr (<test.c:3:18>)\n| | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)\n| PerformPendingInstantiations\n"
  buildTraceGraph(Json)
    Which is: "\nFrontend (test.c)\n| ParseDeclarationOrFunctionDefinition (test.c:2:1)\n| | isIntegerConstantExpr (<test.c:3:18>)\n| | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)\n| | PerformPendingInstantiations\n"
With diff:
@@ -4,3 +4,3 @@
 | | isIntegerConstantExpr (<test.c:3:18>)
 | | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)
-| PerformPendingInstantiations\n
+| | PerformPendingInstantiations\n

...

[Endilll]

I totally forgot about release notes and tests.
While release notes may not be necessary, I think we should test those new checks that you added.

[DeinAlptraum]

Which parts do you mean? The __eq__ operators for other objects?

[Endilll]

Which parts do you mean? The __eq__ operators for other objects?

Yes, and briefComment function, too.