[libclang/python] Add equality comparison operators for File #130383

DeinAlptraum · 2025-03-08T03:01:25Z

This covers the File interface changes added by #120590

llvmbot · 2025-03-08T03:02:02Z

@llvm/pr-subscribers-clang

Author: Jannick Kremer (DeinAlptraum)

Changes

This covers the File interface changes added by #120590

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

3 Files Affected:

(modified) clang/bindings/python/clang/cindex.py (+7)
(modified) clang/bindings/python/tests/cindex/test_file.py (+15)
(modified) clang/docs/ReleaseNotes.rst (+1)

diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index 879a0a3c5c58c..bc8fb7c27fdac 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -3470,6 +3470,12 @@ def __str__(self):
     def __repr__(self):
         return "<File: %s>" % (self.name)
 
+    def __eq__(self, other) -> bool:
+        return isinstance(other, File) and bool(conf.lib.clang_File_isEqual(self, other))
+
+    def __ne__(self, other) -> bool:
+        return not self.__eq__(other)
+
     @staticmethod
     def from_result(res, arg):
         assert isinstance(res, c_object_p)
@@ -3956,6 +3962,7 @@ def set_property(self, property, value):
     ("clang_getFile", [TranslationUnit, c_interop_string], c_object_p),
     ("clang_getFileName", [File], _CXString),
     ("clang_getFileTime", [File], c_uint),
