[clangd] Drop path suffix mapping for std symbols

[kadircet]

Summary

As mentioned in D88144, this is only used as a fallback for symbols
missing in our stdsymbol mappings and preventing us from figuring out what's
broken.

After this patch we'll regress some cases, e.g. std::move, but those were
already broken for libc++ or C.

---

I am also happy with coming up with special casing sfor obvious regressions,
like std::move, before landing. The special casing I have in mind could either
be:

• having a more targeted version of the suffix mapping targeted at those symbols, so we can stop the bleeding as we figure out cases even if we can't invest in a more principled solution.
• just picking a single header in case of ambigious symbols, this might end up being annoying as clangd will insist on inserting the wrong header.
• disable include insertion completely for problematic symbols until we figure out a solution.

[sammccall]

Taking another look at the header list, there are a couple of classes of symbols beyond c/c++ standard library.

We don't have an alternative database for these. Maybe we should keep the mechanism but stop using it for the standard library? What do you think?

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

46–47

should we update these comments?

51

hmm, are we going to regress all of posix?

52

and builtins, these are actually owned by us so should be portable

[kadircet]

We don't have an alternative database for these. Maybe we should keep the mechanism but stop using it for the standard library? What do you think?

Yes i think you are right. Just dropping STL mappings from the list.

[kadircet]

• Only drop STL mapping instead of getting rid of suffix mapping completely.

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

public:
   /// Adds a string-to-string mapping from \p Path to \p CanonicalPath.
   void addMapping(llvm::StringRef Path, llvm::StringRef CanonicalPath);
 
   /// Returns the canonical include for symbol with \p QualifiedName.
   /// \p Header is the file the declaration was reachable from.
   /// Header itself will be returned if there is no relevant mapping.
   llvm::StringRef mapHeader(llvm::StringRef Header,
                             llvm::StringRef QualifiedName) const;
 
-  /// Adds mapping for system headers and some special symbols (e.g. STL symbols
-  /// in <iosfwd> need to be mapped individually). Approximately, the following
-  /// system headers are handled:
-  ///   - C++ standard library e.g. bits/basic_string.h$ -> <string>
+  /// Adds mapping for system headers and some special symbols. Uses a symbol
+  /// based mapping for STL symbols. Handles other libraries by matching on the
+  /// path. Approximately, the following system headers are handled:
+  ///   - C++ standard library e.g. std::string -> <string>
   ///   - Posix library e.g. bits/pthreadtypes.h$ -> <pthread.h>
   ///   - Compiler extensions, e.g. include/avx512bwintrin.h$ -> <immintrin.h>
   /// The mapping is hardcoded and hand-maintained, so it might not cover all
   /// headers.
   void addSystemHeadersMapping(const LangOptions &Language);
 
 private:
   /// A map from full include path to a canonical path.
   llvm::StringMap<std::string> FullPathMapping;
   /// A map from a suffix (one or components of a path) to a canonical path.

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

#undef SYMBOL
     });
     StdSymbolMapping = CSymbols;
   }
 
   // FIXME: remove the std header mapping once we support ambiguous symbols, now
   // it serves as a fallback to disambiguate:
   //   - symbols with multiple headers (e.g. std::move)
   static const auto *SystemHeaderMap = new llvm::StringMap<llvm::StringRef>({
+      // Resource lib
       {"include/__stddef_max_align_t.h", "<cstddef>"},
+      {"include/_G_config.h", "<cstdio>"},
+      {"include/assert.h", "<cassert>"},
+      // Compiler intrinsics
       {"include/__wmmintrin_aes.h", "<wmmintrin.h>"},
       {"include/__wmmintrin_pclmul.h", "<wmmintrin.h>"},
       {"include/adxintrin.h", "<immintrin.h>"},
       {"include/ammintrin.h", "<ammintrin.h>"},
       {"include/avx2intrin.h", "<immintrin.h>"},
       {"include/avx512bwintrin.h", "<immintrin.h>"},
       {"include/avx512cdintrin.h", "<immintrin.h>"},
       {"include/avx512dqintrin.h", "<immintrin.h>"},
static const auto *SystemHeaderMap = new llvm::StringMap<llvm::StringRef>({
       {"include/x86intrin.h", "<x86intrin.h>"},
       {"include/xmmintrin.h", "<xmmintrin.h>"},
       {"include/xopintrin.h", "<x86intrin.h>"},
       {"include/xsavecintrin.h", "<immintrin.h>"},
       {"include/xsaveintrin.h", "<immintrin.h>"},
       {"include/xsaveoptintrin.h", "<immintrin.h>"},
       {"include/xsavesintrin.h", "<immintrin.h>"},
       {"include/xtestintrin.h", "<immintrin.h>"},
-      {"include/_G_config.h", "<cstdio>"},
-      {"include/assert.h", "<cassert>"},
-      {"algorithm", "<algorithm>"},
-      {"valarray", "<valarray>"},
-      {"array", "<array>"},
-      {"atomic", "<atomic>"},
-      {"backward/auto_ptr.h", "<memory>"},
-      {"backward/binders.h", "<string>"},
-      {"bits/algorithmfwd.h", "<algorithm>"},
-      {"bits/alloc_traits.h", "<memory>"},
-      {"bits/allocated_ptr.h", "<memory>"},
-      {"bits/allocator.h", "<allocator>"},
-      {"bits/atomic_base.h", "<atomic>"},
-      {"bits/atomic_lockfree_defines.h", "<exception>"},
-      {"bits/atomic_futex.h", "<atomic>"},
-      {"bits/basic_ios.h", "<ios>"},
-      {"bits/basic_ios.tcc", "<ios>"},
-      {"bits/basic_string.h", "<string>"},
-      {"bits/basic_string.tcc", "<string>"},
-      {"bits/char_traits.h", "<string>"},
-      {"bits/codecvt.h", "<locale>"},
-      {"bits/concept_check.h", "<numeric>"},
-      {"bits/cpp_type_traits.h", "<cmath>"},
-      {"bits/cxxabi_forced.h", "<cxxabi.h>"},
-      {"bits/deque.tcc", "<deque>"},
-      {"bits/exception.h", "<exception>"},
-      {"bits/exception_defines.h", "<exception>"},
-      {"bits/exception_ptr.h", "<exception>"},
-      {"bits/forward_list.h", "<forward_list>"},
-      {"bits/forward_list.tcc", "<forward_list>"},
-      {"bits/fstream.tcc", "<fstream>"},
-      {"bits/functexcept.h", "<list>"},
-      {"bits/functional_hash.h", "<functional>"},
-      {"bits/gslice.h", "<valarray>"},
-      {"bits/gslice_array.h", "<valarray>"},
-      {"bits/hash_bytes.h", "<typeinfo>"},
-      {"bits/hashtable.h", "<unordered_set>"},
-      {"bits/hashtable_policy.h", "<unordered_set>"},
-      {"bits/indirect_array.h", "<valarray>"},
-      {"bits/invoke.h", "<functional>"},
-      {"bits/ios_base.h", "<ios>"},
-      {"bits/istream.tcc", "<istream>"},
-      {"bits/list.tcc", "<list>"},
-      {"bits/locale_classes.h", "<locale>"},
-      {"bits/locale_classes.tcc", "<locale>"},
-      {"bits/locale_conv.h", "<locale>"},
-      {"bits/locale_facets.h", "<locale>"},
-      {"bits/locale_facets.tcc", "<locale>"},
-      {"bits/locale_facets_nonio.h", "<locale>"},
-      {"bits/locale_facets_nonio.tcc", "<locale>"},
-      {"bits/localefwd.h", "<locale>"},
-      {"bits/mask_array.h", "<valarray>"},
-      {"bits/memoryfwd.h", "<memory>"},
-      {"bits/move.h", "<utility>"},
-      {"bits/nested_exception.h", "<exception>"},
-      {"bits/ostream.tcc", "<ostream>"},
-      {"bits/ostream_insert.h", "<ostream>"},
-      {"bits/parse_numbers.h", "<chrono>"},
-      {"bits/postypes.h", "<ios>"},
-      {"bits/predefined_ops.h", "<algorithm>"},
-      {"bits/ptr_traits.h", "<memory>"},
-      {"bits/quoted_string.h", "<iomanip>"},
-      {"bits/random.h", "<random>"},
-      {"bits/random.tcc", "<random>"},
-      {"bits/range_access.h", "<iterator>"},
-      {"bits/refwrap.h", "<functional>"},
-      {"bits/regex.h", "<regex>"},
-      {"bits/regex_automaton.h", "<regex>"},
-      {"bits/regex_compiler.h", "<regex>"},
-      {"bits/regex_constants.h", "<regex>"},
-      {"bits/regex_cursor.h", "<regex>"},
-      {"bits/regex_error.h", "<regex>"},
-      {"bits/regex_executor.h", "<regex>"},
-      {"bits/regex_grep_matcher.h", "<regex>"},
-      {"bits/regex_grep_matcher.tcc", "<regex>"},
-      {"bits/regex_nfa.h", "<regex>"},
-      {"bits/regex_scanner.h", "<regex>"},
-      {"bits/shared_ptr.h", "<memory>"},
-      {"bits/shared_ptr_base.h", "<memory>"},
-      {"bits/shared_ptr_atomic.h", "<atomic>"},
-      {"bits/slice_array.h", "<valarray>"},
-      {"bits/sstream.tcc", "<sstream>"},
-      {"bits/std_abs.h", "<cmath>"},
-      {"bits/std_function.h", "<functional>"},
-      {"bits/std_mutex.h", "<mutex>"},
-      {"bits/stl_algo.h", "<algorithm>"},
-      {"bits/stl_algobase.h", "<algorithm>"},
-      {"bits/stl_bvector.h", "<vector>"},
-      {"bits/stl_construct.h", "<deque>"},
-      {"bits/stl_deque.h", "<deque>"},
-      {"bits/stl_function.h", "<functional>"},
-      {"bits/stl_heap.h", "<heap>"},
-      {"bits/stl_iterator.h", "<iterator>"},
-      {"bits/stl_iterator_base_funcs.h", "<iterator>"},
-      {"bits/stl_iterator_base_types.h", "<iterator>"},
-      {"bits/stl_list.h", "<list>"},
-      {"bits/stl_map.h", "<map>"},
-      {"bits/stl_multimap.h", "<map>"},
-      {"bits/stl_multiset.h", "<set>"},
-      {"bits/stl_numeric.h", "<numeric>"},
-      {"bits/stl_pair.h", "<utility>"},
-      {"bits/stl_queue.h", "<queue>"},
-      {"bits/stl_raw_storage_iter.h", "<memory>"},
-      {"bits/stl_relops.h", "<utility>"},
-      {"bits/stl_set.h", "<set>"},
-      {"bits/stl_stack.h", "<stack>"},
-      {"bits/stl_tempbuf.h", "<memory>"},
-      {"bits/stl_tree.h", "<map>"},
-      {"bits/stl_uninitialized.h", "<memory>"},
-      {"bits/stl_vector.h", "<vector>"},
-      {"bits/stream_iterator.h", "<iterator>"},
-      {"bits/streambuf.tcc", "<streambuf>"},
-      {"bits/streambuf_iterator.h", "<iterator>"},
-      {"bits/stringfwd.h", "<string>"},
-      {"bits/uniform_int_dist.h", "<random>"},
-      {"bits/unique_ptr.h", "<memory>"},
-      {"bits/unordered_map.h", "<unordered_map>"},
-      {"bits/unordered_set.h", "<unordered_set>"},
-      {"bits/uses_allocator.h", "<memory>"},
-      {"bits/valarray_after.h", "<valarray>"},
-      {"bits/valarray_array.h", "<valarray>"},
-      {"bits/valarray_array.tcc", "<valarray>"},
-      {"bits/valarray_before.h", "<valarray>"},
-      {"bits/vector.tcc", "<vector>"},
-      {"bitset", "<bitset>"},
-      {"ccomplex", "<ccomplex>"},
-      {"cctype", "<cctype>"},
-      {"cerrno", "<cerrno>"},
-      {"cfenv", "<cfenv>"},
-      {"cfloat", "<cfloat>"},
-      {"chrono", "<chrono>"},
-      {"cinttypes", "<cinttypes>"},
-      {"climits", "<climits>"},
-      {"clocale", "<clocale>"},
-      {"cmath", "<cmath>"},
-      {"complex", "<complex>"},
-      {"complex.h", "<complex.h>"},
-      {"condition_variable", "<condition_variable>"},
-      {"csetjmp", "<csetjmp>"},
-      {"csignal", "<csignal>"},
-      {"cstdalign", "<cstdalign>"},
-      {"cstdarg", "<cstdarg>"},
-      {"cstdbool", "<cstdbool>"},
-      {"cstdint", "<cstdint>"},
-      {"cstdio", "<cstdio>"},
-      {"cstdlib", "<cstdlib>"},
-      {"cstring", "<cstring>"},
-      {"ctgmath", "<ctgmath>"},
-      {"ctime", "<ctime>"},
-      {"cwchar", "<cwchar>"},
-      {"cwctype", "<cwctype>"},
-      {"cxxabi.h", "<cxxabi.h>"},
-      {"debug/debug.h", "<numeric>"},
-      {"debug/map.h", "<map>"},
-      {"debug/multimap.h", "<multimap>"},
-      {"debug/multiset.h", "<multiset>"},
-      {"debug/set.h", "<set>"},
-      {"deque", "<deque>"},
-      {"exception", "<exception>"},
-      {"ext/alloc_traits.h", "<deque>"},
-      {"ext/atomicity.h", "<memory>"},
-      {"ext/concurrence.h", "<memory>"},
-      {"ext/new_allocator.h", "<string>"},
-      {"ext/numeric_traits.h", "<list>"},
-      {"ext/string_conversions.h", "<string>"},
-      {"ext/type_traits.h", "<cmath>"},
-      {"fenv.h", "<fenv.h>"},
-      {"forward_list", "<forward_list>"},
-      {"fstream", "<fstream>"},
-      {"functional", "<functional>"},
-      {"future", "<future>"},
-      {"initializer_list", "<initializer_list>"},
-      {"iomanip", "<iomanip>"},
-      {"ios", "<ios>"},
-      {"iosfwd", "<iosfwd>"},
-      {"iostream", "<iostream>"},
-      {"istream", "<istream>"},
-      {"iterator", "<iterator>"},
-      {"limits", "<limits>"},
-      {"list", "<list>"},
-      {"locale", "<locale>"},
-      {"map", "<map>"},
-      {"memory", "<memory>"},
-      {"shared_mutex", "<shared_mutex>"},
-      {"mutex", "<mutex>"},
-      {"new", "<new>"},
-      {"numeric", "<numeric>"},
-      {"ostream", "<ostream>"},
-      {"queue", "<queue>"},
-      {"random", "<random>"},
-      {"ratio", "<ratio>"},
-      {"regex", "<regex>"},
-      {"scoped_allocator", "<scoped_allocator>"},
-      {"set", "<set>"},
-      {"sstream", "<sstream>"},
-      {"stack", "<stack>"},
-      {"stdexcept", "<stdexcept>"},
-      {"streambuf", "<streambuf>"},
-      {"string", "<string>"},
-      {"system_error", "<system_error>"},
-      {"tgmath.h", "<tgmath.h>"},
-      {"thread", "<thread>"},
-      {"tuple", "<tuple>"},
-      {"type_traits", "<type_traits>"},
-      {"typeindex", "<typeindex>"},
-      {"typeinfo", "<typeinfo>"},
-      {"unordered_map", "<unordered_map>"},
-      {"unordered_set", "<unordered_set>"},
-      {"utility", "<utility>"},
-      {"valarray", "<valarray>"},
-      {"vector", "<vector>"},
-      {"include/complex.h", "<complex.h>"},
-      {"include/ctype.h", "<cctype>"},
-      {"include/errno.h", "<cerrno>"},
-      {"include/fenv.h", "<fenv.h>"},
-      {"include/inttypes.h", "<cinttypes>"},
-      {"include/libio.h", "<cstdio>"},
-      {"include/limits.h", "<climits>"},
-      {"include/locale.h", "<clocale>"},
-      {"include/math.h", "<cmath>"},
-      {"include/setjmp.h", "<csetjmp>"},
-      {"include/signal.h", "<csignal>"},
-      {"include/stdint.h", "<cstdint>"},
-      {"include/stdio.h", "<cstdio>"},
-      {"include/stdlib.h", "<cstdlib>"},
-      {"include/string.h", "<cstring>"},
-      {"include/time.h", "<ctime>"},
-      {"include/wchar.h", "<cwchar>"},
-      {"include/wctype.h", "<cwctype>"},
-      {"bits/cmathcalls.h", "<complex.h>"},
-      {"bits/errno.h", "<cerrno>"},
-      {"bits/fenv.h", "<fenv.h>"},
-      {"bits/huge_val.h", "<cmath>"},
-      {"bits/huge_valf.h", "<cmath>"},
-      {"bits/huge_vall.h", "<cmath>"},
-      {"bits/inf.h", "<cmath>"},
-      {"bits/local_lim.h", "<climits>"},
-      {"bits/locale.h", "<clocale>"},
-      {"bits/mathcalls.h", "<math.h>"},
-      {"bits/mathdef.h", "<cmath>"},
-      {"bits/nan.h", "<cmath>"},
-      {"bits/posix1_lim.h", "<climits>"},
-      {"bits/posix2_lim.h", "<climits>"},
-      {"bits/setjmp.h", "<csetjmp>"},
-      {"bits/sigaction.h", "<csignal>"},
-      {"bits/sigcontext.h", "<csignal>"},
-      {"bits/siginfo.h", "<csignal>"},
-      {"bits/signum.h", "<csignal>"},
-      {"bits/sigset.h", "<csignal>"},
-      {"bits/sigstack.h", "<csignal>"},
-      {"bits/stdint-intn.h", "<cstdint>"},
-      {"bits/stdint-uintn.h", "<cstdint>"},
-      {"bits/stdio_lim.h", "<cstdio>"},
-      {"bits/sys_errlist.h", "<cstdio>"},
-      {"bits/time.h", "<ctime>"},
-      {"bits/timex.h", "<ctime>"},
-      {"bits/typesizes.h", "<cstdio>"},
-      {"bits/wchar.h", "<cwchar>"},
-      {"bits/wordsize.h", "<csetjmp>"},
-      {"bits/xopen_lim.h", "<climits>"},
-      {"include/xlocale.h", "<cstring>"},
-      {"bits/atomic_word.h", "<memory>"},
-      {"bits/basic_file.h", "<fstream>"},
-      {"bits/c\\+\\+allocator.h", "<string>"},
-      {"bits/c\\+\\+config.h", "<cstddef>"},
-      {"bits/c\\+\\+io.h", "<ios>"},
-      {"bits/c\\+\\+locale.h", "<locale>"},
-      {"bits/cpu_defines.h", "<iosfwd>"},
-      {"bits/ctype_base.h", "<locale>"},
-      {"bits/cxxabi_tweaks.h", "<cxxabi.h>"},
-      {"bits/error_constants.h", "<system_error>"},
-      {"bits/gthr-default.h", "<memory>"},
-      {"bits/gthr.h", "<memory>"},
-      {"bits/opt_random.h", "<random>"},
-      {"bits/os_defines.h", "<iosfwd>"},
       // GNU C headers
       {"include/aio.h", "<aio.h>"},
       {"include/aliases.h", "<aliases.h>"},
       {"include/alloca.h", "<alloca.h>"},
       {"include/ar.h", "<ar.h>"},
       {"include/argp.h", "<argp.h>"},
       {"include/argz.h", "<argz.h>"},
       {"include/arpa/nameser.h", "<resolv.h>"},

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

 
 TEST(CanonicalIncludesTest, CStandardLibrary) {
   CanonicalIncludes CI;
   auto Language = LangOptions();
   Language.C11 = true;
   CI.addSystemHeadersMapping(Language);
   // Usual standard library symbols are mapped correctly.
   EXPECT_EQ("<stdio.h>", CI.mapHeader("path/stdio.h", "printf"));
-  // Suffix mapping isn't available for C, instead of mapping to `<cstdio> we
-  // just leave the header as-is.
-  EXPECT_EQ("include/stdio.h",
-            CI.mapHeader("include/stdio.h", "unknown_symbol"));
 }
 
 TEST(CanonicalIncludesTest, CXXStandardLibrary) {
   CanonicalIncludes CI;
   auto Language = LangOptions();
   Language.CPlusPlus = true;
   CI.addSystemHeadersMapping(Language);
 
   // Usual standard library symbols are mapped correctly.
-  EXPECT_EQ("<vector>", CI.mapHeader("path/vector.h", "std::vector"));
-  EXPECT_EQ("<cstdio>", CI.mapHeader("path/stdio.h", "std::printf"));
-  // std::move is ambiguous, currently always mapped to <utility>
-  EXPECT_EQ("<utility>",
-            CI.mapHeader("libstdc++/bits/stl_algo.h", "std::move"));
+  EXPECT_EQ("<vector>", CI.mapHeader("some_lib.h", "std::vector"));
+  EXPECT_EQ("<cstdio>", CI.mapHeader("some_lib.h", "std::printf"));
+  // std::move is ambiguous, always map to <utility>.
+  EXPECT_EQ("<utility>", CI.mapHeader("some_lib.h", "std::move"));
   // Unknown std symbols aren't mapped.
   EXPECT_EQ("foo/bar.h", CI.mapHeader("foo/bar.h", "std::notathing"));
-  // iosfwd declares some symbols it doesn't own.
-  EXPECT_EQ("<ostream>", CI.mapHeader("iosfwd", "std::ostream"));
-  // And (for now) we assume it owns the others.
-  EXPECT_EQ("<iosfwd>", CI.mapHeader("iosfwd", "std::notwathing"));
 }
 
 TEST(CanonicalIncludesTest, PathMapping) {
   // As used for IWYU pragmas.
   CanonicalIncludes CI;
   CI.addMapping("foo/bar", "<baz>");
 
   EXPECT_EQ("<baz>", CI.mapHeader("foo/bar", "some::symbol"));
TEST(CanonicalIncludesTest, Precedence) {
   CanonicalIncludes CI;
   CI.addMapping("some/path", "<path>");
   LangOptions Language;
   Language.CPlusPlus = true;
   CI.addSystemHeadersMapping(Language);
 
   // We added a mapping from some/path to <path>.
   ASSERT_EQ("<path>", CI.mapHeader("some/path", ""));
-  // We should have a path from 'bits/stl_vector.h' to '<vector>'.
-  ASSERT_EQ("<vector>", CI.mapHeader("bits/stl_vector.h", ""));
   // We should also have a symbol mapping from 'std::map' to '<map>'.
   ASSERT_EQ("<map>", CI.mapHeader("some/header.h", "std::map"));
 
   // And the symbol mapping should take precedence over paths mapping.
   EXPECT_EQ("<map>", CI.mapHeader("bits/stl_vector.h", "std::map"));
   EXPECT_EQ("<map>", CI.mapHeader("some/path", "std::map"));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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

TEST(FileIndexTest, IncludeCollected) {
   auto Symbols = runFuzzyFind(M, "");
   EXPECT_THAT(Symbols, ElementsAre(_));
   EXPECT_THAT(Symbols.begin()->IncludeHeaders.front().IncludeHeader,
               "<the/good/header.h>");
 }
 
 TEST(FileIndexTest, HasSystemHeaderMappingsInPreamble) {
   TestTU TU;
-  TU.HeaderCode = "class Foo{};";
-  TU.HeaderFilename = "algorithm";
+  // std::max is mapped back to <algorithm>
+  TU.HeaderCode = "namespace std { void max(); }";
+  TU.HeaderFilename = "ignored_header_name.h";
 
-  auto Symbols = runFuzzyFind(*TU.index(), "");
+  auto Symbols = runFuzzyFind(*TU.index(), "max");
   EXPECT_THAT(Symbols, ElementsAre(_));
   EXPECT_THAT(Symbols.begin()->IncludeHeaders.front().IncludeHeader,
               "<algorithm>");
 }
 
 TEST(FileIndexTest, TemplateParamsInLabel) {
   auto Source = R"cpp(
 template <class Ty>