Skip to content

[libclang/python] Bump minimum compatibility to Python 3.6 #77228

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

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

linux4life798
Copy link
Contributor

Remove Python 2 support and clean up code that conditions based on version.

Issue #76664.

This simply removes the Python 2 arm of this conditional logic
and unindents the Python 3 logic.
In Python3, inheriting from the object class is implicit.
The LLVM minimum python version is 3.6. The os.fspath
function is always available in Python 3.6, so we
remove the unnecessary fallback.

https://docs.python.org/3/library/os.html#os.fspath

Issues llvm#76664.
The LLVM minimum python version is 3.6. The abstract
base classes were copied from collections to
collections.abc in python 3.3. I believe the usage
of ABC from the original "collections" module or
"collections.abc" module were valid between 3.3 and
3.7, but was eventually dropped in 3.8. So, I believe
this method of import has been valid since 3.3.

https://docs.python.org/dev/library/collections.abc.html#module-collections.abc

Issues llvm#76664.
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jan 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 7, 2024

@llvm/pr-subscribers-clang

Author: Craig Hesling (linux4life798)

Changes

Remove Python 2 support and clean up code that conditions based on version.

Issue #76664.


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

2 Files Affected:

  • (modified) clang/bindings/python/README.txt (+1-1)
  • (modified) clang/bindings/python/clang/cindex.py (+73-98)
diff --git a/clang/bindings/python/README.txt b/clang/bindings/python/README.txt
index 44c715e5de56f7..3e509662144fac 100644
--- a/clang/bindings/python/README.txt
+++ b/clang/bindings/python/README.txt
@@ -10,7 +10,7 @@ runner. For example:
 --
 $ env PYTHONPATH=$(echo ~/llvm/clang/bindings/python/) \
       CLANG_LIBRARY_PATH=$(llvm-config --libdir) \
-  python -m unittest discover -v
+  python3 -m unittest discover -v
 tests.cindex.test_index.test_create ... ok
 ...
 
diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index d780ee353a133c..754f03d718e882 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -66,81 +66,50 @@
 
 import clang.enumerations
 
+import collections.abc
 import os
-import sys
-
-if sys.version_info[0] == 3:
-    # Python 3 strings are unicode, translate them to/from utf8 for C-interop.
-    class c_interop_string(c_char_p):
-        def __init__(self, p=None):
-            if p is None:
-                p = ""
-            if isinstance(p, str):
-                p = p.encode("utf8")
-            super(c_char_p, self).__init__(p)
 
-        def __str__(self):
-            return self.value
-
-        @property
-        def value(self):
-            if super(c_char_p, self).value is None:
-                return None
-            return super(c_char_p, self).value.decode("utf8")
-
-        @classmethod
-        def from_param(cls, param):
-            if isinstance(param, str):
-                return cls(param)
-            if isinstance(param, bytes):
-                return cls(param)
-            if param is None:
-                # Support passing null to C functions expecting char arrays
-                return None
-            raise TypeError(
-                "Cannot convert '{}' to '{}'".format(type(param).__name__, cls.__name__)
-            )
-
-        @staticmethod
-        def to_python_string(x, *args):
-            return x.value
 
-    def b(x):
-        if isinstance(x, bytes):
-            return x
-        return x.encode("utf8")
+# Python 3 strings are unicode, translate them to/from utf8 for C-interop.
+class c_interop_string(c_char_p):
+    def __init__(self, p=None):
+        if p is None:
+            p = ""
+        if isinstance(p, str):
+            p = p.encode("utf8")
+        super(c_char_p, self).__init__(p)
 
-elif sys.version_info[0] == 2:
-    # Python 2 strings are utf8 byte strings, no translation is needed for
-    # C-interop.
-    c_interop_string = c_char_p
-
-    def _to_python_string(x, *args):
-        return x
-
-    c_interop_string.to_python_string = staticmethod(_to_python_string)
+    def __str__(self):
+        return self.value
 
-    def b(x):
-        return x
+    @property
+    def value(self):
+        if super(c_char_p, self).value is None:
+            return None
+        return super(c_char_p, self).value.decode("utf8")
 