+    ("clang_File_isEqual", [File, File], bool),
     ("clang_getIBOutletCollectionType", [Cursor], Type),
     ("clang_getIncludedFile", [Cursor], c_object_p),
     (
diff --git a/clang/bindings/python/tests/cindex/test_file.py b/clang/bindings/python/tests/cindex/test_file.py
index 14a3084ee2b47..0e8431054e951 100644
--- a/clang/bindings/python/tests/cindex/test_file.py
+++ b/clang/bindings/python/tests/cindex/test_file.py
@@ -16,3 +16,18 @@ def test_file(self):
         self.assertEqual(str(file), "t.c")
         self.assertEqual(file.name, "t.c")
         self.assertEqual(repr(file), "<File: t.c>")
+
+    def test_file_eq(self):
+        index = Index.create()
+        tu = index.parse(
+            "t.c",
+            unsaved_files=[
+                ("t.c", "int a = 729;"),
+                ("s.c", "int a = 729;"),
+            ],
+        )
+        file1 = File.from_name(tu, "t.c")
+        file2 = File.from_name(tu, "s.c")
+        self.assertEqual(file1, file1)
+        self.assertNotEqual(file1, file2)
+        self.assertNotEqual(file1, "t.c")
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a25808e36bd51..a2f10f20ce716 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -444,6 +444,7 @@ Python Binding Changes
 ----------------------
 - Added ``Type.get_methods``, a binding for ``clang_visitCXXMethods``, which
   allows visiting the methods of a class.
+- Add equality comparison operators for ``File`` type
 
 OpenMP Support
 --------------

github-actions · 2025-03-08T03:04:50Z

✅ With the latest revision this PR passed the Python code formatter.

DeinAlptraum · 2025-03-08T04:17:09Z

@Endilll do you understand where this test failure might come from? It passes for me locally...

Endilll · 2025-03-08T15:11:31Z

I think the failure in question is

 ======================================================================
FAIL: test_file_eq (tests.cindex.test_file.TestFile.test_file_eq)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/llvm-project/llvm-project/clang/bindings/python/tests/cindex/test_file.py", line 32, in test_file_eq
    self.assertNotEqual(file1, file2)
AssertionError: <File: t.c> == <File: s.c>

----------------------------------------------------------------------

and I have to idea why this happens.

DeinAlptraum · 2025-03-20T04:28:37Z

@Endilll I managed to align my local errors with those in the CI and fix them by using actual files for the test instead... curious where the issue comes from, but I don't feel like looking into this deeper rn.
Anyway, this is ready for review now. Sorry for the delay.

clang/bindings/python/tests/cindex/test_file.py

clang/docs/ReleaseNotes.rst

Endilll

@AaronBallman do you have a clue why equality comparison for in-memory files works differently compared to on-disk files?

clang/bindings/python/tests/cindex/test_file.py

github-actions · 2025-03-22T11:03:49Z

✅ With the latest revision this PR passed the C/C++ code formatter.

DeinAlptraum · 2025-03-22T11:50:07Z

As for investigation:
I wrote four tests in test_file.py that just assert all files as equal (the ones named test_file_eq_failing...) which is expected to fail, and all four of these tests fail for me locally. But in the CI the first three (using in-memory files) succeed, so it seems that indeed all in-memory files seem to be treated as equal. The fourth one using files read from disk correctly treats them as unique and thus fails.

AaronBallman · 2025-03-24T11:53:52Z

@AaronBallman do you have a clue why equality comparison for in-memory files works differently compared to on-disk files?

Are they? I think you're supposed to use FileSystem::equivalent() to test for it which uses an underlying unique ID.

DeinAlptraum · 2025-03-24T13:13:03Z

clang/bindings/python/tests/cindex/test_file.py

+    def test_file_eq_failing(self):
+        index = Index.create()
+        tu = index.parse(
+            "t.c",
+            unsaved_files=[
+                ("t.c", "int a = 729;"),
+                ("s.c", "int a = 729;"),
+            ],
+        )
+        file1 = File.from_name(tu, "t.c")
+        file2 = File.from_name(tu, "s.c")
+        # FIXME: These files are not supposed to be equal
+        self.assertEqual(file1, file2)
+
+    def test_file_eq_failing_2(self):
+        index = Index.create()
+        tu = index.parse(
+            "t.c",
+            unsaved_files=[
+                ("t.c", "int a = 729;"),
+                ("s.c", "int a = 728;"),
+            ],
+        )
+        file1 = File.from_name(tu, "t.c")
+        file2 = File.from_name(tu, "s.c")
+        # FIXME: These files are not supposed to be equal
+        self.assertEqual(file1, file2)
+
+    def test_file_eq_failing_3(self):
+        index = Index.create()
+        tu = index.parse(
+            "t.c",
+            unsaved_files=[
+                ("t.c", '#include "a.c"\n#include "b.c";'),
+                ("a.c", "int a = 729;"),
+                ("b.c", "int b = 729;"),
+            ],
+        )
+        file1 = File.from_name(tu, "t.c")
+        file2 = File.from_name(tu, "a.c")
+        file3 = File.from_name(tu, "b.c")
+        # FIXME: These files are not supposed to be equal
+        self.assertEqual(file2, file3)
+        self.assertEqual(file1, file2)
+        self.assertEqual(file1, file3)
+
+    def test_file_eq_failing_4(self):
+        path = os.path.join(inputs_dir, "testfile.c")
+        path_a = os.path.join(inputs_dir, "a.inc")
+        path_b = os.path.join(inputs_dir, "b.inc")
+        tu = TranslationUnit.from_source(path)
+        print(tu.spelling, tu.cursor.spelling)
+        file1 = File.from_name(tu, path)
+        file2 = File.from_name(tu, path_a)
+        file3 = File.from_name(tu, path_b)
+        # FIXME: These files are not supposed to be equal
+        self.assertEqual(file2, file3)
+        self.assertEqual(file1, file2)
+        self.assertEqual(file1, file3)


@AaronBallman take a look at the tests I wrote here. At least in the CI, only the last one is actually failing. From the first three, it looks like all in-memory files are considered equal, but test_file_eq_failing_4 which reads them from disk doesn't have that problem.

Even weirder, all four of these tests fail when I run them locally.

I think this might be a bug here:

llvm-project/clang/tools/libclang/CIndex.cpp

Line 5173 in 1b07e86

return FEnt1.getUniqueID() == FEnt2.getUniqueID();

For starters, FileEntryRef has an equality operator:

llvm-project/clang/include/clang/Basic/FileEntry.h

Line 89 in 1b07e86

friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {

so it's worth seeing if using that gives the correct behavior.

But also, you can use FileManager::getNoncachedStatValue to get a vfs::Status object on which you can call equivalent() to test for equivalency, so it's worth seeing if that gives the correct behavior.

@AaronBallman sorry for the delay and thanks a lot for the suggestions!
Changing the line you mentioned to use the equality operator instead indeed seems to result in the correct behavior! It seems like file equality doesn't have any tests in LibclangTest.cpp either, so I'll add one for this and open a PR later.

Opened a PR at #135773

Co-authored-by: Vlad Serebrennikov <[email protected]>

DeinAlptraum · 2025-04-24T05:21:55Z

@Endilll having merged #135773 and rebased, the issues with file equality for in-memory files are resolved.
I've reduced this to two tests now, that run on the except same C code except one of them uses on-disk files and the other one uses in-memory files. This replicates pretty much exactly how we test clang_File_isEqual on C++ side.

clang/bindings/python/tests/cindex/INPUTS/a.inc

…0383) This covers the `File` interface changes added by llvm#120590 --------- Co-authored-by: Mathias Stearn <[email protected]> Co-authored-by: Vlad Serebrennikov <[email protected]>

DeinAlptraum requested a review from Endilll March 8, 2025 03:01

llvmbot added clang Clang issues not falling into any other category clang:as-a-library libclang and C++ API labels Mar 8, 2025

Endilll reviewed Mar 20, 2025

View reviewed changes

clang/bindings/python/tests/cindex/test_file.py Outdated Show resolved Hide resolved

clang/bindings/python/tests/cindex/test_file.py Show resolved Hide resolved

clang/docs/ReleaseNotes.rst Show resolved Hide resolved

DeinAlptraum force-pushed the 120590-fileinterface branch from 4cc056c to b8a4f30 Compare March 22, 2025 02:52

Endilll reviewed Mar 22, 2025

View reviewed changes

clang/bindings/python/tests/cindex/test_file.py Show resolved Hide resolved

clang/bindings/python/tests/cindex/test_file.py Outdated Show resolved Hide resolved

clang/bindings/python/tests/cindex/test_file.py Outdated Show resolved Hide resolved

DeinAlptraum commented Mar 24, 2025

View reviewed changes

DeinAlptraum mentioned this pull request Apr 15, 2025

[libclang/C++] Fix clang_File_isEqual for in-memory files #135773

Merged

RedBeard0531 and others added 10 commits April 24, 2025 11:44

[libclang/python] Add equality comparison operators for File

1e6ed02

Use existing test input files instead of in-memory files

4b72ede

Fix formatting

d1da5fa

Adress review comments

c02dcc3

Update clang/bindings/python/tests/cindex/test_file.py

1a439b7

Co-authored-by: Vlad Serebrennikov <[email protected]>

Update clang/bindings/python/tests/cindex/test_file.py

acfa2de

Co-authored-by: Vlad Serebrennikov <[email protected]>

Add example test

40acc19

Fix test error

aff6bfe

Fix test again

f0bc427

Add proper tests for on-disk and in-memory files

bb4e33e

DeinAlptraum force-pushed the 120590-fileinterface branch from f0951dd to bb4e33e Compare April 24, 2025 02:47

Fix formatting, remove superfluous print statements

ce73282

Endilll approved these changes Apr 24, 2025

View reviewed changes

clang/bindings/python/tests/cindex/INPUTS/a.inc Outdated Show resolved Hide resolved

Add newlines at end of test files

ae4aab5

DeinAlptraum merged commit c6c0846 into llvm:main Apr 24, 2025
9 of 13 checks passed

DeinAlptraum deleted the 120590-fileinterface branch April 24, 2025 09:16

DeinAlptraum mentioned this pull request May 16, 2025

[libclang/python] Add a few things to the python api #120590

Closed

[libclang/python] Add equality comparison operators for File #130383

[libclang/python] Add equality comparison operators for File #130383

Uh oh!

Conversation

DeinAlptraum commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 8, 2025

Uh oh!

github-actions bot commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeinAlptraum commented Mar 8, 2025

Uh oh!

Endilll commented Mar 8, 2025

Uh oh!

DeinAlptraum commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Endilll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeinAlptraum commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Mar 24, 2025

Uh oh!

DeinAlptraum Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

AaronBallman Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

DeinAlptraum Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

DeinAlptraum Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

DeinAlptraum commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DeinAlptraum commented Mar 8, 2025 •

edited

Loading

github-actions bot commented Mar 8, 2025 •

edited

Loading

github-actions bot commented Mar 22, 2025 •

edited

Loading

DeinAlptraum commented Mar 22, 2025 •

edited

Loading