[clangd] [C++20] [Modules] Code Intelligence is broken for named modules

[ChuanqiXu9]

Currently, when we write codes in named modules, we can't get help from code intelligence, which is bad. We need try to support code intelligence for named modules.

[llvmbot]

@llvm/issue-subscribers-clang-modules

[HighCommander4]

Please see clangd/clangd#1293

[ChuanqiXu9]

Please see clangd/clangd#1293

There are some slight differences. Modules has a lot of meanings. And this issue mainly cares about the named modules. So this issue should be a subset of issue of clangd1293

[ChuanqiXu9]

Please see clangd/clangd#1293

BTW, there is an another thing confusing me. Where do we (the clangd developers) report issues? In the llvm issues or in the clangd issues you give?

[HighCommander4]

So this issue should be a subset of issue of clangd1293

I agree.

BTW, there is an another thing confusing me. Where do we (the clangd developers) report issues? In the llvm issues or in the clangd issues you give?

The clangd issue tracker pre-dates the entire LLVM project moving off of Bugzilla, and tends to be where most users report issues.

[ChuanqiXu9]

So this issue should be a subset of issue of clangd1293

I agree.

BTW, there is an another thing confusing me. Where do we (the clangd developers) report issues? In the llvm issues or in the clangd issues you give?

The clangd issue tracker pre-dates the entire LLVM project moving off of Bugzilla, and tends to be where most users report issues.

Got it. Thanks. I am looking for to support named modules in clangd for clang.

[ChuanqiXu9]

@HighCommander4 Hi, may I ask how can I add a test case from in clangd? From my explanation, the clangd can support the following case (although badly):

/// A.cppm
module;
#include <cstdio>
export module A;
export void printA() {
    std::printf("A\n");
}

/// B.cppm
module;
#include <cstdio>
export module B;
export void printB() {
   std::printf("B\n");
}

// main.cpp
import A;
import B;
int main() {
    return 0;
}

// compile_commands.json
[
    {
      "directory": "/home/chuanqi.xcq/workspace.xuchuanqi/cxx-modules-examples/duplicate_header",
      "command": "/home/chuanqi.xcq/workspace.xuchuanqi/llvm-project-for-work/build/bin/clang++  -fprebuilt-module-path=/home/chuanqi.xcq/workspace.xuchuanqi/cxx-modules-examples/duplicate_header -std=c++20 -o /home/chuanqi.xcq/workspace.xuchuanqi/cxx-modules-examples/duplicate_header/main.cpp.o -c /home/chuanqi.xcq/workspace.xuchuanqi/cxx-modules-examples/duplicate_header/main.cpp -I.",
      "file": "/home/chuanqi.xcq/workspace.xuchuanqi/cxx-modules-examples/duplicate_header/main.cpp"
    }
]

And we need to compile the small project manually by (the implicit build strategy needs more discussion and which is longer to be land):

clang++ -std=c++20 --precompile -c A.cppm -o A.pcm
clang++ -std=c++20 --precompile -c B.cppm -o B.pcm
clang++ -std=c++20 -fprebuilt-module-path=. main.cpp A.pcm B.pcm

Then in main.cpp, when I type p, the clangd can:

• Suggest printA() and printB() expectedly (good)
• Shows many symbols which shouldn't show up in modules (Bad)

In modules, macros can't be found by the importee and the importee can only see the exported names. And I am looking for how to support it in clangd.

And I want to sent this example as a test case to clangd to make sure that we can still see the exported names (good) to prevent any accidental changes. And here is my question. I looked at the examples in clangd, but it is far different from those in clang, which is simply sources. But the tests of clangd is full of json and I don't know where can I start.