+    @classmethod
+    def from_param(cls, param):
+        if isinstance(param, str):
+            return cls(param)
+        if isinstance(param, bytes):
+            return cls(param)
+        if param is None:
+            # Support passing null to C functions expecting char arrays
+            return None
+        raise TypeError(
+            "Cannot convert '{}' to '{}'".format(type(param).__name__, cls.__name__)
+        )
 
-# Importing ABC-s directly from collections is deprecated since Python 3.7,
-# will stop working in Python 3.8.
-# See: https://docs.python.org/dev/whatsnew/3.7.html#id3
-if sys.version_info[:2] >= (3, 7):
-    from collections import abc as collections_abc
-else:
-    import collections as collections_abc
+    @staticmethod
+    def to_python_string(x, *args):
+        return x.value
 
-# We only support PathLike objects on Python version with os.fspath present
-# to be consistent with the Python standard library. On older Python versions
-# we only support strings and we have dummy fspath to just pass them through.
-try:
-    fspath = os.fspath
-except AttributeError:
 
-    def fspath(x):
+def b(x):
+    if isinstance(x, bytes):
         return x
+    return x.encode("utf8")
 
 
 # ctypes doesn't implicitly convert c_void_p to the appropriate wrapper
@@ -202,7 +171,7 @@ def __init__(self, enumeration, message):
 ### Structures and Utility Classes ###
 
 
-class CachedProperty(object):
+class CachedProperty:
     """Decorator that lazy-loads the value of a property.
 
     The first time the property is accessed, the original property function is
@@ -392,7 +361,7 @@ def __repr__(self):
         return "<SourceRange start %r, end %r>" % (self.start, self.end)
 
 
-class Diagnostic(object):
+class Diagnostic:
     """
     A Diagnostic is a single instance of a Clang diagnostic. It includes the
     diagnostic severity, the message, the location the diagnostic occurred, as
@@ -433,7 +402,7 @@ def spelling(self):
 
     @property
     def ranges(self):
-        class RangeIterator(object):
+        class RangeIterator:
             def __init__(self, diag):
                 self.diag = diag
 
@@ -449,7 +418,7 @@ def __getitem__(self, key):
 
     @property
     def fixits(self):
-        class FixItIterator(object):
+        class FixItIterator:
             def __init__(self, diag):
                 self.diag = diag
 
@@ -468,7 +437,7 @@ def __getitem__(self, key):
 
     @property
     def children(self):
-        class ChildDiagnosticsIterator(object):
+        class ChildDiagnosticsIterator:
             def __init__(self, diag):
                 self.diag_set = conf.lib.clang_getChildDiagnostics(diag)
 
@@ -532,7 +501,7 @@ def from_param(self):
         return self.ptr
 
 
-class FixIt(object):
+class FixIt:
     """
     A FixIt represents a transformation to be applied to the source to
     "fix-it". The fix-it should be applied by replacing the given source range
@@ -547,7 +516,7 @@ def __repr__(self):
         return "<FixIt range %r, value %r>" % (self.range, self.value)
 
 
-class TokenGroup(object):
+class TokenGroup:
     """Helper class to facilitate token management.
 
     Tokens are allocated from libclang in chunks. They must be disposed of as a
@@ -603,7 +572,7 @@ def get_tokens(tu, extent):
             yield token
 
 
-class TokenKind(object):
+class TokenKind:
     """Describes a specific type of a Token."""
 
     _value_map = {}  # int -> TokenKind
@@ -642,7 +611,7 @@ def register(value, name):
 
 
 ### Cursor Kinds ###
-class BaseEnumeration(object):
+class BaseEnumeration:
     """
     Common base class for named enumerations held in sync with Index.h values.
 
@@ -2059,7 +2028,7 @@ def from_cursor_result(res, fn, args):
         return res
 
 
-class StorageClass(object):
+class StorageClass:
     """
     Describes the storage class of a declaration
     """
@@ -2353,7 +2322,7 @@ def argument_types(self):
         container is a Type instance.
         """
 
