libexpr: don't read Value::string.s directly

Change-Id: I0f224459fcbff8bc53c3668bc6ea52881c453fd0

lix/legacy/nix-env.cc

@@ -1267,7 +1267,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                         else {
                             if (v->type() == nString) {
                                 attrs2["type"] = "string";
-                                attrs2["value"] = v->string.s;
+                                attrs2["value"] = v->str();
                                 xml.writeEmptyElement("meta", attrs2);
                             } else if (v->type() == nInt) {
                                 attrs2["type"] = "int";
@@ -1287,7 +1287,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                 for (auto elem : v->listItems()) {
                                     if (elem->type() != nString) continue;
                                     XMLAttrs attrs3;
-                                    attrs3["value"] = elem->string.s;
+                                    attrs3["value"] = elem->str();
                                     xml.writeEmptyElement("string", attrs3);
                                 }
                             } else if (v->type() == nAttrs) {
@@ -1299,7 +1299,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                     if(a.value->type() != nString) continue;
                                     XMLAttrs attrs3;
                                     attrs3["type"] = globals.state->symbols[i.name];
-                                    attrs3["value"] = a.value->string.s;
+                                    attrs3["value"] = a.value->str();
                                     xml.writeEmptyElement("string", attrs3);
                             }
                             }

lix/libcmd/repl.cc

@@ -788,7 +788,7 @@ ProcessLineResult NixRepl::processLine(std::string line)
         Value v;
         evalString(arg, v);
         if (v.type() == nString) {
-            std::cout << v.string.s;
+            std::cout << v.str();
         } else {
             printValue(std::cout, v);
         }

lix/libexpr/eval-cache.cc

@@ -437,8 +437,9 @@ Value & AttrCursor::forceValue(EvalState & state)
 
     if (root->db && (!cachedValue || std::get_if<placeholder_t>(&cachedValue->second))) {
         if (v.type() == nString)
-            cachedValue = {root->db->setString(getKey(), v.string.s, v.string.context),
-                           string_t{v.string.s, {}}};
+            cachedValue = {
+                root->db->setString(getKey(), v.str(), v.string.context), string_t{v.str(), {}}
+            };
         else if (v.type() == nPath) {
             auto path = v.path().canonical().abs();
             cachedValue = {root->db->setString(getKey(), path), string_t{path, {}}};
@@ -563,7 +564,7 @@ std::string AttrCursor::getString(EvalState & state)
         state.ctx.errors.make<TypeError>("'%s' is not a string but %s", getAttrPathStr(state), v.type()).debugThrow();
     }
 
-    return v.type() == nString ? v.string.s : v.path().to_string();
+    return v.type() == nString ? std::string(v.str()) : v.path().to_string();
 }
 
 string_t AttrCursor::getStringWithContext(EvalState & state)
@@ -605,7 +606,7 @@ string_t AttrCursor::getStringWithContext(EvalState & state)
     if (v.type() == nString) {
         NixStringContext context;
         copyContext(v, context);
-        return {v.string.s, std::move(context)};
+        return {std::string(v.str()), std::move(context)};
     } else if (v.type() == nPath) {
         return {v.path().to_string(), {}};
     } else {

lix/libexpr/eval.cc

@@ -140,7 +140,7 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env)
         Value nameValue;
         name.expr->eval(state, env, nameValue);
         state.forceStringNoCtx(nameValue, name.expr->getPos(), "while evaluating an attribute name");
-        return state.ctx.symbols.create(nameValue.string.s);
+        return state.ctx.symbols.create(nameValue.str());
     }
 }
 
@@ -1152,7 +1152,7 @@ void ExprSet::eval(EvalState & state, Env & env, Value & v)
         if (nameVal.type() == nNull)
             continue;
         state.forceStringNoCtx(nameVal, i.pos, "while evaluating the name of a dynamic attribute");
