[Tooling/Inclusion] Add symbol mappings for `std::experimental::filesystem`

[zyounan]

Summary

Clangd maintains a symbol map from standard library, in order to prevent
unexpected header/symbol leaks from internal files. (e.g. files under
bits/ for libstdc++) This symbol map was generated by a python script
that parses pages of offline cppreference archive. The script didn't
handle the case for std::experimental::, where most symbols are from
TS. It works well as symbols are directly laid out in the corresponding
header under experimental directory for most of time.

However, libstdc++'s implementation split symbols of TS FS into a few
header files located in experimental/bits. This would make the code
completion provide internal headers when we simply select the symbols.

There are slightly differences between TS FS and C++17 FS. Some
functions like system_complete was replaced by absolute and
relative-related operations were introduced later by another proposal.
Even so, all mainstream implementation are based on N4100, the final
filesystem TS draft that was published in 2014 and from which symbols
we've added are exported.

This fixes https://github.com/clangd/clangd/issues/1481

[zyounan]

Do not format inc file

[zyounan]

Format

[kadircet]

thanks a lot for the patch, we're already having some efforts right now to improve our cppreference parsing and get to a more complete set of symbols (@VitaNuo for visibility, who drives these efforts).

Regarding the experimental symbols, first of all I am not sure if it's a good idea to include them in clangd (or other clang-tools) at all. As a clang-tool binary can stay around for years (e.g. people can use 3 years old clangd) and symbols under std::experimental are subject to change throughout that period, hence the information provided is going to be vastly different than what the truth is in that time window (also i don't know if they'll be "same" across libc++ vs libstdc++ implementations).
In addition to that, AFAICT there's no great index page to parse for technical-specs hence we might indeed be forced to manually curate these symbols and that's something we've been trying to avoid due to maintenance costs. In the case of technical-specs this cost will increase, because they're subject to change a lot more frequent than regular std symbols and value is even less as these are not used by majority of the developers.

So I am afraid this is not some extra complexity we can take in.

[kadircet]

And to follow up, I definitely see the problem you're facing and it's something we'd like to address as well, just not in the way you proposed.

This falls under the bucket of "we might have symbols missing from our stdlib mappings and should try to compensate for that", e.g. we should probably try disabling include-insertion for such cases where we have a high confidence that the symbol is part of stdlib but we don't have an exact mapping in our symbol database to remedy the issue around inserting "wrong" includes.

[zyounan]

This comment was removed by zyounan.

[zyounan]

we're already having some efforts right now to improve our cppreference parsing and get to a more complete set of symbols (@VitaNuo for visibility, who drives these efforts)..

Yes, this is another issue I want to address, but you've already pointed out that the parser is improving.

The cppref archive page is not always up to date. As of now, the official version is still dumped in 2019, that means we're likely missing (partial) symbols from C++20/23, e.g. std::expected.

[zyounan]

Revise symbols from N4100

[kadircet]

Thanks for updating this, as I mentioned in https://reviews.llvm.org/D143319#4115186, we should put these symbols into their own symbol map. ATM StdSymbolMap.inc is still generated by an automated tool and shouldn't be modified by hand.
So can you rather put these symbols into llvm/llvm-project/clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc ? you also need to rebase your branch, we've moved these mappings from include directory to lib. they're implementation details now, and not public interfaces.
After putting it into a new file, you'll also need to include it in https://github.com/llvm/llvm-project/blob/main/clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp#L65.

As for tests, rather than using clangd, can you introduce tests into StandardLibrary instead in https://github.com/llvm/llvm-project/blob/main/clang/unittests/Tooling/StandardLibraryTest.cpp ?

[zyounan]

Sorry for the late update, I'm rebasing my branch now.

[zyounan]

Rebase to main && Move tests to clang/Tooling

[kadircet]

thanks a lot! mostly LG couple more nits

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

1 ↗