-        class ArgumentsIterator(collections_abc.Sequence):
+        class ArgumentsIterator(collections.abc.Sequence):
             def __init__(self, parent):
                 self.parent = parent
                 self.length = None
@@ -2608,7 +2577,7 @@ def __ne__(self, other):
 # a void*.
 
 
-class ClangObject(object):
+class ClangObject:
     """
     A helper for Clang objects. This class helps act as an intermediary for
     the ctypes library and the Clang CIndex library.
@@ -2656,8 +2625,8 @@ class _CXUnsavedFile(Structure):
 }
 
 
-class CompletionChunk(object):
-    class Kind(object):
+class CompletionChunk:
+    class Kind:
         def __init__(self, name):
             self.name = name
 
@@ -2747,7 +2716,7 @@ def isKindResultType(self):
 
 
 class CompletionString(ClangObject):
-    class Availability(object):
+    class Availability:
         def __init__(self, name):
             self.name = name
 
@@ -2849,7 +2818,7 @@ def results(self):
 
     @property
     def diagnostics(self):
-        class DiagnosticsItr(object):
+        class DiagnosticsItr:
             def __init__(self, ccr):
                 self.ccr = ccr
 
@@ -3003,13 +2972,13 @@ def from_source(
                 if hasattr(contents, "read"):
                     contents = contents.read()
                 contents = b(contents)
-                unsaved_array[i].name = b(fspath(name))
+                unsaved_array[i].name = b(os.fspath(name))
                 unsaved_array[i].contents = contents
                 unsaved_array[i].length = len(contents)
 
         ptr = conf.lib.clang_parseTranslationUnit(
             index,
-            fspath(filename) if filename is not None else None,
+            os.fspath(filename) if filename is not None else None,
             args_array,
             len(args),
             unsaved_array,
@@ -3040,7 +3009,7 @@ def from_ast_file(cls, filename, index=None):
         if index is None:
             index = Index.create()
 
-        ptr = conf.lib.clang_createTranslationUnit(index, fspath(filename))
+        ptr = conf.lib.clang_createTranslationUnit(index, os.fspath(filename))
         if not ptr:
             raise TranslationUnitLoadError(filename)
 
@@ -3159,7 +3128,7 @@ def diagnostics(self):
         Return an iterable (and indexable) object containing the diagnostics.
         """
 
-        class DiagIterator(object):
+        class DiagIterator:
             def __init__(self, tu):
                 self.tu = tu
 
@@ -3193,7 +3162,7 @@ def reparse(self, unsaved_files=None, options=0):
                 if hasattr(contents, "read"):
                     contents = contents.read()
                 contents = b(contents)
