Use existing AvailabilityKind enum for code completion availability

[DeinAlptraum]

This adresses point 4 from #156680. This is a necessary step before CompletionChunk.Kind can be removed.

The ChunkCompletion.Kind implements __str__ and __repr__ differently from our other enum classes. I have adapted the __repr__ of CompletionString to stringify the availability of the chunk differently so that it still looks the same as before.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jannick Kremer (DeinAlptraum)

Changes

This adresses point 4 from #156680. This is a necessary step before CompletionChunk.Kind can be removed.

The ChunkCompletion.Kind implements __str__ and __repr__ differently from our other enum classes. I have adapted the __repr__ of CompletionString to stringify the availability of the chunk differently so that it still looks the same as before.

---

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

1 Files Affected:

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

diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index 13a91d83ede1c..7f2c2183ec1bb 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -3039,6 +3039,16 @@ class _CXUnsavedFile(Structure):
 }
 
 
+# Converting the new enum names (full upper-case, underscore separated)
+# to the old ones (separated by capitalization), e.g. RESULT_TYPE -> ResultType
+def _kind_to_old_name(kind: BaseEnumeration):
+    # Remove underscores
+    components = kind.name.split("_")
+    # Upper-camel case each split component
+    components = [component.lower().capitalize() for component in components]
+    return "".join(components)
+
+
 class CompletionChunk:
     class Kind:
         def __init__(self, name: str):
@@ -3165,9 +3175,9 @@ def priority(self) -> int:
         return conf.lib.clang_getCompletionPriority(self.obj)  # type: ignore [no-any-return]
 
     @property
-    def availability(self) -> CompletionChunk.Kind:
+    def availability(self) -> AvailabilityKind:
         res = conf.lib.clang_getCompletionAvailability(self.obj)
-        return availabilityKinds[res]
+        return AvailabilityKind.from_id(res)
 
     @property
     def briefComment(self) -> str:
@@ -3179,20 +3189,12 @@ def __repr__(self) -> str:
             + " || Priority: "
             + str(self.priority)
             + " || Availability: "
-            + str(self.availability)
+            + _kind_to_old_name(self.availability)
             + " || Brief comment: "
             + str(self.briefComment)
         )
 
 
-availabilityKinds = {
-    0: CompletionChunk.Kind("Available"),
-    1: CompletionChunk.Kind("Deprecated"),
-    2: CompletionChunk.Kind("NotAvailable"),
-    3: CompletionChunk.Kind("NotAccessible"),
-}
-
-
 class CodeCompletionResult(Structure):
     _fields_ = [("cursorKind", c_int), ("completionString", c_object_p)]
 

[DeinAlptraum]

@Endilll about CompletionChunk.Kind, you wrote in the issue that we should

preserve convertibility of the new enum to the same strings users get today (e.g. "CurrentParameter"), so that nothing breaks for the current users of CompletionChunk.kind;

This is not entirely possible: since we want to switch to returning the proper AvailabilityKind enum variants rather than this replication building on CompletionChunk.Kind that we've had so far, we cannot control how this is printed: the result of CompletionString.availability will of course print the same as our other enums, leading to the following differences:

      __str__                         __repr__
Old   NotAccessible                   <ChunkKind: NotAccessible>
New   AvailabilityKind.NOT_ACCESSIBLE AvailabilityKind.NOT_ACCESSIBLE

As mentioned above, I changed the __repr__ of CompletionString to adapt the spelling of AvaialabilityKind so it still prints the same as before, but I'm not sure that keeping just this part the same as before really makes this much better... what do you think?

[Endilll]

Can you also add a test to ensure that nothing breaks?

[Endilll]

we cannot control how this is printed

Is there anything preventing us from providing our own __str__ for AvailabilityKind? https://godbolt.org/z/3W4P9n3hK

[DeinAlptraum]

Is there anything preventing us from providing our own __str__ for AvailabilityKind?

There are currently two ways to get AvailabilityKinds out of the API (except for intializing them yourself): accessing the availability property on a Cursor, or the property of the same name on a CompletionString.

Changing the __str__ of AvailabilityKind directly would mean a breaking change over there instead.

[DeinAlptraum]

Can you also add a test to ensure that nothing breaks?

What would you like to be tested?
We already have tests asserting exact CompletionStrings, see here:

llvm-project/clang/bindings/python/tests/cindex/test_code_completion.py

Lines 51 to 55 in 077a796

	expected = [
	"{'int', ResultType} | {'test1', TypedText} || Priority: 50 || Availability: Available || Brief comment: Aaa.",
	"{'void', ResultType} | {'test2', TypedText} | {'(', LeftParen} | {')', RightParen} || Priority: 50 || Availability: Available || Brief comment: Bbb.",
	"{'return', TypedText} | {';', SemiColon} || Priority: 40 || Availability: Available || Brief comment: ",
	]

which I used to ensure that the format is correct (I kept adapting the converter function above until these tests passed)

[DeinAlptraum]

I think it's also worth asking whether we want to go with this approach at all: this is already a breaking change w.r.t. the string representations of the availability attribute of CompletionString.
What's our plan with AvailabilityKind as it is presented in CompletionString's __repr__? Do we want to eventually change this to use the same string representation as all other kind enums? Should it remain like this forever?
In the former case it might make sense to do this breaking changes together, but I'm really not sure what's the best option here anyway

[Endilll]

Now that this function has a new name, I found myself confused why it takes an enumerator and extracts a string out of it. I think it should just take a string instead.

[Endilll]

There are currently two ways to get AvailabilityKinds out of the API (except for intializing them yourself): accessing the availability property on a Cursor, or the property of the same name on a CompletionString.

Changing the __str__ of AvailabilityKind directly would mean a breaking change over there instead.

Fair.

What would you like to be tested?

I believe that today there are only three ways to work with the return value of CompletionString.availability:

1. Call CompletionChunk.Kind.__repr__. It returns "<ChunkKind: %s>" % self, which I hope is only used for debugging purposes.
2. Call CompletionString.__repr__. The long string that it returns is also tailored for humans, and is not suitable to be used for anything else, even if a user would try to parse it.
3. Call CompletionChunk.Kind.__str__. I think this is what is predominantly used today, because it's at least sane to use: str(completion_string.availability) == "NotAccessible".

(Note that they are not even equality comparable, so they have to be converted to strings: str(completion_string.availability) == str(completion_string2.availability).)

This third usage is the one we should care about, and the one I'd like to be tested.

What's our plan with AvailabilityKind as it is presented in CompletionString's repr?

Consequently, I don't think we need to be concerned with the exact output of either __repr__ functions.

I think it's also worth asking whether we want to go with this approach at all: this is already a breaking change w.r.t. the string representations of the availability attribute of CompletionString.

I think we need AvailabilityKindCompat derived from AvailabilityKind. It should do two more things on top of being AvailabilityKind:

1. It should have __str__ that returns camel case names, like CompletionChunk.Kind.__str__ today, and emit DeprecationWarning. This way backwards compatibility is preserved.
2. It should be equality comparable with AvailabilityKind, so that when we replace AvailabilityKindCompat with AvailabilityKind, nothing breaks. This way we have a path forward.

CompletionString.availability should return AvailabilityKindCompat during deprecation period, then removed. CompletionString.Availiability (the type) is not used today and is not going to be, so we should remove it. availabilityKind should also go away, but you've already done that.