(On Diff #496934)

i think comment is a little too verbose. can you just say:

These are derived from N4100[fs.filesystem.synopsis], final draft for experimental filesystem.

There's no need for mentioning that these became the standard in C++17 or being a cornerstone for stdlib implementations. As they won't necessarily be true for other technical specs nor if we were adding this pre-c++17 standardisation. But we'd still like to have these widely adapted symbols included in the mapping to make sure we're not generating false negatives.

6 ↗

(On Diff #496934)

can you strip clang-format pragmas to be similar to other mapping files.

clang/unittests/Tooling/StandardLibraryTest.cpp

83 ↗

(On Diff #496934)

another EXPECT_FALSE with Lang::C would also be important to make sure we're not adding these for C mappings by mistake.

91 ↗

(On Diff #496934)

can you also check for Symbol->headers()

93 ↗

(On Diff #496934)

i don't think there's much point in asserting these "meta" details about the mapping (same for the test below).

[zyounan]

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

6 ↗

(On Diff #496934)

I'd love to if possible, but for some reason clang-format would add extra spaces before and after the slash, <experimental / filesystem>, which looks ugly and I don't expect it right.

[kadircet]

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

6 ↗

(On Diff #496934)

i don't follow, why do you need to run clang-format on this file? symbols are already ordered

[zyounan]

Revise

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

1 ↗

(On Diff #496934)

Well, I'm not quite sure if I should address these small differences between TS and C++17. But FWIW, the verbosity should go away.

clang/unittests/Tooling/StandardLibraryTest.cpp

93 ↗

(On Diff #496934)

They're used to emphasize the slight difference. They can be removed if you don't think it necessary :)

[zyounan]

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

6 ↗

(On Diff #496934)

Specifically, it is git clang-format. The pipeline would fail if this command does modify some files IIRC.

[kadircet]

thanks!

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

6 ↗

(On Diff #496934)

not sure what workflow you're using but clang-format is not a mandatory part of the workflow.
recommended way is updating/uploading patches using arcanist, whose config only contains files with one of the cc|h|cpp extensions, see https://github.com/llvm/llvm-project/blob/main/.arclint.

anyways, happy to land this for you if it's not easy to disable clang-format linting on your workflow for whatever reason.

[zyounan]

Thank you for your detailed explanation and sorry again for my dumb mistake before. :)

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

6 ↗

(On Diff #496934)

I've looked into the script of the running pipeline, it shows that it is running ${SRC}/scripts/premerge_checks.py --check-clang-format, which would [run git clang-format](https://github.com/google/llvm-premerge-checks/blob/abe5c8991b5e81f0182528ff8ce515ba89a66c0a/scripts/premerge_checks.py#L163). I'm using arcanist with default config, and I don't know if I'm missing something to skip this. (Perhaps it is because the rule you mentioned doesn't exclude .inc files, and we didn't write any slashes in such files either.)

[zyounan]

clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc

6 ↗

(On Diff #496934)

...Or it is the rule defined in pipeline doesn't exclude them... https://github.com/google/llvm-premerge-checks/blob/abe5c8991b5e81f0182528ff8ce515ba89a66c0a/scripts/clang-format.ignore

[zyounan]

Apply clang-format to StandardLibrary.cpp to fix pipeline failure

clang-tools-extra/clangd/index/CanonicalIncludes.cpp

void CanonicalIncludes::addSystemHeadersMapping(const LangOptions &Language) {
   if (Language.CPlusPlus) {
     static const auto *Symbols = new llvm::StringMap<llvm::StringRef>({
 #define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name, #Header},
 #include "clang/Tooling/Inclusions/StdSymbolMap.inc"
         // There are two std::move()s, this is by far the most common.
         SYMBOL(move, std::, <utility>)
         // There are multiple headers for size_t, pick one.
         SYMBOL(size_t, std::, <cstddef>)
+    // These symbols are located in `std::experimental`.
+    // For libstdc++, This mapping mainly fixes issue that some internal headers
+    // would be inserted for code completion as not all symbols from
+    // `std::experimental::filesystem` are in the main header, i.e.,
+    // <experimental/filesystem>
+    // libc++ had already dropped support for experimental FS as of now.
+    // MSVC STL includes all experimental FS symbols in one single file.
+    // TODO: More TS symbols? It appears to me that only FS was not implemented
+    // in one single file in libstdc++. What about libc++ or MSVC?
+#include "clang/Tooling/Inclusions/TsStdSymbolMap.inc"
 #undef SYMBOL
     });
     StdSymbolMapping = Symbols;
   } else if (Language.C11) {
     static const auto *CSymbols = new llvm::StringMap<llvm::StringRef>({
 #define SYMBOL(Name, NameSpace, Header) {#Name, #Header},
 #include "clang/Tooling/Inclusions/CSymbolMap.inc"
         // There are multiple headers for size_t, pick one.

clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp

TEST(CanonicalIncludesTest, CXXStandardLibrary) {
   EXPECT_EQ("<ostream>", CI.mapSymbol("std::ostream"));
   // And (for now) we assume it owns the others.
   auto InMemFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
   FileManager Files(FileSystemOptions(), InMemFS);
   auto File = addFile(*InMemFS, Files, testPath("iosfwd"));
   EXPECT_EQ("<iosfwd>", CI.mapHeader(File));
 }
 
+TEST(CanonicalIncludesTest, CXXSymbolsFromTS) {
+  CanonicalIncludes CI;
+  auto Language = LangOptions();
+  Language.CPlusPlus = true;
+  CI.addSystemHeadersMapping(Language);
+
+  EXPECT_EQ("<experimental/filesystem>",
+            CI.mapSymbol("std::experimental::filesystem::path"));
+  EXPECT_EQ("<experimental/filesystem>",
+            CI.mapSymbol("std::experimental::filesystem::file_type"));
+  EXPECT_EQ("<experimental/filesystem>",
+            CI.mapSymbol("std::experimental::filesystem::file_status"));
+  EXPECT_EQ("<experimental/filesystem>",
+            CI.mapSymbol(
+                "std::experimental::filesystem::recursive_directory_iterator"));
+}
+
 TEST(CanonicalIncludesTest, PathMapping) {
   auto InMemFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
   FileManager Files(FileSystemOptions(), InMemFS);
   std::string BarPath = testPath("foo/bar");
   auto Bar = addFile(*InMemFS, Files, BarPath);
   auto Other = addFile(*InMemFS, Files, testPath("foo/baz"));
   // As used for IWYU pragmas.
   CanonicalIncludes CI;

clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc

+// FIXME: Add a parser for experimental pages to `clang/tools/include-mapping/cppreference_parser.py`
+
+// clang-format off
+SYMBOL(absolute, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(canonical, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(copy, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(copy_file, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(copy_options, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(copy_symlink, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(create_directories, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(create_directory, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(create_directory_symlink, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(create_hard_link, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(create_symlink, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(current_path, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(directory_entry, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(directory_iterator, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(directory_options, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(equivalent, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(exists, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(file_size, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(file_status, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(file_time_type, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(file_type, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(filesystem_error, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(hard_link_count, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_block_file, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_character_file, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_directory, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_empty, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_fifo, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_other, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_regular_file, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_socket, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(is_symlink, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(last_write_time, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(path, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(perm_options, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(permissions, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(perms, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(proximate, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(read_symlink, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(recursive_directory_iterator, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(relative, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(remove, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(remove_all, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(rename, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(resize_file, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(space, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(space_info, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(status, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(status_known, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(symlink_status, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(temp_directory_path, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(u8path, std::experimental::filesystem::, <experimental/filesystem>)
+SYMBOL(weakly_canonical, std::experimental::filesystem::, <experimental/filesystem>)
+// clang-format on