Merge pull request #11093 from NixOS/fix-concatStringsSep

Fix C++ `concatStringsSep`

src/build-remote/build-remote.cc

@@ -16,6 +16,7 @@
 #include "serialise.hh"
 #include "build-result.hh"
 #include "store-api.hh"
+#include "strings.hh"
 #include "derivations.hh"
 #include "local-store.hh"
 #include "legacy.hh"

src/libcmd/command.cc

@@ -1,3 +1,5 @@
+#include <nlohmann/json.hpp>
+
 #include "command.hh"
 #include "markdown.hh"
 #include "store-api.hh"
@@ -6,8 +8,7 @@
 #include "nixexpr.hh"
 #include "profiles.hh"
 #include "repl.hh"
-
-#include <nlohmann/json.hpp>
+#include "strings.hh"
 
 extern char * * environ __attribute__((weak));
 

src/libcmd/installables.cc

@@ -27,6 +27,8 @@
 
 #include <nlohmann/json.hpp>
 
+#include "strings-inline.hh"
+
 namespace nix {
 
 void completeFlakeInputPath(
@@ -374,6 +376,7 @@ void completeFlakeRefWithFragment(
                         auto attrPath2 = (*attr)->getAttrPath(attr2);
                         /* Strip the attrpath prefix. */
                         attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size());
+                        // FIXME: handle names with dots
                         completions.add(flakeRefS + "#" + prefixRoot + concatStringsSep(".", evalState->symbols.resolve(attrPath2)));
                     }
                 }

src/libcmd/repl.cc

@@ -33,6 +33,8 @@
 #include <gc/gc_cpp.h>
 #endif
 
+#include "strings.hh"
+
 namespace nix {
 
 /**

src/libexpr/eval-cache.cc

@@ -225,7 +225,7 @@ struct AttrDb
                 (key.first)
                 (symbols[key.second])
                 (AttrType::ListOfStrings)
-                (concatStringsSep("\t", l)).exec();
+                (dropEmptyInitThenConcatStringsSep("\t", l)).exec();
 
             return state->db.getLastInsertedRowId();
         });
@@ -435,12 +435,12 @@ std::vector<Symbol> AttrCursor::getAttrPath(Symbol name) const
 
 std::string AttrCursor::getAttrPathStr() const
 {
-    return concatStringsSep(".", root->state.symbols.resolve(getAttrPath()));
+    return dropEmptyInitThenConcatStringsSep(".", root->state.symbols.resolve(getAttrPath()));
 }
 
 std::string AttrCursor::getAttrPathStr(Symbol name) const
 {
-    return concatStringsSep(".", root->state.symbols.resolve(getAttrPath(name)));
+    return dropEmptyInitThenConcatStringsSep(".", root->state.symbols.resolve(getAttrPath(name)));
 }
 
 Value & AttrCursor::forceValue()

src/libexpr/eval.cc

@@ -49,6 +49,8 @@
 
 #endif
 
+#include "strings-inline.hh"
+
 using json = nlohmann::json;
 
 namespace nix {

src/libexpr/nixexpr.cc

@@ -7,6 +7,8 @@
 #include <cstdlib>
 #include <sstream>
 
+#include "strings-inline.hh"
+
 namespace nix {
 
 unsigned long Expr::nrExprs = 0;

src/libexpr/symbol-table.hh

@@ -41,6 +41,11 @@ public:
     }
 
     friend std::ostream & operator <<(std::ostream & os, const SymbolStr & symbol);
+
+    bool empty() const
+    {
+        return s->empty();
+    }
 };
 
 /**

src/libflake/flake/config.cc

@@ -47,7 +47,7 @@ void ConfigFile::apply(const Settings & flakeSettings)
         else if (auto* b = std::get_if<Explicit<bool>>(&value))
             valueS = b->t ? "true" : "false";
         else if (auto ss = std::get_if<std::vector<std::string>>(&value))
-            valueS = concatStringsSep(" ", *ss); // FIXME: evil
+            valueS = dropEmptyInitThenConcatStringsSep(" ", *ss); // FIXME: evil
         else
             assert(false);
 

src/libflake/flake/lockfile.cc

@@ -9,6 +9,8 @@
 #include <iterator>
 #include <nlohmann/json.hpp>
 
+#include "strings.hh"
+
 namespace nix::flake {
 
 static FlakeRef getFlakeRef(

src/libmain/shared.cc

@@ -23,6 +23,7 @@
 #include <openssl/crypto.h>
 
 #include "exit.hh"
+#include "strings.hh"
 
 namespace nix {
 

src/libstore/build/derivation-goal.cc

@@ -32,6 +32,8 @@
 
 #include <nlohmann/json.hpp>
 
+#include "strings.hh"
+
 namespace nix {
 
 DerivationGoal::DerivationGoal(const StorePath & drvPath,

src/libstore/build/entry-points.cc

@@ -4,6 +4,7 @@
 #  include "derivation-goal.hh"
 #endif
 #include "local-store.hh"
+#include "strings.hh"
 
 namespace nix {
 

src/libstore/derivations.cc

@@ -10,6 +10,8 @@
 #include <boost/container/small_vector.hpp>
 #include <nlohmann/json.hpp>
 
+#include "strings-inline.hh"
+
 namespace nix {
 
 std::optional<StorePath> DerivationOutput::path(const StoreDirConfig & store, std::string_view drvName, OutputNameView outputName) const

src/libstore/globals.cc

@@ -35,6 +35,8 @@
 #include <sys/sysctl.h>
 #endif
 
+#include "strings.hh"
+
 namespace nix {
 
 
@@ -82,7 +84,7 @@ Settings::Settings()
         Strings ss;
         for (auto & p : tokenizeString<Strings>(*s, ":"))
             ss.push_back("@" + p);
-        builders = concatStringsSep(" ", ss);
+        builders = concatStringsSep("\n", ss);
     }
 
 #if defined(__linux__) && defined(SANDBOX_SHELL)

src/libstore/local-store.cc

@@ -51,6 +51,8 @@
 
 #include <sqlite3.h>
 
+#include "strings.hh"
+
 
 namespace nix {
 

src/libstore/misc.cc

@@ -10,6 +10,7 @@
 #include "callback.hh"
 #include "closure.hh"
 #include "filetransfer.hh"
+#include "strings.hh"
 
 namespace nix {
 

src/libstore/nar-info-disk-cache.cc

@@ -7,6 +7,8 @@
 #include <sqlite3.h>
 #include <nlohmann/json.hpp>
 
+#include "strings.hh"
+
 namespace nix {
 
 static const char * schema = R"sql(

src/libstore/nar-info.cc

@@ -1,6 +1,7 @@
 #include "globals.hh"
 #include "nar-info.hh"
 #include "store-api.hh"
+#include "strings.hh"
 
 namespace nix {
 

src/libstore/outputs-spec.cc

@@ -5,6 +5,7 @@
 #include "regex-combinators.hh"
 #include "outputs-spec.hh"
 #include "path-regex.hh"
+#include "strings-inline.hh"
 
 namespace nix {
 

src/libstore/path-info.cc

@@ -4,6 +4,7 @@
 #include "store-api.hh"
 #include "json-utils.hh"
 #include "comparator.hh"
+#include "strings.hh"
 
 namespace nix {
 

src/libstore/path-info.hh

@@ -171,6 +171,9 @@ struct ValidPathInfo : UnkeyedValidPathInfo {
      */
     bool checkSignature(const Store & store, const PublicKeys & publicKeys, const std::string & sig) const;
 
+    /**
+     * References as store path basenames, including a self reference if it has one.
+     */
     Strings shortRefs() const;
 
     ValidPathInfo(const ValidPathInfo & other) = default;

src/libstore/path-with-outputs.cc

@@ -1,7 +1,9 @@
+#include <regex>
+
 #include "path-with-outputs.hh"
 #include "store-api.hh"
+#include "strings.hh"
 
-#include <regex>
 
 namespace nix {
 

src/libstore/store-api.cc

@@ -22,6 +22,8 @@
 #include <filesystem>
 #include <nlohmann/json.hpp>
 
+#include "strings.hh"
+
 using json = nlohmann::json;
 
 namespace nix {

src/libstore/unix/build/hook-instance.cc

@@ -3,6 +3,7 @@
 #include "hook-instance.hh"
 #include "file-system.hh"
 #include "child.hh"
+#include "strings.hh"
 
 namespace nix {
 

src/libstore/unix/build/local-derivation-goal.cc

@@ -64,6 +64,8 @@
 #include <grp.h>
 #include <iostream>
 
+#include "strings.hh"
+
 namespace nix {
 
 void handleDiffHook(

src/libutil/canon-path.cc

@@ -1,6 +1,7 @@
 #include "canon-path.hh"
 #include "util.hh"
 #include "file-path-impl.hh"
+#include "strings-inline.hh"
 
 namespace nix {
 

src/libutil/config.cc

@@ -9,6 +9,8 @@
 
 #include <nlohmann/json.hpp>
 
+#include "strings.hh"
+
 namespace nix {
 
 Config::Config(StringMap initials)
@@ -114,7 +116,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p
         if (tokens.empty()) continue;
 
         if (tokens.size() < 2)
-            throw UsageError("illegal configuration line '%1%' in '%2%'", line, path);
+            throw UsageError("syntax error in configuration line '%1%' in '%2%'", line, path);
 
         auto include = false;
         auto ignoreMissing = false;
@@ -127,7 +129,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p
 
         if (include) {
             if (tokens.size() != 2)
-                throw UsageError("illegal configuration line '%1%' in '%2%'", line, path);
+                throw UsageError("syntax error in configuration line '%1%' in '%2%'", line, path);
             auto p = absPath(tokens[1], dirOf(path));
             if (pathExists(p)) {
                 try {
@@ -143,7 +145,7 @@ static void parseConfigFiles(const std::string & contents, const std::string & p
         }
 
         if (tokens[1] != "=")
-            throw UsageError("illegal configuration line '%1%' in '%2%'", line, path);
+            throw UsageError("syntax error in configuration line '%1%' in '%2%'", line, path);
 
         std::string name = std::move(tokens[0]);
 

src/libutil/file-system.cc

@@ -23,6 +23,8 @@
 # include <io.h>
 #endif
 
+#include "strings-inline.hh"
+
 namespace fs = std::filesystem;
 
 namespace nix {

src/libutil/meson.build

@@ -148,6 +148,7 @@ sources = files(
   'signature/signer.cc',
   'source-accessor.cc',
   'source-path.cc',
+  'strings.cc',
   'suggestions.cc',
   'tarfile.cc',
   'terminal.cc',
@@ -215,6 +216,8 @@ headers = [config_h] + files(
   'source-accessor.hh',
   'source-path.hh',
   'split.hh',
+  'strings.hh',
+  'strings-inline.hh',
   'suggestions.hh',
   'sync.hh',
   'tarfile.hh',

src/libutil/strings-inline.hh

@@ -0,0 +1,31 @@
+#pragma once
+
+#include "strings.hh"
+
+namespace nix {
+
+template<class C>
+std::string concatStringsSep(const std::string_view sep, const C & ss)
+{
+    size_t size = 0;
+    bool tail = false;
+    // need a cast to string_view since this is also called with Symbols
+    for (const auto & s : ss) {
+        if (tail)
+            size += sep.size();
+        size += std::string_view(s).size();
+        tail = true;
+    }
+    std::string s;
+    s.reserve(size);
+    tail = false;
+    for (auto & i : ss) {
+        if (tail)
+            s += sep;
+        s += i;
+        tail = true;
+    }
+    return s;
+}
+
+} // namespace nix

src/libutil/strings.cc

@@ -0,0 +1,19 @@
+#include <string>
+
+#include "strings-inline.hh"
+#include "util.hh"
+
+namespace nix {
+
+template std::string concatStringsSep(std::string_view, const Strings &);
+template std::string concatStringsSep(std::string_view, const StringSet &);
+template std::string concatStringsSep(std::string_view, const std::vector<std::string> &);
+
+typedef std::string_view strings_2[2];
+template std::string concatStringsSep(std::string_view, const strings_2 &);
+typedef std::string_view strings_3[3];
+template std::string concatStringsSep(std::string_view, const strings_3 &);
+typedef std::string_view strings_4[4];
+template std::string concatStringsSep(std::string_view, const strings_4 &);
+
+} // namespace nix

src/libutil/strings.hh

@@ -0,0 +1,21 @@
+#pragma once
+
+#include <list>
+#include <set>
+#include <string_view>
+#include <string>
+#include <vector>
+
+namespace nix {
+
+/**
+ * Concatenate the given strings with a separator between the elements.
+ */
+template<class C>
+std::string concatStringsSep(const std::string_view sep, const C & ss);
+
+extern template std::string concatStringsSep(std::string_view, const std::list<std::string> &);
+extern template std::string concatStringsSep(std::string_view, const std::set<std::string> &);
+extern template std::string concatStringsSep(std::string_view, const std::vector<std::string> &);
+
+}

src/libutil/util.hh

@@ -11,6 +11,8 @@
 #include <sstream>
 #include <optional>
 
+#include "strings.hh"
+
 namespace nix {
 
 void initLibUtil();
@@ -33,13 +35,26 @@ template<class C> C tokenizeString(std::string_view s, std::string_view separato
 
 
 /**
- * Concatenate the given strings with a separator between the
- * elements.
+ * Ignore any empty strings at the start of the list, and then concatenate the
+ * given strings with a separator between the elements.
+ *
+ * @deprecated This function exists for historical reasons. You probably just
+ *             want to use `concatStringsSep`.
  */
 template<class C>
-std::string concatStringsSep(const std::string_view sep, const C & ss)
+[[deprecated("Consider removing the empty string dropping behavior. If acceptable, use concatStringsSep instead.")]]
+std::string dropEmptyInitThenConcatStringsSep(const std::string_view sep, const C & ss)
 {
     size_t size = 0;
+
+    // TODO? remove to make sure we don't rely on the empty item ignoring behavior,
+    //       or just get rid of this function by understanding the remaining calls.
+    // for (auto & i : ss) {
+    //     // Make sure we don't rely on the empty item ignoring behavior
+    //     assert(!i.empty());
+    //     break;
+    // }
+
     // need a cast to string_view since this is also called with Symbols
     for (const auto & s : ss) size += sep.size() + std::string_view(s).size();
     std::string s;

src/nix/develop.cc

@@ -19,6 +19,8 @@
 #include <nlohmann/json.hpp>
 #include <algorithm>
 
+#include "strings.hh"
+
 using namespace nix;
 
 struct DevelopSettings : Config

src/nix/diff-closures.cc

@@ -6,6 +6,8 @@
 
 #include <regex>
 
+#include "strings.hh"
+
 namespace nix {
 
 struct Info

src/nix/env.cc

@@ -3,6 +3,7 @@
 
 #include "command.hh"
 #include "run.hh"
+#include "strings.hh"
 
 using namespace nix;
 
@@ -92,6 +93,7 @@ struct CmdShell : InstallablesCommand, MixEnvironment
             }
         }
 
+        // TODO: split losslessly; empty means .
         auto unixPath = tokenizeString<Strings>(getEnv("PATH").value_or(""), ":");
         unixPath.insert(unixPath.begin(), pathAdditions.begin(), pathAdditions.end());
         auto unixPathString = concatStringsSep(":", unixPath);

src/nix/flake.cc

@@ -21,6 +21,8 @@
 #include <nlohmann/json.hpp>
 #include <iomanip>
 
+#include "strings-inline.hh"
+
 using namespace nix;
 using namespace nix::flake;
 using json = nlohmann::json;
@@ -802,6 +804,7 @@ struct CmdFlakeCheck : FlakeCommand
             throw Error("some errors were encountered during the evaluation");
 
         if (!omittedSystems.empty()) {
+            // TODO: empty system is not visible; render all as nix strings?
             warn(
                 "The check omitted these incompatible systems: %s\n"
                 "Use '--all-systems' to check all.",

src/nix/main.cc

@@ -41,6 +41,8 @@ extern std::string chrootHelperName;
 void chrootHelper(int argc, char * * argv);
 #endif
 
+#include "strings.hh"
+
 namespace nix {
 
 enum struct AliasStatus {

src/nix/path-info.cc

@@ -9,6 +9,8 @@
 
 #include <nlohmann/json.hpp>
 
+#include "strings.hh"
+
 using namespace nix;
 using nlohmann::json;
 

src/nix/profile.cc

@@ -17,6 +17,8 @@
 #include <regex>
 #include <iomanip>
 
+#include "strings.hh"
+
 using namespace nix;
 
 struct ProfileElementSource
@@ -58,7 +60,7 @@ struct ProfileElement
         StringSet names;
         for (auto & path : storePaths)
             names.insert(DrvName(path.name()).name);
-        return concatStringsSep(", ", names);
+        return dropEmptyInitThenConcatStringsSep(", ", names);
     }
 
     /**

src/nix/search.cc

@@ -15,6 +15,8 @@
 #include <fstream>
 #include <nlohmann/json.hpp>
 
+#include "strings.hh"
+
 using namespace nix;
 using json = nlohmann::json;
 

tests/unit/libstore/path-info.cc

@@ -26,9 +26,9 @@ static UnkeyedValidPathInfo makeEmpty()
     };
 }
 
-static UnkeyedValidPathInfo makeFull(const Store & store, bool includeImpureInfo)
+static ValidPathInfo makeFullKeyed(const Store & store, bool includeImpureInfo)
 {
-    UnkeyedValidPathInfo info = ValidPathInfo {
+    ValidPathInfo info = ValidPathInfo {
         store,
         "foo",
         FixedOutputInfo {
@@ -57,6 +57,9 @@ static UnkeyedValidPathInfo makeFull(const Store & store, bool includeImpureInfo
     }
     return info;
 }
+static UnkeyedValidPathInfo makeFull(const Store & store, bool includeImpureInfo) {
+    return makeFullKeyed(store, includeImpureInfo);
+}
 
 #define JSON_TEST(STEM, OBJ, PURE)                                        \
     TEST_F(PathInfoTest, PathInfo_ ## STEM ## _from_json) {               \
@@ -86,4 +89,13 @@ JSON_TEST(empty_impure, makeEmpty(), true)
 JSON_TEST(pure, makeFull(*store, false), false)
 JSON_TEST(impure, makeFull(*store, true), true)
 
+TEST_F(PathInfoTest, PathInfo_full_shortRefs) {
+    ValidPathInfo it = makeFullKeyed(*store, true);
+    // it.references = unkeyed.references;
+    auto refs = it.shortRefs();
+    ASSERT_EQ(refs.size(), 2);
+    ASSERT_EQ(*refs.begin(), "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar");
+    ASSERT_EQ(*++refs.begin(), "n5wkd9frr45pa74if5gpz9j7mifg27fh-foo");
 }
+
+} // namespace nix

tests/unit/libutil/references.cc

@@ -15,7 +15,7 @@ struct RewriteParams {
             strRewrites.insert(from + "->" + to);
         return os <<
             "OriginalString: " << bar.originalString << std::endl <<
-            "Rewrites: " << concatStringsSep(",", strRewrites) << std::endl <<
+            "Rewrites: " << dropEmptyInitThenConcatStringsSep(",", strRewrites) << std::endl <<
             "Expected result: " << bar.finalString;
     }
 };

tests/unit/libutil/strings.cc

@@ -0,0 +1,83 @@
+#include <gtest/gtest.h>
+
+#include "strings.hh"
+
+namespace nix {
+
+using Strings = std::vector<std::string>;
+
+/* ----------------------------------------------------------------------------
+ * concatStringsSep
+ * --------------------------------------------------------------------------*/
+
+TEST(concatStringsSep, empty)
+{
+    Strings strings;
+
+    ASSERT_EQ(concatStringsSep(",", strings), "");
+}
+
+TEST(concatStringsSep, justOne)
+{
+    Strings strings;
+    strings.push_back("this");
+
+    ASSERT_EQ(concatStringsSep(",", strings), "this");
+}
+
+TEST(concatStringsSep, emptyString)
+{
+    Strings strings;
+    strings.push_back("");
+
+    ASSERT_EQ(concatStringsSep(",", strings), "");
+}
+
+TEST(concatStringsSep, emptyStrings)
+{
+    Strings strings;
+    strings.push_back("");
+    strings.push_back("");
+
+    ASSERT_EQ(concatStringsSep(",", strings), ",");
+}
+
+TEST(concatStringsSep, threeEmptyStrings)
+{
+    Strings strings;
+    strings.push_back("");
+    strings.push_back("");
+    strings.push_back("");
+
+    ASSERT_EQ(concatStringsSep(",", strings), ",,");
+}
+
+TEST(concatStringsSep, buildCommaSeparatedString)
+{
+    Strings strings;
+    strings.push_back("this");
+    strings.push_back("is");
+    strings.push_back("great");
+
+    ASSERT_EQ(concatStringsSep(",", strings), "this,is,great");
+}
+
+TEST(concatStringsSep, buildStringWithEmptySeparator)
+{
+    Strings strings;
+    strings.push_back("this");
+    strings.push_back("is");
+    strings.push_back("great");
+
+    ASSERT_EQ(concatStringsSep("", strings), "thisisgreat");
+}
+
+TEST(concatStringsSep, buildSingleString)
+{
+    Strings strings;
+    strings.push_back("this");
+
+    ASSERT_EQ(concatStringsSep(",", strings), "this");
+}
+
+} // namespace nix

tests/unit/libutil/tests.cc

@@ -227,32 +227,32 @@ namespace nix {
     }
 
     /* ----------------------------------------------------------------------------
-     * concatStringsSep
+     * dropEmptyInitThenConcatStringsSep
      * --------------------------------------------------------------------------*/
 
-    TEST(concatStringsSep, buildCommaSeparatedString) {
+    TEST(dropEmptyInitThenConcatStringsSep, buildCommaSeparatedString) {
         Strings strings;
         strings.push_back("this");
         strings.push_back("is");
         strings.push_back("great");
 
-        ASSERT_EQ(concatStringsSep(",", strings), "this,is,great");
+        ASSERT_EQ(dropEmptyInitThenConcatStringsSep(",", strings), "this,is,great");
     }
 
-    TEST(concatStringsSep, buildStringWithEmptySeparator) {
+    TEST(dropEmptyInitThenConcatStringsSep, buildStringWithEmptySeparator) {
         Strings strings;
         strings.push_back("this");
         strings.push_back("is");
         strings.push_back("great");
 
-        ASSERT_EQ(concatStringsSep("", strings), "thisisgreat");
+        ASSERT_EQ(dropEmptyInitThenConcatStringsSep("", strings), "thisisgreat");
     }
 
-    TEST(concatStringsSep, buildSingleString) {
+    TEST(dropEmptyInitThenConcatStringsSep, buildSingleString) {
         Strings strings;
         strings.push_back("this");
 
-        ASSERT_EQ(concatStringsSep(",", strings), "this");
+        ASSERT_EQ(dropEmptyInitThenConcatStringsSep(",", strings), "this");
     }
 
     /* ----------------------------------------------------------------------------