[cfe-commits] [PATCH] [cindex.py] Do not fail when registering non-existing functions

[Tobias Grosser]

On 08/25/2012 08:45 AM, Gregory Szorc wrote:
> I agree with the sentiment of wanting increased compatibility, but I'm
> hesitant that this change is the right answer.

Hi Gregory,

thanks for the feedback.

> IMO the version compatibility story should be "use the same revision
> number for both libclang and the Python bindings." If the revision of
> libclang and the Python bindings varies, there is a potential for them
> to be out of sync, for backwards incompatibility to cause
> unexpected/undefined behavior, etc. This is dangerous. (I would
> actually like to have a feature in the Python bindings where they
> (optionally) refuse to talk to a libclang that was built from a
> different revision of the source tree than the bindings.) If we were
> to commit this patch, we are effectively saying that we /think/ the
> bindings /might/ be compatible with your version of libclang. But, we
> really don't know what features are supported or if they will work
> correctly. If we were to take this patch, we'd also need a
> compatibility story. What if a user of Clang 2.9 complains about the
> current trunk bindings not working? Are we obligated to support them?
> What about a 3.0 user? 3.1? Where do you draw the line? We have a
> clear line today: use the same version control revision. This patch
> removes that and introduces a lot of ambiguity.

You seem to have a strong position when you state that the only approach 
that should be used is: 'use the same revision than clang'

I understand your goal of keeping the python bindings in sync with 
libclang. As you explained, in an ideal world the python bindings mirror 
the features of libclang, they are tested before every release and they 
are released _and_ installed with clang. I believe our own goal (and 
also what we claim to support) should be to get as close as possible to 
such an ideal world.

Unfortunately we are not in an ideal world. In the last releases 
(2.8/2.9/3.0/3.1) the python bindings have been lacking behind libclang,
they have not been properly tested, have sometimes been buggy and they 
have also never been installed. We can definitely improve on this by 
adding more test cases (e.g. code completion) or by installing the 
python bindings with clang. However, I honestly doubt we will manage to 
develop libclang and the python bindings in sync. In the last years, 
features to the python bindings have mostly been added (and tested) when 
a tool using such features was developed. This happened often after 
releases, meaning older libclang.so versions often support a feature, 
but the python bindings from that point did not yet support it.

Tools that use the python bindings need to work in this non-ideal world.
clang_complete (but also SublimeClang [1]) never followed the 'use the 
same revision than clang' version rule. Instead they have been bundling 
their own version of the python bindings since they started. Like this 
they are able to work with many of the existing clang installations, 
which - in case of smaller plugins - is a big plus.
clang_complete e.g. currently bundles cindex.py bindings from around 
clang 3.0. This allows it to provide code completion with clang 3.0 and 
newer. Unfortunately the current bindings fail to load as soon as a 
libclang function does not exist. This means clang_complete does not 
support clang 2.8 / 2.9 and it can also not move to newer bindings and 
their features without breaking support for clang 3.0.
For code completion the only reason of incompatibility is the 
initialization. With the proposed patch applied, the most recent 
cindex.py version can provide both code completion and error 
highlighting for clang 2.8, 2.9, 3.0, 3.1 and trunk. What I want to say: 
This patch is not only about some small performance improvements, but it 
enables the use of the current python bindings on a wide range of 
libclang versions.

However, this is probably what you are afraid of: Using the very same 
python binding for a wide range of clang versions. Something that is not 
officially supported and that is not covered by test cases. I understand 
that there is some risk and that there may be compatibility issues. 
However, I don't believe this is a no-go. For code completion the single 
cindex.py approach worked out well. This is becasue libclang itself is 
very stable and changes to it are generally additions of new 
functionality. The semantics of existing functions do not change. There 
may still be backward compatibility issues in some border cases, but in 
general newer bindings are highly backward compatible. Obviously, such 
compatibility claims are not checked for larger parts of cindex.py and I 
don't believe the libclang developers should spent time on compatibility 
testing or should feel themselves forced to remain compatible. However, 
I don't see a problem, if external users test for backward compatibility 
and take advantage of backward compatibility, if it is exists.

> I understand that you are trying to enable users of old Clang to reap
> the recent performance improvements of the Python bindings without
> having to upgrade Clang. This is a noble goal. However, this is risky.
> Instead, I think this decision should be left to downstream users.
> Backporting individual patches is relatively simple. And, the test
> suite does give some peace of mind (although the code complete APIs
> don't have tests, sadly). Alternatively, if Clang maintained its
> release branches post-release, performance patches like the recent
> code complete ones would likely be obvious candidates for backport. If
> nothing else, some power user could maintain her own branch of
> bindings with backported trunk changes that she guarantees to work
> with libclang X.Y. I believe that the trunk bindings should only
> provide compatibility with trunk libclang.

Who are the downstream users? And what decisions does this patch take 
that should be left to the downstream users?

Regarding maintaining post release branches for each release: I think it 
is a frightening idea. clang and LLVM have no release branches, because 
they add a lot of maintenance overhead. Asking each project to maintain 
post-release branches individually will cause even more overhead. For 
cases where this overhead can not be avoided, we may have no choice, but 
I don't see any reason to force every user to maintain post release 
branches. Automatic code completion works well with a single cindex.py 
file and the projects that bundle cindex.py for this use case perform 
happily the compatibility testing. They are in some way the power users 
you are talking about. Do you see a problem if they decide a single 
cindex.py works well for them?

Even if we apply this patch, we could still look into some way to give 
the user optionally stronger guarantees for his python bindings. 
Checking if the bindings and the installed libclang.so are in sync is 
one way (we need to cover here also the private Apple releases, a simple 
version number checker does not work). Another idea is to check 
compatibility based on features. We could have simple tests that check 
if a specific libclang version supports feature X (e.g. the 
CompilationDatabase). In case the version Apple cut supports that 
feature, we would return yes, otherwise no. In case an unsupported 
feature would be used anyway, the python bindings could throw a 
descriptive exception.

To subsume: I think there are different use cases and this patch 
improves a important one.

Cheers
Tobi

[1] https://github.com/quarnster/SublimeClang