(If it is pretty simple to you, it'll be much appreciated if you commit it directly and I can learn from your example)

Thanks

[HighCommander4]

@ChuanqiXu9 I actually can't reproduce your issue locally. When I set up a workspace with the files you describe, and I perform completion after pr, the only proposals I get are printA(), printB(), and __PRETTY_FUNCTION__. (The last one is a built-in macro so it seems reasonable.)

In your screenshot, main.cpp also contains #include "foo.h". What are the contents of "foo.h"? Maybe the PRI... macros are coming from there.

[ChuanqiXu9]

@ChuanqiXu9 I actually can't reproduce your issue locally. When I set up a workspace with the files you describe, and I perform completion after pr, the only proposals I get are printA(), printB(), and __PRETTY_FUNCTION__. (The last one is a built-in macro so it seems reasonable.)

In your screenshot, main.cpp also contains #include "foo.h". What are the contents of "foo.h"? Maybe the PRI... macros are coming from there.

Hi, foo.h is a trivial one:

inline void func_foo() {}

I want to test the normal includes works too. So PRI may not come from them. In fact, I can get other suggestion from STL too:

And if I click on the std::priority_queue, the source would be rewritten to:

It surprises me it would add #include <queue> automatically in the top of the file. So maybe the reason come from the settings?

My clang and clangd is trunk 674a17e
And the vscode-clangd is download in the market place and I redirect the path to clangd to the one I compiled.

---

But after all, it looks good to me that you can reproduce the result that we can see printA() and printB() in main.cpp. It looks pretty good. And my intention is to add a test case for this to avoid accidental changes (we can see printA() and printB() in the importee.)

[ChuanqiXu9]

Oh, I found where the PRId* comes from. It comes from the C99 standard. It is defined in #include <stdint.h>.

The following macros are defined:

Macros:

    PRId8
    PRId16
    PRId32
    PRId64

    PRIdLEAST8
    PRIdLEAST16
    PRIdLEAST32
    PRIdLEAST64

    PRIdFAST8
    PRIdFAST16
    PRIdFAST32
    PRIdFAST64
...

If we relates this to the suggestion for std::priority_queue and std::promise, the answer should be that my clangd can suggest the symbols in the standard library even if I don't include the corresponding header. (I don't know the reason. I don't think I did any special settings)

[HighCommander4]

the answer should be that my clangd can suggest the symbols in the standard library even if I don't include the corresponding header

You're right. Clangd trunk has an "index the standard library" feature, where it indexes the standard library headers in the background even if no files in your project include them yet, so that standard library symbols can be offered as completion proposals right from the start.

(I was testing with a slightly older trunk which did not have this feature enabled yet, that's why I wasn't seeing those completion proposals.)

[ChuanqiXu9]

the answer should be that my clangd can suggest the symbols in the standard library even if I don't include the corresponding header

You're right. Clangd trunk has an "index the standard library" feature, where it indexes the standard library headers in the background even if no files in your project include them yet, so that standard library symbols can be offered as completion proposals right from the start.

(I was testing with a slightly older trunk which did not have this feature enabled yet, that's why I wasn't seeing those completion proposals.)

So, if it is possible to add this example to the in-tree tests? (the #include <cstdio> should be removed, of course). I am still looking for the methods. It looks like we have some tests in the clang/unittests. But all of them only works for a single file. And I am not sure how should we test it in multiple files. In the tests directory, we have something like (

llvm-project/clang/test/Modules/Reachability-func-default-arg.cpp

Line 2 in 471f2cf

// RUN: mkdir -p %t

):

// RUN: mkdir %t

to create the temporary working directory but I don't know the similar things in unittests.

[HighCommander4]

@ChuanqiXu9 Here is an example of a multi-file code completion test for clangd, where the additional file is a header.

I haven't thought through how this might work with modules, especially prebuilt ones. Possibly the test infrastructure needs some changes to make that work.

[HighCommander4]

The .cppm files could be added using AdditionalFiles like this. But I'm not sure about running the intermediate commands.

Can clang be made to build main.cpp in a single command, building the modules "on the fly"? If so, the combination of AdditionalFiles and ExtraArgs may be sufficient.

[ChuanqiXu9]

Can clang be made to build main.cpp in a single command, building the modules "on the fly"? If so, the combination of AdditionalFiles and ExtraArgs may be sufficient.

No, we can't do that. And we have consensus that the implicit build is bad so it won't come in the future too.

I haven't thought through how this might work with modules, especially prebuilt ones. Possibly the test infrastructure needs some changes to make that work.

Agreed. I'll try to look at it. And it would be sufficient to me if we can create temporary directories in unit tests. I'll try to look at more examples and docs. @dwblaikie , hi, do you know how to create temporary directories in unit tests. (I CC you since I feel like you are familiar with the unittests. Or do you know who is the right person to ask this question?)

[ChuanqiXu9]

Oh, I finally found the methods to add test in clangd in the traditional clang style. I sent https://reviews.llvm.org/D137693.

And while the method to create temporary directory is still very meaningful, I might not pursue it recently. I feel such tests may be sufficient.

And I want to close the issue after I sent a basic document in https://clang.llvm.org/docs/StandardCPlusPlusModules.html to tell people how to use clangd to do code completion. I know there are many bugs and problems still:

• How should the clangd interact with the build systems? Now, we require the user to build the modules ahead manually. It is not so friendly. And it would be significantly worse when the user edits any of the module units. Then the clangd would show many errors due to the clang found the source files and the module files is not consistency. And we need to the users to rebuild it again now. It is unfriendly too. So here comes questions: should the clangd tell the build system to rebuild it automatically? Should we ban the consistency check between source files and module files? All of the problems need time to design.
• There are existing problems. I found two immediately: [C++20] [Modules] [clangd] We can't find exported entities by export using style #58889 and [C++20] [Modules] [clangd] We can't find exported entities by export using style #58889. I believe there are more problems.

But after all, it would be good direction to me.

[ChuanqiXu9]

Closed for the sake of @sam-mccall mentioned in https://reviews.llvm.org/D137770. We do need better design for this. And let's track it by a new issue when we had a good plan.