-        auto nameSym = state.ctx.symbols.create(nameVal.string.s);
+        auto nameSym = state.ctx.symbols.create(nameVal.str());
         Bindings::iterator j = v.attrs->find(nameSym);
         if (j != v.attrs->end())
             state.ctx.errors.make<EvalError>("dynamic attribute '%1%' already defined at %2%", state.ctx.symbols[nameSym], state.ctx.positions[j->pos]).atPos(i.pos).withFrame(env, *this).debugThrow();
@@ -2214,7 +2214,7 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string
                 showType(v),
                 ValuePrinter(*this, v, errorPrintOptions)
             ).atPos(pos).debugThrow();
-        return v.string.s;
+        return v.str();
     } catch (Error & e) {
         e.addTrace(ctx.positions[pos], errorCtx);
         throw;
@@ -2242,7 +2242,14 @@ std::string_view EvalState::forceStringNoCtx(Value & v, const PosIdx pos, std::s
 {
     auto s = forceString(v, pos, errorCtx);
     if (v.string.context) {
-        ctx.errors.make<EvalError>("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string.s, v.string.context[0]).withTrace(pos, errorCtx).debugThrow();
+        ctx.errors
+            .make<EvalError>(
+                "the string '%1%' is not allowed to refer to a store path (such as '%2%')",
+                v.str(),
+                v.string.context[0]
+            )
+            .withTrace(pos, errorCtx)
+            .debugThrow();
     }
     return s;
 }
@@ -2255,7 +2262,7 @@ bool EvalState::isDerivation(Value & v)
     if (i == v.attrs->end()) return false;
     forceValue(*i->value, i->pos);
     if (i->value->type() != nString) return false;
-    return strcmp(i->value->string.s, "derivation") == 0;
+    return i->value->str() == "derivation";
 }
 
 
@@ -2292,7 +2299,7 @@ BackedStringView EvalState::coerceToString(
 
     if (v.type() == nString) {
         copyContext(v, context);
-        return std::string_view(v.string.s);
+        return v.str();
     }
 
     if (v.type() == nPath) {
@@ -2511,7 +2518,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v
             return v1.boolean == v2.boolean;
 
         case nString:
-            return strcmp(v1.string.s, v2.string.s) == 0;
+            return v1.str() == v2.str();
 
         case nPath:
             return strcmp(v1._path, v2._path) == 0;

lix/libexpr/flake/flake.cc

@@ -111,7 +111,7 @@ static void parseFlakeInputAttr(EvalState & state, const Attr & attr, fetchers::
 #pragma GCC diagnostic ignored "-Wswitch-enum"
     switch (attr.value->type()) {
     case nString:
-        attrs.emplace(state.ctx.symbols[attr.name], attr.value->string.s);
+        attrs.emplace(state.ctx.symbols[attr.name], std::string(attr.value->str()));
         break;
     case nBool:
         attrs.emplace(state.ctx.symbols[attr.name], Explicit<bool>{attr.value->boolean});
@@ -164,7 +164,7 @@ static FlakeInput parseFlakeInput(EvalState & state,
         try {
             if (attr.name == sUrl) {
                 expectType(state, nString, *attr.value, attr.pos);
-                url = attr.value->string.s;
+                url = attr.value->str();
                 attrs.emplace("url", *url);
             } else if (attr.name == sFlake) {
                 expectType(state, nBool, *attr.value, attr.pos);
@@ -177,7 +177,7 @@ static FlakeInput parseFlakeInput(EvalState & state,
                         .first;
             } else if (attr.name == sFollows) {
                 expectType(state, nString, *attr.value, attr.pos);
-                auto follows(parseInputPath(attr.value->string.s));
+                auto follows(parseInputPath(attr.value->str()));
                 follows.insert(follows.begin(), lockRootPath.begin(), lockRootPath.end());
                 input.follows = follows;
             } else {
@@ -330,7 +330,7 @@ static Flake getFlake(
 
     if (auto description = vInfo.attrs->get(state.ctx.s.description)) {
         expectType(state, nString, *description->value, description->pos);
-        flake.description = description->value->string.s;
+        flake.description = description->value->str();
     }
 
     auto sInputs = state.ctx.symbols.create("inputs");

lix/libexpr/get-drvs.cc

@@ -244,8 +244,10 @@ DrvInfo::Outputs DrvInfo::queryOutputs(EvalState & state, bool withPaths, bool o
                 errMsg + "element is %s where a string was expected",
                 Uncolored(showType(elem->type()))
             );
-        auto out = outputs.find(elem->string.s);
-        if (out == outputs.end()) throw Error(errMsg + "output '%s' does not exist", elem->string.s);
+        auto out = outputs.find(std::string(elem->str()));
+        if (out == outputs.end()) {
+            throw Error(errMsg + "output '%s' does not exist", elem->str());
+        }
         result.insert(*out);
     }
     return result;
@@ -317,7 +319,7 @@ std::string DrvInfo::queryMetaString(EvalState & state, const std::string & name
 {
     Value * v = queryMeta(state, name);
     if (!v || v->type() != nString) return "";
-    return v->string.s;
+    return std::string(v->str());
 }
 
 
@@ -329,8 +331,9 @@ NixInt DrvInfo::queryMetaInt(EvalState & state, const std::string & name, NixInt
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            integer meta fields. */
-        if (auto n = string2Int<NixInt::Inner>(v->string.s))
+        if (auto n = string2Int<NixInt::Inner>(v->str())) {
             return NixInt{*n};
+        }
     }
     return def;
 }
@@ -343,8 +346,12 @@ bool DrvInfo::queryMetaBool(EvalState & state, const std::string & name, bool de
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            Boolean meta fields. */
-        if (strcmp(v->string.s, "true") == 0) return true;
-        if (strcmp(v->string.s, "false") == 0) return false;
+        if (v->str() == "true") {
+            return true;
+        }
+        if (v->str() == "false") {
+            return false;
+        }
     }
     return def;
 }

lix/libexpr/nixexpr.cc

@@ -55,7 +55,7 @@ JSON ExprLiteral::toJSON(const SymbolTable & symbols) const
             break;
         case nString:
             valueType = "String";
-            value = v.string.s;
+            value = v.str();
             break;
         case nPath:
             valueType = "Path";

lix/libexpr/primops.cc

@@ -453,7 +453,7 @@ struct CompareValues : NeverAsync
                 case nFloat:
                     return v1->fpoint < v2->fpoint;
                 case nString:
-                    return strcmp(v1->string.s, v2->string.s) < 0;
+                    return v1->str() < v2->str();
                 case nPath:
                     return strcmp(v1->_path, v2->_path) < 0;
                 case nList:
@@ -695,7 +695,7 @@ static void prim_trace(EvalState & state, Value * * args, Value & v)
 {
     state.forceValue(*args[0], noPos);
     if (args[0]->type() == nString)
-        printError("trace: %1%", args[0]->string.s);
+        printError("trace: %1%", args[0]->str());
     else
         printError("trace: %1%", ValuePrinter(state, *args[0]));
     if (auto last = evalSettings.builtinsTraceDebugger && state.ctx.debug
@@ -1584,8 +1584,9 @@ static void prim_attrNames(EvalState & state, Value * * args, Value & v)
     for (auto & i : *args[0]->attrs)
         v.listElems()[n++] = const_cast<Value *>(state.ctx.symbols[i.name].toValuePtr());
 
-    std::sort(v.listElems(), v.listElems() + n,
-              [](Value * v1, Value * v2) { return strcmp(v1->string.s, v2->string.s) < 0; });
+    std::sort(v.listElems(), v.listElems() + n, [](Value * v1, Value * v2) {
+        return v1->str() < v2->str();
+    });
 }
 
 /* Return the values of the attributes in a set as a list, in the same
@@ -1719,7 +1720,7 @@ static void prim_removeAttrs(EvalState & state, Value * * args, Value & v)
     names.reserve(args[1]->listSize());
     for (auto elem : args[1]->listItems()) {
         state.forceStringNoCtx(*elem, noPos, "while evaluating the values of the second argument passed to builtins.removeAttrs");
-        names.emplace_back(state.ctx.symbols.create(elem->string.s), nullptr);
+        names.emplace_back(state.ctx.symbols.create(elem->str()), nullptr);
     }
     std::sort(names.begin(), names.end());
 

lix/libexpr/primops/fetchClosure.cc

@@ -131,7 +131,7 @@ void prim_fetchClosure(EvalState & state, Value * * args, Value & v)
 
         else if (attrName == "toPath") {
             state.forceValue(*attr.value, attr.pos);
-            bool isEmptyString = attr.value->type() == nString && attr.value->string.s == std::string("");
+            bool isEmptyString = attr.value->type() == nString && attr.value->str().empty();
             if (isEmptyString) {
                 toPath = StorePathOrGap {};
             }

lix/libexpr/print-ambiguous.cc

@@ -29,7 +29,7 @@ void printAmbiguous(
         printLiteralBool(str, v.boolean);
         break;
     case nString:
-        escapeString(str, v.string.s);
+        escapeString(str, v.str());
         break;
     case nPath:
         str << v.path().to_string(); // !!! escaping?

lix/libexpr/print.cc

@@ -202,10 +202,9 @@ private:
     {
         escapeString(
             output,
-            v.string.s,
+            v.str(),
             {
-                .maxLength = options.maxStringLength,
-                .outputAnsiColors = options.ansiColors,
+                .maxLength = options.maxStringLength, .outputAnsiColors = options.ansiColors,
                 // NB: Non-printing characters won't be escaped.
             }
         );

lix/libexpr/value-to-json.cc

@@ -28,7 +28,7 @@ JSON printValueAsJSON(EvalState & state, bool strict,
 
         case nString:
             copyContext(v, context);
-            out = v.string.s;
+            out = v.str();
             break;
 
         case nPath:

lix/libexpr/value-to-xml.cc

@@ -5,8 +5,7 @@
 
 namespace nix {
 
-
-static XMLAttrs singletonAttrs(const std::string & name, const std::string & value)
+static XMLAttrs singletonAttrs(const std::string & name, const std::string_view value)
 {
     XMLAttrs attrs;
     attrs[name] = value;
@@ -71,7 +70,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location,
         case nString:
             /* !!! show the context? */
             copyContext(v, context);
-            doc.writeEmptyElement("string", singletonAttrs("value", v.string.s));
+            doc.writeEmptyElement("string", singletonAttrs("value", v.str()));
             break;
 
         case nPath:
@@ -93,14 +92,15 @@ static void printValueAsXML(EvalState & state, bool strict, bool location,
                 if (a != v.attrs->end()) {
                     if (strict) state.forceValue(*a->value, a->pos);
                     if (a->value->type() == nString)
-                        xmlAttrs["drvPath"] = drvPath = a->value->string.s;
+                        xmlAttrs["drvPath"] = drvPath = a->value->str();
                 }
 
                 a = v.attrs->find(state.ctx.s.outPath);
                 if (a != v.attrs->end()) {
                     if (strict) state.forceValue(*a->value, a->pos);
-                    if (a->value->type() == nString)
-                        xmlAttrs["outPath"] = a->value->string.s;
+                    if (a->value->type() == nString) {
+                        xmlAttrs["outPath"] = a->value->str();
+                    }
                 }
 
                 XMLOpenElement _(doc, "derivation", xmlAttrs);

lix/libexpr/value.hh

@@ -284,7 +284,7 @@ public:
     /// enabled), and string and context data copied into that memory.
     Value(string_t, char const * strPtr, char const ** contextPtr = nullptr)
         : internalType(tString)
-        , string({ .s = strPtr, .context = contextPtr })
+        , string({.content = strPtr, .context = contextPtr})
     { }
 
     /// Constructx a nix language value of type "string", with a copy of the
@@ -294,7 +294,7 @@ public:
     /// performs a dynamic (GC) allocation to do so.
     Value(string_t, std::string_view copyFrom, NixStringContext const & context = {})
         : internalType(tString)
-        , string({ .s = gcCopyStringIfNeeded(copyFrom), .context = nullptr })
+        , string({.content = gcCopyStringIfNeeded(copyFrom), .context = nullptr})
     {
         if (context.empty()) {
             // It stays nullptr.
@@ -325,7 +325,7 @@ public:
     /// to do so.
     Value(string_t, char const * strPtr, NixStringContext const & context)
         : internalType(tString)
-        , string({ .s = strPtr, .context = nullptr })
+        , string({.content = strPtr, .context = nullptr})
     {
         if (context.empty()) {
             // It stays nullptr
@@ -583,7 +583,7 @@ public:
          * For canonicity, the store paths should be in sorted order.
          */
         struct {
-            const char * s;
+            const char * content;
             const char * * context; // must be in sorted order
         } string;
 
@@ -687,7 +687,7 @@ public:
     inline void mkString(const char * s, const char * * context = 0)
     {
         internalType = tString;
-        string.s = s;
+        string.content = s;
         string.context = context;
     }
 
@@ -853,7 +853,7 @@ public:
     std::string_view str() const
     {
         assert(internalType == tString);
-        return std::string_view(string.s);
+        return std::string_view(string.content);
     }
 };
 

lix/nix/eval.cc

@@ -88,7 +88,7 @@ struct CmdEval : MixJSON, InstallableCommand, MixReadOnlyOption
                 state->forceValue(v, pos);
                 if (v.type() == nString)
                     // FIXME: disallow strings with contexts?
-                    writeFile(path, v.string.s);
+                    writeFile(path, v.str());
                 else if (v.type() == nAttrs) {
                     if (mkdir(path.c_str(), 0777) == -1)
                         throw SysError("creating directory '%s'", path);

subprojects/nix-eval-jobs/src/worker.cc

@@ -113,7 +113,7 @@ readConstituents(const nix::Value *v, nix::box_ptr<nix::EvalState> &state,
             auto v = a->value->listElems()[n];
             state->forceValue(*v, nix::noPos);
             if (v->type() == nix::nString)
-                namedConstituents.push_back(v->string.s);
+                namedConstituents.emplace_back(v->str());
         }
 
         return Constituents(constituents, namedConstituents);

tests/unit/libexpr-support/tests/libexpr.hh

@@ -77,7 +77,7 @@ namespace nix {
         if (arg.type() != nString) {
             return false;
         }
-        return std::string_view(arg.string.s) == std::string_view(s);
+        return arg.str() == std::string_view(s);
     }
 
     MATCHER_P(IsIntEq, v, fmt("The string is equal to \"%1%\"", v)) {
@@ -113,7 +113,8 @@ namespace nix {
                 *result_listener << "Expected a path got " << arg.type();
                 return false;
             } else if (std::string_view(arg._path) != p) {
-                *result_listener << "Expected a path that equals \"" << p << "\" but got: " << arg.string.s;
+                *result_listener << "Expected a path that equals \"" << p
+                                 << "\" but got: " << arg.path();
                 return false;
             }
             return true;

tests/unit/libexpr/primops.cc

@@ -726,14 +726,14 @@ namespace nix {
         // FIXME: add a test that verifies the string context is as expected
         auto v = eval("builtins.replaceStrings [\"oo\" \"a\"] [\"a\" \"i\"] \"foobar\"");
         ASSERT_EQ(v.type(), nString);
-        ASSERT_EQ(v.string.s, std::string_view("fabir"));
+        ASSERT_EQ(v.str(), std::string_view("fabir"));
     }
 
     TEST_F(PrimOpTest, concatStringsSep) {
         // FIXME: add a test that verifies the string context is as expected
         auto v = eval("builtins.concatStringsSep \"%\" [\"foo\" \"bar\" \"baz\"]");
         ASSERT_EQ(v.type(), nString);
-        ASSERT_EQ(std::string_view(v.string.s), "foo%bar%baz");
+        ASSERT_EQ(v.str(), "foo%bar%baz");
     }
 
     TEST_F(PrimOpTest, split1) {