-                unsaved_files_array[i].name = b(fspath(name))
+                unsaved_files_array[i].name = b(os.fspath(name))
                 unsaved_files_array[i].contents = contents
                 unsaved_files_array[i].length = len(contents)
         ptr = conf.lib.clang_reparseTranslationUnit(
@@ -3217,7 +3186,11 @@ def save(self, filename):
         """
         options = conf.lib.clang_defaultSaveOptions(self)
         result = int(
-            conf.lib.clang_saveTranslationUnit(self, fspath(filename), options)
+            conf.lib.clang_saveTranslationUnit(
+                self,
+                os.fspath(filename),
+                options,
+            )
         )
         if result != 0:
             raise TranslationUnitSaveError(result, "Error saving TranslationUnit.")
@@ -3261,12 +3234,12 @@ def codeComplete(
                 if hasattr(contents, "read"):
                     contents = contents.read()
                 contents = b(contents)
-                unsaved_files_array[i].name = b(fspath(name))
+                unsaved_files_array[i].name = b(os.fspath(name))
                 unsaved_files_array[i].contents = contents
                 unsaved_files_array[i].length = len(contents)
         ptr = conf.lib.clang_codeCompleteAt(
             self,
-            fspath(path),
+            os.fspath(path),
             line,
             column,
             unsaved_files_array,
@@ -3300,7 +3273,9 @@ class File(ClangObject):
     @staticmethod
     def from_name(translation_unit, file_name):
         """Retrieve a file handle within the given translation unit."""
-        return File(conf.lib.clang_getFile(translation_unit, fspath(file_name)))
+        return File(
+            conf.lib.clang_getFile(translation_unit, os.fspath(file_name)),
+        )
 
     @property
     def name(self):
@@ -3328,7 +3303,7 @@ def from_result(res, fn, args):
         return res
 
 
-class FileInclusion(object):
+class FileInclusion:
     """
     The FileInclusion class represents the inclusion of one source file by
     another via a '#include' directive or as the input file for the translation
@@ -3377,7 +3352,7 @@ def __init__(self, enumeration, message):
         Exception.__init__(self, "Error %d: %s" % (enumeration, message))
 
 
-class CompileCommand(object):
+class CompileCommand:
     """Represents the compile command used to build a file"""
 
     def __init__(self, cmd, ccmds):
@@ -3409,7 +3384,7 @@ def arguments(self):
             yield conf.lib.clang_CompileCommand_getArg(self.cmd, i)
 
 
-class CompileCommands(object):
+class CompileCommands:
     """
     CompileCommands is an iterable object containing all CompileCommand
     that can be used for building a specific file.
@@ -3460,7 +3435,7 @@ def fromDirectory(buildDir):
         errorCode = c_uint()
         try:
             cdb = conf.lib.clang_CompilationDatabase_fromDirectory(
-                fspath(buildDir), byref(errorCode)
+                os.fspath(buildDir), byref(errorCode)
             )
         except CompilationDatabaseError as e:
             raise CompilationDatabaseError(
@@ -3474,7 +3449,7 @@ def getCompileCommands(self, filename):
         build filename. Returns None if filename is not found in the database.
         """
         return conf.lib.clang_CompilationDatabase_getCompileCommands(
-            self, fspath(filename)
+            self, os.fspath(filename)
         )
 
     def getAllCompileCommands(self):
@@ -3865,7 +3840,7 @@ def register(item):
         register(f)
 
 
-class Config(object):
+class Config:
     library_path = None
     library_file = None
     compatibility_check = True
@@ -3880,7 +3855,7 @@ def set_library_path(path):
                 "any other functionalities in libclang."
             )
 
-        Config.library_path = fspath(path)
+        Config.library_path = os.fspath(path)
 
     @staticmethod
     def set_library_file(filename):
@@ -3891,7 +3866,7 @@ def set_library_file(filename):
                 "any other functionalities in libclang."
             )
 
-        Config.library_file = fspath(filename)
+        Config.library_file = os.fspath(filename)
 
     @staticmethod
     def set_compatibility_check(check_status):

@h-vetinari
Copy link
Contributor

This should just bump to 3.8 IMO. Anything below that is end-of-life and a waste of time to support.

@linux4life798
Copy link
Contributor Author

linux4life798 commented Jan 15, 2024

This should just bump to 3.8 IMO. Anything below that is end-of-life and a waste of time to support.

I agree and my personal plan is still to bump the version to 3.7 or 3.8, but it was more important to me to make progress towards adding any usable type annotations than it is to raise the min python version across llvm or just within libclang. I figured this incremental change was safe, couldn't be blocked based on min version, and doesn't introduce any syntax that has been obsoleted in later python. Feel free to checkout #77219, too.

Let me know if you think there is something I could do to help progress the transition to python min version 3.8 (or 3.7).

@Endilll
Copy link
Contributor

Endilll commented Jan 17, 2024

Looks sensible, but I'm not experienced enough with migration from Python 2 to 3 to approve.

Copy link
Collaborator

@tru tru left a comment

Choose a reason for hiding this comment

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

LGTM.

@linux4life798
Copy link
Contributor Author

Thanks all! @boomanaiden154 or @tru, could you merge this PR?

@boomanaiden154 boomanaiden154 merged commit 75f2321 into llvm:main Jan 17, 2024
@linux4life798 linux4life798 deleted the remove-python2-bump-python3.6 branch January 17, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants