[clangd] Allow Unix config paths on Darwin

[keith]

Summary

It's common for CLIs on macOS to read configuration files from more
unix-standard directories with either dotfiles directly in ~ or files in
~/.config. Previously macOS clangd configuration only read from
~/Library/Preferences on macOS, now it respects the other Unix
directories with XDG_CONFIG_HOME or just ~/.config as a fallback. This
is implemented by changing user_config_directory to
user_config_directories which sets a vector of paths to search,
maintaining the previous order of operations.

[kadircet]

I got a couple of concerns about the general ideas in the change. Even though the convenience of using ~/.config/clangd/config.yaml seems nice, I think it'll just end up creating confusion in the long run.

According to various macOS developer documentation & QAs, canonical user config directory for applications are ~/Library/Preferences (see https://developer.apple.com/library/archive/qa/qa1170/_index.html#//apple_ref/doc/uid/DTS10001702-CH1-SECTION3 and various others).
Regarding XDG_CONFIG_HOME, AFAICT macOS doesn't really implement XDG Base Directory Specification, so querying its existence in that platform seems controversial.

Another concern is around multiple user config files. Clangd's config mechanism actually implements overriding of previous config options, and user config file is the most authoritative one and in the case of multiple such config files it's unclear which one should have precedence. So i don't think we should ever use "multiple" user config files.

cc @sammccall as he was the one that introduced this.

[keith]

In D113645#3126652, @kadircet wrote:

I got a couple of concerns about the general ideas in the change. Even though the convenience of using ~/.config/clangd/config.yaml seems nice, I think it'll just end up creating confusion in the long run.

According to various macOS developer documentation & QAs, canonical user config directory for applications are ~/Library/Preferences (see https://developer.apple.com/library/archive/qa/qa1170/_index.html#//apple_ref/doc/uid/DTS10001702-CH1-SECTION3 and various others).

While this makes sense, I think the practical realities today are that ~/.config is always preferred by unix style tools, even when on macOS.

Regarding XDG_CONFIG_HOME, AFAICT macOS doesn't really implement XDG Base Directory Specification, so querying its existence in that platform seems controversial.

Yea I considered still omitting that one, but figured it was fair to simplify the compiler conditionals and assume that if someone had that variable set on macOS it was intentional. But I think it would be fine to exclude.

Another concern is around multiple user config files. Clangd's config mechanism actually implements overriding of previous config options, and user config file is the most authoritative one and in the case of multiple such config files it's unclear which one should have precedence. So i don't think we should ever use "multiple" user config files.

Yea this makes sense. Theoretically we could check these "search paths" to determine if the configs actually existed to make that more clear, but with --log=verbose the order of operations is clear where clangd is searching for these so I think that shouldn't be a common issue.

clang-tools-extra/clangd/tool/ClangdMain.cpp

#endif
   Opts.CodeComplete.RankingModel = RankingModel;
 
   RealThreadsafeFS TFS;
   std::vector<std::unique_ptr<config::Provider>> ProviderStack;
   std::unique_ptr<config::Provider> Config;
   if (EnableConfig) {
     ProviderStack.push_back(
         config::Provider::fromAncestorRelativeYAMLFiles(".clangd", TFS));
-    llvm::SmallString<256> UserConfig;
-    if (llvm::sys::path::user_config_directory(UserConfig)) {
+    std::vector<std::string> UserConfigs;
+    if (llvm::sys::path::user_config_directories(UserConfigs)) {
+      for (auto ConfigRoot : UserConfigs) {
+        llvm::SmallString<128> UserConfig;
+        UserConfig.assign(ConfigRoot);
         llvm::sys::path::append(UserConfig, "clangd", "config.yaml");
         vlog("User config file is {0}", UserConfig);
         ProviderStack.push_back(config::Provider::fromYAMLFile(
             UserConfig, /*Directory=*/"", TFS, /*Trusted=*/true));
+      }
     } else {
       elog("Couldn't determine user config file, not loading");
     }
   }
   ProviderStack.push_back(std::make_unique<FlagsConfigProvider>());
   std::vector<const config::Provider *> ProviderPointers;
   for (const auto &P : ProviderStack)
     ProviderPointers.push_back(P.get());

llvm/include/llvm/Support/Path.h

 void system_temp_directory(bool erasedOnReboot, SmallVectorImpl<char> &result);
 
 /// Get the user's home directory.
 ///
 /// @param result Holds the resulting path name.
 /// @result True if a home directory is set, false otherwise.
 bool home_directory(SmallVectorImpl<char> &result);
 
-/// Get the directory where packages should read user-specific configurations.
-/// e.g. $XDG_CONFIG_HOME.
+/// Get the directories where packages should read user-specific
+/// configurations. e.g. $XDG_CONFIG_HOME.
 ///
-/// @param result Holds the resulting path name.
+/// @param result Holds the resulting paths.
 /// @result True if the appropriate path was determined, it need not exist.
-bool user_config_directory(SmallVectorImpl<char> &result);
+bool user_config_directories(std::vector<std::string> &results);
 
 /// Get the directory where installed packages should put their
 /// machine-local cache, e.g. $XDG_CACHE_HOME.
 ///
 /// @param result Holds the resulting path name.
 /// @result True if the appropriate path was determined, it need not exist.
 bool cache_directory(SmallVectorImpl<char> &result);
 

llvm/lib/Support/Unix/Path.inc

if (ConfLen > 0) {
     }
 
     Result.clear();
   }
   #endif
   return false;
 }
 
-bool user_config_directory(SmallVectorImpl<char> &result) {
+bool user_config_directories(std::vector<std::string> &results) {
+  bool found = false;
+  SmallString<128> result;
 #ifdef __APPLE__
   // Mac: ~/Library/Preferences/
   if (home_directory(result)) {
     append(result, "Library", "Preferences");
-    return true;
+    results.push_back(result.c_str());
+    found = true;
   }
-#else
+#endif
+
   // XDG_CONFIG_HOME as defined in the XDG Base Directory Specification:
   // http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
   if (const char *RequestedDir = getenv("XDG_CONFIG_HOME")) {
     result.clear();
     result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
-    return true;
+    results.push_back(result.c_str());
+    found = true;
   }
-#endif
+
   // Fallback: ~/.config
+  result.clear();
   if (!home_directory(result)) {
-    return false;
+    return found;
   }
   append(result, ".config");
+  results.push_back(result.c_str());
   return true;
 }
 
 bool cache_directory(SmallVectorImpl<char> &result) {
 #ifdef __APPLE__
   if (getDarwinConfDir(false/*tempDir*/, result)) {
     return true;
   }

llvm/lib/Support/Windows/Path.inc

if (ok)
     llvm::sys::path::make_preferred(result);
   return ok;
 }
 
 bool home_directory(SmallVectorImpl<char> &result) {
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
-bool user_config_directory(SmallVectorImpl<char> &result) {
+bool user_config_directories(std::vector<std::string> &results) {
   // Either local or roaming appdata may be suitable in some cases, depending
   // on the data. Local is more conservative, Roaming may not always be correct.
-  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+  SmallString<MAX_PATH> result;
+  bool found = getKnownFolderPath(FOLDERID_LocalAppData, &result);
+  results.push_back(result.c_str());
+  return found;
 }
 
 bool cache_directory(SmallVectorImpl<char> &result) {
   return getKnownFolderPath(FOLDERID_LocalAppData, result);
 }
 
 static bool getTempDirEnvVar(const wchar_t *Var, SmallVectorImpl<char> &Res) {
   SmallVector<wchar_t, 1024> Buf;

llvm/unittests/Support/Path.cpp

TEST(Support, HomeDirectoryWithNoEnv) {
   SmallString<128> HomeDir;
   EXPECT_TRUE(path::home_directory(HomeDir));
   EXPECT_EQ(PwDir, HomeDir);
 }
 
 TEST(Support, ConfigDirectoryWithEnv) {
   WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
 
-  SmallString<128> ConfigDir;
-  EXPECT_TRUE(path::user_config_directory(ConfigDir));
-  EXPECT_EQ("/xdg/config", ConfigDir);
+  std::vector<std::string> ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector<std::string> Expected = {"/xdg/config"};
+  EXPECT_EQ(Expected, ConfigDirs);
 }
 
 TEST(Support, ConfigDirectoryNoEnv) {
   WithEnv Env("XDG_CONFIG_HOME", nullptr);
 
   SmallString<128> Fallback;
   ASSERT_TRUE(path::home_directory(Fallback));
   path::append(Fallback, ".config");
 
-  SmallString<128> CacheDir;
-  EXPECT_TRUE(path::user_config_directory(CacheDir));
-  EXPECT_EQ(Fallback, CacheDir);
+  std::vector<std::string> ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector<std::string> Expected = {std::string(Fallback)};
+  EXPECT_EQ(Expected, ConfigDirs);
 }
 
 TEST(Support, CacheDirectoryWithEnv) {
   WithEnv Env("XDG_CACHE_HOME", "/xdg/cache");
 
   SmallString<128> CacheDir;
   EXPECT_TRUE(path::cache_directory(CacheDir));
   EXPECT_EQ("/xdg/cache", CacheDir);
TEST(Support, CacheDirectoryNoEnv) {
   SmallString<128> CacheDir;
   EXPECT_TRUE(path::cache_directory(CacheDir));
   EXPECT_EQ(Fallback, CacheDir);
 }
 #endif
 
 #ifdef __APPLE__
 TEST(Support, ConfigDirectory) {
-  SmallString<128> Fallback;
-  ASSERT_TRUE(path::home_directory(Fallback));
-  path::append(Fallback, "Library/Preferences");
+  WithEnv Env("XDG_CONFIG_HOME", nullptr);
+
+  SmallString<128> HomerDir;
+  ASSERT_TRUE(path::home_directory(HomerDir));
+
+  SmallString<128> Preferences = HomerDir;
+  path::append(Preferences, "Library/Preferences");
+
+  SmallString<128> ConfigDir = HomerDir;
+  path::append(ConfigDir, ".config");
+
+  std::vector<std::string> ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector<std::string> Expected = {std::string(Preferences),
+                                       std::string(ConfigDir)};
+  EXPECT_TRUE(Expected == ConfigDirs);
+}
+
+TEST(Support, XDGConfigDirectory) {
+  WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
+
+  SmallString<128> HomerDir;
+  ASSERT_TRUE(path::home_directory(HomerDir));
+
+  SmallString<128> Preferences = HomerDir;
+  path::append(Preferences, "Library/Preferences");
+
+  SmallString<128> ConfigDir = HomerDir;
+  path::append(ConfigDir, ".config");
 
-  SmallString<128> ConfigDir;
-  EXPECT_TRUE(path::user_config_directory(ConfigDir));
-  EXPECT_EQ(Fallback, ConfigDir);
+  std::vector<std::string> ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector<std::string> Expected = {std::string(Preferences), "/xdg/config",
+                                       std::string(ConfigDir)};
+  EXPECT_TRUE(Expected == ConfigDirs);
 }
 #endif
 
 #ifdef _WIN32
 TEST(Support, ConfigDirectory) {
   std::string Expected = getEnvWin(L"LOCALAPPDATA");
   // Do not try to test it if we don't know what to expect.
   if (!Expected.empty()) {