parser: Check for null cursors from get_tokens

[heftig]

Clang 21's cindex.py should be replacing all returned null cursors with None. However, due to a bug (as of 21.1.3) it still returns null cursors from tu.get_tokens.

We need to explicitly check for these because cindex.py 21 also raises an exception when any property of a null cursor is accessed.

File "hawkmoth/parser.py", line 169, in _comment_extract
  if token_cursor.kind in [CursorKind.INVALID_FILE,
     ^^^^^^^^^^^^^^^^^
File "clang/cindex.py", line 1578, in inner
  raise Exception("Tried calling method on a null-cursor.")

[jnikula]

Thanks for the fix, I can reproduce. The test suite fails all over the place running clang 21 (in e.g. Fedora Rawhide).

Looks like this was introduced by @DeinAlptraum in llvm/llvm-project#138103.

I honestly hate having to do getattr(token_cursor, "is_null", lambda: False)() to be compatible with multiple clang versions. Would be nice to know what the idea was here. What are we supposed to be doing?

cindex.py says null cursors are translated to None. but we never hit None cursors, and adding a token_cursor is None check isn't enough.

[jnikula]

In particular, this cursor_null_guard() documentation in cindex.py doesn't appear to be true for cursors originating from Token.cursor:

    This decorator is used to ensure that no methods are called on null-cursors.
    The bindings map null cursors to `None`, so users are not expected
    to encounter them.

[DeinAlptraum]

Hi, I'm currently more or less the maintainer of the libclang-python bindings. We don't usually hear about how people use them (or if they use them at all) so I'm always happy to get feedback like this.

Re: null cursor treatment
We previously had a very inconsistent effort across the bindings to convert null Cursors to None, so users don't have to deal with null cursors. This wasn't seen through completely and led to a half-assed situation where users would have to deal both with null cursors in some cases (which do not behave sensibly for some methods) and null Cursors turned None methods. This makes both the usage of the Cursor class difficult due to inconsistency, and complicates the type annotation efforts I'm currently going through, which is why we wanted to unify this.

We thus tried to fully go the "null Cursors are interpreted as None" route for consistency, and then aim to generally refuse processing null Cursors as far as possible to prevent users from having to deal with weird behaviors.

I honestly hate having to do getattr(token_cursor, "is_null", lambda: False)() to be compatible with multiple clang versions

Yes, this absolutely shouldn't happen and is what we tried to avoid

adding a token_cursor is None check isn't enough

This was what we were aiming for: all methods (properties) being safe and working as intended on actual Cursor objects, only having to check for None.
It sounds like there's something that still returns null Cursors (instead of turning them into None) that we overlooked. If you have a code example to reproduce this, I'd be happy if you could share it (or better yet, open a bug report over at LLVM)

[jnikula]

Hi, I'm currently more or less the maintainer of the libclang-python bindings. We don't usually hear about how people use them (or if they use them at all) so I'm always happy to get feedback like this.

Hey, thanks a lot for chiming in! We hit issues with libclang and/or the bindings every once in a while, so it's nice to know who to try to reach out to! 😉

This was what we were aiming for: all methods (properties) being safe and working as intended on actual Cursor objects, only having to check for None. It sounds like there's something that still returns null Cursors (instead of turning them into None) that we overlooked.

The goal seems sane.

I just edited #291 (comment) to add that the null Cursors appear to originate from Token.cursor, and at a quick look at the current cindex.py source, I don't see any translation to None.

If you have a code example to reproduce this, I'd be happy if you could share it (or better yet, open a bug report over at LLVM)

Currently, it's basically running . venv and make test in this repo with clang 21... I don't have the time right now to create a minimal reproducer... but it's basically this loop https://github.com/jnikula/hawkmoth/blob/master/src/hawkmoth/parser.py#L154

    for token in tu.get_tokens(extent=tu.cursor.extent):
        # ...
        token_cursor = token.cursor

        if token_cursor.kind in [...]:
        # the above hits the null cursor check

I'm assuming tu.get_tokens() simply returns tokens that have null Cursors, and considering our test suite fails almost all test cases now, it's not an edge case.

[DeinAlptraum]

Thanks, I was able to reproduce this.
I opened an issue and a fix PR.
The fix is simple and getting this merged will be easy, but I'm not familiar with how our release cycles work, so I have no idea when you will actually see the fix.

[Endilll]

but I'm not familiar with how our release cycles work, so I have no idea when you will actually see the fix.

(Formal Python bindings maintainer here.)
I approved the fix, so it will be in main as soon as you merge it. At worst it will be a part of Clang 22, which will be released around March 2023. It will arrive earlier if backported to 21.x branch. My default position on backports is "no", but I'll talk to other maintainers and let you know if this can be done.

[jnikula]

Thanks, I was able to reproduce this. I opened an issue and a fix PR. The fix is simple and getting this merged will be easy, but I'm not familiar with how our release cycles work, so I have no idea when you will actually see the fix.

Thanks @DeinAlptraum for the quick fix!

(Formal Python bindings maintainer here.) I approved the fix, so it will be in main as soon as you merge it. At worst it will be a part of Clang 22, which will be released around March 2023. It will arrive earlier if backported to 21.x branch. My default position on backports is "no", but I'll talk to other maintainers and let you know if this can be done.

Happy to get your attention too @Endilll.

Do either of you have any suggestions on how to cleanly check for null cursors in a way that's compatible across Clang versions? As said, the getattr check on is_null method is not great. Or could we reliably check for token.kind and assume some kinds will always have non-null Cursor, and others null Cursor? (I fear checking this myself from libclang source is a bit of a rabbit hole!)

Regarding backports, we might have to carry the workaround no matter what, in case someone ends up using Clang 21.x earlier than the fix, like current Fedora Rawhide (and I believe 43). But backporting would certainly help projects that have yet to hit the issue, and potentially avoid a bunch of debugging, before eventually making the same conclusions as @heftig and I.

[Endilll]

Do either of you have any suggestions on how to cleanly check for null cursors in a way that's compatible across Clang versions?

Unfortunately, I don't have any concrete suggestions. Moreover, I should say that after years of being mostly neglected, Python bindings will be changing towards more consistent API within a single version, at the expense of consistency across versions where necessary. (Whereas libclang C API will remain API- and ABI-stable, as was always promised.) I'm the one to drive this point, and I take the blame of making life harder for people who has to support multiple versions simultaneously. My hope is that the pain will subside over time as you drop support for older Clang versions, while you get rid of some technical debt that our inconsistent API imposed on you.

We do our best to keep breakages to minimum, providing users with detailed changelog of potentially breaking changes, and even deprecation periods where it makes sense. The alternative is API that can be only piled onto without ever changing what's there, like the C one, and I don't like it. Even then the behavior of the functions can change in subtle ways, because of the changes to the underlying C++ API, so the only way to robustly shield yourself from changes is to freeze the version of Clang down to specific commit.

I guess I went off-topic a bit, but that should give you an impression where Clang is headed at the moment.

[DeinAlptraum]

I don't exactly like suggesting the use of what feels like an implementation detail, but this is Python so it's not like we can stop people anyway, and the C-API should be stable...
I don't think we make any guarantees about this, but it should have been unchanged for the last 13 years or so:
You could check via myCursor == conf.lib.clang_getNullCursor(), i.e. effectively reimplementig the Cursor.is_null method by directly calling the C-API on the library object.

[Endilll]

I don't exactly like suggesting the use of what feels like an implementation detail, but this is Python so it's not like we can stop people anyway, and the C-API should be stable... I don't think we make any guarantees about this, but it should have been unchanged for the last 13 years or so: You could check via myCursor == conf.lib.clang_getNullCursor(), i.e. effectively reimplementig the Cursor.is_null method by directly calling the C-API on the library object.

Sounds like another argument to support low-level API in Python bindings.

[jnikula]

@heftig I'm leaning towards doing something like this:

diff --git a/src/hawkmoth/parser.py b/src/hawkmoth/parser.py
index bb2b80a0ec55..b95625ae4ec3 100644
--- a/src/hawkmoth/parser.py
+++ b/src/hawkmoth/parser.py
@@ -39,6 +39,7 @@ from clang.cindex import (
     TranslationUnit,
     TranslationUnitLoadError,
     Diagnostic,
+    conf,
 )
 
 from hawkmoth import docstring
@@ -151,6 +152,8 @@ def _comment_extract(tu):
 
     def is_doc(cursor): return cursor and docstring.Docstring.is_doc(cursor.spelling)
 
+    def is_null(cursor): return cursor == conf.lib.clang_getNullCursor()
+
     for token in tu.get_tokens(extent=tu.cursor.extent):
         # Handle all comments we come across.
         if token.kind == TokenKind.COMMENT:
@@ -163,6 +166,8 @@ def _comment_extract(tu):
         # Store off the token's cursor for a slight performance improvement
         # instead of accessing the `cursor` property multiple times.
         token_cursor = token.cursor
+        if token_cursor is None or is_null(token_cursor):
+            continue
 
         # Cursors that are 1) never documented themselves, and 2) allowed
         # between the comment and the actual cursor being documented.

The idea is two-fold. First, on Clang 22+ (and possibly 21.x if the fix is backported), it'll short-circuit on is None with @DeinAlptraum's bindings fix. Second, the is_null() check will hopefully make everything behave the same across all Clang versions, so if this unearths some other issues, we'll find out about them before Clang 21.x/22.

The is_null() helper could use a brief comment about all this.

[jnikula]

@heftig Will you update the pull request, or shall I?

[jnikula]

Thanks for the follow-up @heftig, merged!

[DeinAlptraum]

FYI: the fix has been released with LLVM 21.1.4 yesterday
https://discourse.llvm.org/t/llvm-21-1-4-released/88651