From a385e51a086006c0f7d7c4bc6ed910dbe5375c4d Mon Sep 17 00:00:00 2001 From: pennae Date: Fri, 22 Apr 2022 21:45:39 +0200 Subject: [PATCH] rename SymbolIdx -> Symbol, Symbol -> SymbolStr after #6218 `Symbol` no longer confers a uniqueness invariant on the string it wraps, it is now possible to create multiple symbols that compare equal but whose string contents have different addresses. this guarantee is now only provided by `SymbolIdx`, leaving `Symbol` only as a string wrapper that knows about the intricacies of how symbols need to be formatted for output. this change renames `SymbolIdx` to `Symbol` to restore the previous semantics of `Symbol` to that name. we also keep the wrapper type and rename it to `SymbolStr` instead of returning plain strings from lookups into the symbol table because symbols are formatted for output in many places. theoretically we do not need `SymbolStr`, only a function that formats a string for output as a symbol, but having to wrap every symbol that appears in a message into eg `formatSymbol()` is error-prone and inconvient. --- src/libcmd/installables.cc | 6 +-- src/libexpr/attr-path.cc | 2 +- src/libexpr/attr-set.cc | 4 +- src/libexpr/attr-set.hh | 12 +++--- src/libexpr/eval-cache.cc | 46 ++++++++++++----------- src/libexpr/eval-cache.hh | 2 +- src/libexpr/eval.cc | 8 ++-- src/libexpr/eval.hh | 12 +++--- src/libexpr/nixexpr.cc | 19 ++++------ src/libexpr/nixexpr.hh | 34 ++++++++--------- src/libexpr/parser.y | 6 +-- src/libexpr/primops.cc | 8 ++-- src/libexpr/symbol-table.hh | 60 ++++++++++++++++++------------ src/libexpr/value.hh | 15 +++----- src/nix/app.cc | 2 +- src/nix/flake.cc | 74 +++++++++++++++++++------------------ src/nix/repl.cc | 4 +- src/nix/search.cc | 16 ++++---- 18 files changed, 171 insertions(+), 159 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index c81f76a22..693b5e2a2 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -291,7 +291,7 @@ void completeFlakeRefWithFragment( std::string lastAttr; if (!attrPath.empty() && !hasSuffix(attrPathS, ".")) { - lastAttr = attrPath.back(); + lastAttr = evalState->symbols[attrPath.back()]; attrPath.pop_back(); } @@ -299,11 +299,11 @@ void completeFlakeRefWithFragment( if (!attr) continue; for (auto & attr2 : (*attr)->getAttrs()) { - if (hasPrefix(attr2, lastAttr)) { + if (hasPrefix(evalState->symbols[attr2], lastAttr)) { auto attrPath2 = (*attr)->getAttrPath(attr2); /* Strip the attrpath prefix. */ attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size()); - completions->add(flakeRefS + "#" + concatStringsSep(".", attrPath2)); + completions->add(flakeRefS + "#" + concatStringsSep(".", evalState->symbols.resolve(attrPath2))); } } } diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index f63caeb10..94ab60f9a 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -36,7 +36,7 @@ std::vector parseAttrPath(EvalState & state, std::string_view s) { std::vector res; for (auto & a : parseAttrPath(s)) - res.emplace_back(a); + res.push_back(state.symbols.create(a)); return res; } diff --git a/src/libexpr/attr-set.cc b/src/libexpr/attr-set.cc index 1d17ef7e4..61996eae4 100644 --- a/src/libexpr/attr-set.cc +++ b/src/libexpr/attr-set.cc @@ -26,7 +26,7 @@ Bindings * EvalState::allocBindings(size_t capacity) /* Create a new attribute named 'name' on an existing attribute set stored in 'vAttrs' and return the newly allocated Value which is associated with this attribute. */ -Value * EvalState::allocAttr(Value & vAttrs, const SymbolIdx & name) +Value * EvalState::allocAttr(Value & vAttrs, const Symbol & name) { Value * v = allocValue(); vAttrs.attrs->push_back(Attr(name, v)); @@ -40,7 +40,7 @@ Value * EvalState::allocAttr(Value & vAttrs, std::string_view name) } -Value & BindingsBuilder::alloc(const SymbolIdx & name, PosIdx pos) +Value & BindingsBuilder::alloc(const Symbol & name, PosIdx pos) { auto value = state.allocValue(); bindings->push_back(Attr(name, value, pos)); diff --git a/src/libexpr/attr-set.hh b/src/libexpr/attr-set.hh index 3bf23eeb2..31251efc4 100644 --- a/src/libexpr/attr-set.hh +++ b/src/libexpr/attr-set.hh @@ -19,10 +19,10 @@ struct Attr both of them are uint32 wrappers, they are next to each other to make sure that Attr has no padding on 64 bit machines. that way we keep Attr size at two words with no wasted space. */ - SymbolIdx name; + Symbol name; PosIdx pos; Value * value; - Attr(SymbolIdx name, Value * value, PosIdx pos = noPos) + Attr(Symbol name, Value * value, PosIdx pos = noPos) : name(name), pos(pos), value(value) { }; Attr() { }; bool operator < (const Attr & a) const @@ -66,7 +66,7 @@ public: attrs[size_++] = attr; } - iterator find(const SymbolIdx & name) + iterator find(const Symbol & name) { Attr key(name, 0); iterator i = std::lower_bound(begin(), end(), key); @@ -74,7 +74,7 @@ public: return end(); } - Attr * get(const SymbolIdx & name) + Attr * get(const Symbol & name) { Attr key(name, 0); iterator i = std::lower_bound(begin(), end(), key); @@ -128,7 +128,7 @@ public: : bindings(bindings), state(state) { } - void insert(SymbolIdx name, Value * value, PosIdx pos = noPos) + void insert(Symbol name, Value * value, PosIdx pos = noPos) { insert(Attr(name, value, pos)); } @@ -143,7 +143,7 @@ public: bindings->push_back(attr); } - Value & alloc(const SymbolIdx & name, PosIdx pos = noPos); + Value & alloc(const Symbol & name, PosIdx pos = noPos); Value & alloc(std::string_view name, PosIdx pos = noPos); diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 0d2160efd..00e80ae3b 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -92,6 +92,7 @@ struct AttrDb AttrId setAttrs( AttrKey key, + const SymbolTable & symbols, const std::vector & attrs) { return doSQLite([&]() @@ -110,7 +111,7 @@ struct AttrDb for (auto & attr : attrs) state->insertAttribute.use() (rowId) - (attr) + (symbols[attr]) (AttrType::Placeholder) (0, false).exec(); @@ -253,7 +254,7 @@ struct AttrDb std::vector attrs; auto queryAttributes(state->queryAttributes.use()(rowId)); while (queryAttributes.next()) - attrs.emplace_back(queryAttributes.getStr(0)); + attrs.emplace_back(symbols.create(queryAttributes.getStr(0))); return {{rowId, attrs}}; } case AttrType::String: { @@ -331,7 +332,7 @@ AttrKey AttrCursor::getKey() parent->first->getKey(), root->state.symbols); assert(parent->first->cachedValue); } - return {parent->first->cachedValue->first, parent->second}; + return {parent->first->cachedValue->first, root->state.symbols[parent->second]}; } Value & AttrCursor::getValue() @@ -340,7 +341,7 @@ Value & AttrCursor::getValue() if (parent) { auto & vParent = parent->first->getValue(); root->state.forceAttrs(vParent, noPos); - auto attr = vParent.attrs->get(root->state.symbols.create(parent->second)); + auto attr = vParent.attrs->get(parent->second); if (!attr) throw Error("attribute '%s' is unexpectedly missing", getAttrPathStr()); _value = allocRootValue(attr->value); @@ -369,12 +370,12 @@ std::vector AttrCursor::getAttrPath(Symbol name) const std::string AttrCursor::getAttrPathStr() const { - return concatStringsSep(".", getAttrPath()); + return concatStringsSep(".", root->state.symbols.resolve(getAttrPath())); } std::string AttrCursor::getAttrPathStr(Symbol name) const { - return concatStringsSep(".", getAttrPath(name)); + return concatStringsSep(".", root->state.symbols.resolve(getAttrPath(name))); } Value & AttrCursor::forceValue() @@ -414,9 +415,9 @@ Suggestions AttrCursor::getSuggestionsForAttr(Symbol name) auto attrNames = getAttrs(); std::set strAttrNames; for (auto & name : attrNames) - strAttrNames.insert(std::string(name)); + strAttrNames.insert(root->state.symbols[name]); - return Suggestions::bestMatches(strAttrNames, name); + return Suggestions::bestMatches(strAttrNames, root->state.symbols[name]); } std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool forceErrors) @@ -428,11 +429,11 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool if (cachedValue) { if (auto attrs = std::get_if>(&cachedValue->second)) { for (auto & attr : *attrs) - if (attr == name) - return std::make_shared(root, std::make_pair(shared_from_this(), name)); + if (root->state.symbols[attr] == name) + return std::make_shared(root, std::make_pair(shared_from_this(), attr)); return nullptr; } else if (std::get_if(&cachedValue->second)) { - auto attr = root->db->getAttr({cachedValue->first, name}, root->state.symbols); + auto attr = root->db->getAttr({cachedValue->first, std::string(name)}, root->state.symbols); if (attr) { if (std::get_if(&attr->second)) return nullptr; @@ -440,10 +441,10 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool if (forceErrors) debug("reevaluating failed cached attribute '%s'"); else - throw CachedEvalError("cached failure of attribute '%s'", getAttrPathStr(name)); + throw CachedEvalError("cached failure of attribute '%s'", getAttrPathStr(root->state.symbols.create(name))); } else return std::make_shared(root, - std::make_pair(shared_from_this(), name), nullptr, std::move(attr)); + std::make_pair(shared_from_this(), root->state.symbols.create(name)), nullptr, std::move(attr)); } // Incomplete attrset, so need to fall thru and // evaluate to see whether 'name' exists @@ -470,7 +471,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool if (root->db) { if (!cachedValue) cachedValue = {root->db->setPlaceholder(getKey()), placeholder_t()}; - root->db->setMissing({cachedValue->first, name}); + root->db->setMissing({cachedValue->first, std::string(name)}); } return nullptr; } @@ -479,18 +480,18 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool if (root->db) { if (!cachedValue) cachedValue = {root->db->setPlaceholder(getKey()), placeholder_t()}; - cachedValue2 = {root->db->setPlaceholder({cachedValue->first, name}), placeholder_t()}; + cachedValue2 = {root->db->setPlaceholder({cachedValue->first, std::string(name)}), placeholder_t()}; } return make_ref( - root, std::make_pair(shared_from_this(), name), attr->value, std::move(cachedValue2)); + root, std::make_pair(shared_from_this(), root->state.symbols.create(name)), attr->value, std::move(cachedValue2)); } ref AttrCursor::getAttr(std::string_view name, bool forceErrors) { auto p = maybeGetAttr(name, forceErrors); if (!p) - throw Error("attribute '%s' does not exist", getAttrPathStr(name)); + throw Error("attribute '%s' does not exist", getAttrPathStr(root->state.symbols.create(name))); return ref(p); } @@ -498,7 +499,7 @@ OrSuggestions> AttrCursor::findAlongAttrPath(const std::vectormaybeGetAttr(attr, force); + auto child = res->maybeGetAttr(root->state.symbols[attr], force); if (!child) { auto suggestions = res->getSuggestionsForAttr(attr); return OrSuggestions>::failed(suggestions); @@ -606,13 +607,14 @@ std::vector AttrCursor::getAttrs() std::vector attrs; for (auto & attr : *getValue().attrs) - attrs.push_back(root->state.symbols[attr.name]); - std::sort(attrs.begin(), attrs.end(), [](const Symbol & a, const Symbol & b) { - return (const std::string &) a < (const std::string &) b; + attrs.push_back(attr.name); + std::sort(attrs.begin(), attrs.end(), [&](Symbol a, Symbol b) { + std::string_view sa = root->state.symbols[a], sb = root->state.symbols[b]; + return sa < sb; }); if (root->db) - cachedValue = {root->db->setAttrs(getKey(), attrs), attrs}; + cachedValue = {root->db->setAttrs(getKey(), root->state.symbols, attrs), attrs}; return attrs; } diff --git a/src/libexpr/eval-cache.hh b/src/libexpr/eval-cache.hh index f4481c72a..288ecd7e3 100644 --- a/src/libexpr/eval-cache.hh +++ b/src/libexpr/eval-cache.hh @@ -51,7 +51,7 @@ struct missing_t {}; struct misc_t {}; struct failed_t {}; typedef uint64_t AttrId; -typedef std::pair AttrKey; +typedef std::pair AttrKey; typedef std::pair string_t; typedef std::variant< diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 8fc144a84..ef167087b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -308,7 +308,7 @@ static BoehmGCStackAllocator boehmGCStackAllocator; #endif -static SymbolIdx getName(const AttrName & name, EvalState & state, Env & env) +static Symbol getName(const AttrName & name, EvalState & state, Env & env) { if (name.symbol) { return name.symbol; @@ -769,7 +769,7 @@ void EvalState::throwEvalError(const PosIdx pos, const char * s, const std::stri }); } -void EvalState::throwEvalError(const PosIdx p1, const char * s, const SymbolIdx sym, const PosIdx p2) const +void EvalState::throwEvalError(const PosIdx p1, const char * s, const Symbol sym, const PosIdx p2) const { // p1 is where the error occurred; p2 is a position mentioned in the message. throw EvalError({ @@ -787,7 +787,7 @@ void EvalState::throwTypeError(const PosIdx pos, const char * s) const } void EvalState::throwTypeError(const PosIdx pos, const char * s, const ExprLambda & fun, - const SymbolIdx s2) const + const Symbol s2) const { throw TypeError({ .msg = hintfmt(s, fun.showNamePos(*this), symbols[s2]), @@ -796,7 +796,7 @@ void EvalState::throwTypeError(const PosIdx pos, const char * s, const ExprLambd } void EvalState::throwTypeError(const PosIdx pos, const Suggestions & suggestions, const char * s, - const ExprLambda & fun, const SymbolIdx s2) const + const ExprLambda & fun, const Symbol s2) const { throw TypeError(ErrorInfo { .msg = hintfmt(s, fun.showNamePos(*this), symbols[s2]), diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 59c9c4873..5990b97b6 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -78,7 +78,7 @@ public: static inline std::string derivationNixPath = "//builtin/derivation.nix"; - const SymbolIdx sWith, sOutPath, sDrvPath, sType, sMeta, sName, sValue, + const Symbol sWith, sOutPath, sDrvPath, sType, sMeta, sName, sValue, sSystem, sOverrides, sOutputs, sOutputName, sIgnoreNulls, sFile, sLine, sColumn, sFunctor, sToString, sRight, sWrong, sStructuredAttrs, sBuilder, sArgs, @@ -87,7 +87,7 @@ public: sRecurseForDerivations, sDescription, sSelf, sEpsilon, sStartSet, sOperator, sKey, sPath, sPrefix; - SymbolIdx sDerivationNix; + Symbol sDerivationNix; /* If set, force copying files to the Nix store even if they already exist there. */ @@ -269,14 +269,14 @@ public: [[gnu::noinline, gnu::noreturn]] void throwEvalError(const PosIdx pos, const char * s, const std::string & s2, const std::string & s3) const; [[gnu::noinline, gnu::noreturn]] - void throwEvalError(const PosIdx p1, const char * s, const SymbolIdx sym, const PosIdx p2) const; + void throwEvalError(const PosIdx p1, const char * s, const Symbol sym, const PosIdx p2) const; [[gnu::noinline, gnu::noreturn]] void throwTypeError(const PosIdx pos, const char * s) const; [[gnu::noinline, gnu::noreturn]] - void throwTypeError(const PosIdx pos, const char * s, const ExprLambda & fun, const SymbolIdx s2) const; + void throwTypeError(const PosIdx pos, const char * s, const ExprLambda & fun, const Symbol s2) const; [[gnu::noinline, gnu::noreturn]] void throwTypeError(const PosIdx pos, const Suggestions & suggestions, const char * s, - const ExprLambda & fun, const SymbolIdx s2) const; + const ExprLambda & fun, const Symbol s2) const; [[gnu::noinline, gnu::noreturn]] void throwTypeError(const char * s, const Value & v) const; [[gnu::noinline, gnu::noreturn]] @@ -392,7 +392,7 @@ public: inline Value * allocValue(); inline Env & allocEnv(size_t size); - Value * allocAttr(Value & vAttrs, const SymbolIdx & name); + Value * allocAttr(Value & vAttrs, const Symbol & name); Value * allocAttr(Value & vAttrs, std::string_view name); Bindings * allocBindings(size_t capacity); diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 9fee3cb58..c529fdc89 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -24,8 +24,10 @@ static void showString(std::ostream & str, std::string_view s) str << '"'; } -static void showId(std::ostream & str, std::string_view s) +std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) { + std::string_view s = symbol; + if (s.empty()) str << "\"\""; else if (s == "if") // FIXME: handle other keywords @@ -34,7 +36,7 @@ static void showId(std::ostream & str, std::string_view s) char c = s[0]; if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { showString(str, s); - return; + return str; } for (auto c : s) if (!((c >= 'a' && c <= 'z') || @@ -42,15 +44,10 @@ static void showId(std::ostream & str, std::string_view s) (c >= '0' && c <= '9') || c == '_' || c == '\'' || c == '-')) { showString(str, s); - return; + return str; } str << s; } -} - -std::ostream & operator << (std::ostream & str, const Symbol & sym) -{ - showId(str, sym.s); return str; } @@ -499,12 +496,12 @@ void ExprPos::bindVars(const EvalState & es, const StaticEnv & env) /* Storing function names. */ -void Expr::setName(SymbolIdx name) +void Expr::setName(Symbol name) { } -void ExprLambda::setName(SymbolIdx name) +void ExprLambda::setName(Symbol name) { this->name = name; body->setName(name); @@ -526,7 +523,7 @@ std::string ExprLambda::showNamePos(const EvalState & state) const size_t SymbolTable::totalSize() const { size_t n = 0; - dump([&] (const Symbol & s) { n += std::string_view(s).size(); }); + dump([&] (const std::string & s) { n += s.size(); }); return n; } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 217c7e74d..cba099f9c 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -126,9 +126,9 @@ struct StaticEnv; /* An attribute path is a sequence of attribute names. */ struct AttrName { - SymbolIdx symbol; + Symbol symbol; Expr * expr; - AttrName(const SymbolIdx & s) : symbol(s) {}; + AttrName(const Symbol & s) : symbol(s) {}; AttrName(Expr * e) : expr(e) {}; }; @@ -146,7 +146,7 @@ struct Expr virtual void bindVars(const EvalState & es, const StaticEnv & env); virtual void eval(EvalState & state, Env & env, Value & v); virtual Value * maybeThunk(EvalState & state, Env & env); - virtual void setName(SymbolIdx name); + virtual void setName(Symbol name); }; #define COMMON_METHODS \ @@ -196,7 +196,7 @@ typedef uint32_t Displacement; struct ExprVar : Expr { PosIdx pos; - SymbolIdx name; + Symbol name; /* Whether the variable comes from an environment (e.g. a rec, let or function argument) or from a "with". */ @@ -211,8 +211,8 @@ struct ExprVar : Expr Level level; Displacement displ; - ExprVar(const SymbolIdx & name) : name(name) { }; - ExprVar(const PosIdx & pos, const SymbolIdx & name) : pos(pos), name(name) { }; + ExprVar(const Symbol & name) : name(name) { }; + ExprVar(const PosIdx & pos, const Symbol & name) : pos(pos), name(name) { }; COMMON_METHODS Value * maybeThunk(EvalState & state, Env & env); }; @@ -223,7 +223,7 @@ struct ExprSelect : Expr Expr * e, * def; AttrPath attrPath; ExprSelect(const PosIdx & pos, Expr * e, const AttrPath & attrPath, Expr * def) : pos(pos), e(e), def(def), attrPath(attrPath) { }; - ExprSelect(const PosIdx & pos, Expr * e, const SymbolIdx & name) : pos(pos), e(e), def(0) { attrPath.push_back(AttrName(name)); }; + ExprSelect(const PosIdx & pos, Expr * e, const Symbol & name) : pos(pos), e(e), def(0) { attrPath.push_back(AttrName(name)); }; COMMON_METHODS }; @@ -248,7 +248,7 @@ struct ExprAttrs : Expr : inherited(inherited), e(e), pos(pos) { }; AttrDef() { }; }; - typedef std::map AttrDefs; + typedef std::map AttrDefs; AttrDefs attrs; struct DynamicAttrDef { Expr * nameExpr, * valueExpr; @@ -273,7 +273,7 @@ struct ExprList : Expr struct Formal { PosIdx pos; - SymbolIdx name; + Symbol name; Expr * def; }; @@ -283,9 +283,9 @@ struct Formals Formals_ formals; bool ellipsis; - bool has(SymbolIdx arg) const { + bool has(Symbol arg) const { auto it = std::lower_bound(formals.begin(), formals.end(), arg, - [] (const Formal & f, const SymbolIdx & sym) { return f.name < sym; }); + [] (const Formal & f, const Symbol & sym) { return f.name < sym; }); return it != formals.end() && it->name == arg; } @@ -304,11 +304,11 @@ struct Formals struct ExprLambda : Expr { PosIdx pos; - SymbolIdx name; - SymbolIdx arg; + Symbol name; + Symbol arg; Formals * formals; Expr * body; - ExprLambda(PosIdx pos, SymbolIdx arg, Formals * formals, Expr * body) + ExprLambda(PosIdx pos, Symbol arg, Formals * formals, Expr * body) : pos(pos), arg(arg), formals(formals), body(body) { }; @@ -316,7 +316,7 @@ struct ExprLambda : Expr : pos(pos), formals(formals), body(body) { } - void setName(SymbolIdx name); + void setName(Symbol name); std::string showNamePos(const EvalState & state) const; inline bool hasFormals() const { return formals != nullptr; } COMMON_METHODS @@ -426,7 +426,7 @@ struct StaticEnv const StaticEnv * up; // Note: these must be in sorted order. - typedef std::vector> Vars; + typedef std::vector> Vars; Vars vars; StaticEnv(bool isWith, const StaticEnv * up, size_t expectedSize = 0) : isWith(isWith), up(up) { @@ -450,7 +450,7 @@ struct StaticEnv vars.erase(it, end); } - Vars::const_iterator find(const SymbolIdx & name) const + Vars::const_iterator find(const Symbol & name) const { Vars::value_type key(name, 0); auto i = std::lower_bound(vars.begin(), vars.end(), key); diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index b73fd1786..be0598b75 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -86,7 +86,7 @@ static void dupAttr(const EvalState & state, const AttrPath & attrPath, const Po }); } -static void dupAttr(const EvalState & state, SymbolIdx attr, const PosIdx pos, const PosIdx prevPos) +static void dupAttr(const EvalState & state, Symbol attr, const PosIdx pos, const PosIdx prevPos) { throw ParseError({ .msg = hintfmt("attribute '%1%' already defined at %2%", state.symbols[attr], state.positions[prevPos]), @@ -157,14 +157,14 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath, static Formals * toFormals(ParseData & data, ParserFormals * formals, - PosIdx pos = noPos, SymbolIdx arg = {}) + PosIdx pos = noPos, Symbol arg = {}) { std::sort(formals->formals.begin(), formals->formals.end(), [] (const auto & a, const auto & b) { return std::tie(a.name, a.pos) < std::tie(b.name, b.pos); }); - std::optional> duplicate; + std::optional> duplicate; for (size_t i = 0; i + 1 < formals->formals.size(); i++) { if (formals->formals[i].name != formals->formals[i + 1].name) continue; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 36a67a39d..40e9f7091 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -584,7 +584,7 @@ typedef std::list ValueList; static Bindings::iterator getAttr( EvalState & state, std::string_view funcName, - SymbolIdx attrSym, + Symbol attrSym, Bindings * attrSet, const PosIdx pos) { @@ -2047,7 +2047,7 @@ static void prim_path(EvalState & state, const PosIdx pos, Value * * args, Value PathSet context; for (auto & attr : *args[0]->attrs) { - auto & n(state.symbols[attr.name]); + auto n = state.symbols[attr.name]; if (n == "path") path = state.coerceToPath(attr.pos, *attr.value, context); else if (attr.name == state.sName) @@ -2314,7 +2314,7 @@ static void prim_listToAttrs(EvalState & state, const PosIdx pos, Value * * args auto attrs = state.buildBindings(args[0]->listSize()); - std::set seen; + std::set seen; for (auto v2 : args[0]->listItems()) { state.forceAttrs(*v2, pos); @@ -2517,7 +2517,7 @@ static void prim_zipAttrsWith(EvalState & state, const PosIdx pos, Value * * arg // attribute with the merge function application. this way we need not // use (slightly slower) temporary storage the GC does not know about. - std::map> attrsSeen; + std::map> attrsSeen; state.forceFunction(*args[0], pos); state.forceList(*args[1], pos); diff --git a/src/libexpr/symbol-table.hh b/src/libexpr/symbol-table.hh index b6ad60f68..63fb25d73 100644 --- a/src/libexpr/symbol-table.hh +++ b/src/libexpr/symbol-table.hh @@ -12,82 +12,94 @@ namespace nix { /* Symbol table used by the parser and evaluator to represent and look up identifiers and attributes efficiently. SymbolTable::create() converts a string into a symbol. Symbols have the property that - they can be compared efficiently (using a pointer equality test), + they can be compared efficiently (using an equality test), because the symbol table stores only one copy of each string. */ -class Symbol +/* This class mainly exists to give us an operator<< for ostreams. We could also + return plain strings from SymbolTable, but then we'd have to wrap every + instance of a symbol that is fmt()ed, which is inconvenient and error-prone. */ +class SymbolStr { friend class SymbolTable; + private: - std::string s; + const std::string * s; + + explicit SymbolStr(const std::string & symbol): s(&symbol) {} public: - Symbol(std::string_view s) : s(s) { } - - // FIXME: remove bool operator == (std::string_view s2) const { - return s == s2; + return *s == s2; } operator const std::string & () const { - return s; + return *s; } operator const std::string_view () const { - return s; + return *s; } - friend std::ostream & operator << (std::ostream & str, const Symbol & sym); + friend std::ostream & operator <<(std::ostream & os, const SymbolStr & symbol); }; -class SymbolIdx +class Symbol { friend class SymbolTable; private: uint32_t id; - explicit SymbolIdx(uint32_t id): id(id) {} + explicit Symbol(uint32_t id): id(id) {} public: - SymbolIdx() : id(0) {} + Symbol() : id(0) {} explicit operator bool() const { return id > 0; } - bool operator<(const SymbolIdx other) const { return id < other.id; } - bool operator==(const SymbolIdx other) const { return id == other.id; } - bool operator!=(const SymbolIdx other) const { return id != other.id; } + bool operator<(const Symbol other) const { return id < other.id; } + bool operator==(const Symbol other) const { return id == other.id; } + bool operator!=(const Symbol other) const { return id != other.id; } }; class SymbolTable { private: - std::unordered_map> symbols; - ChunkedVector store{16}; + std::unordered_map> symbols; + ChunkedVector store{16}; public: - SymbolIdx create(std::string_view s) + Symbol create(std::string_view s) { // Most symbols are looked up more than once, so we trade off insertion performance // for lookup performance. // TODO: could probably be done more efficiently with transparent Hash and Equals // on the original implementation using unordered_set auto it = symbols.find(s); - if (it != symbols.end()) return SymbolIdx(it->second.second + 1); + if (it != symbols.end()) return Symbol(it->second.second + 1); - const auto & [rawSym, idx] = store.add(s); + const auto & [rawSym, idx] = store.add(std::string(s)); symbols.emplace(rawSym, std::make_pair(&rawSym, idx)); - return SymbolIdx(idx + 1); + return Symbol(idx + 1); } - const Symbol & operator[](SymbolIdx s) const + std::vector resolve(const std::vector & symbols) const + { + std::vector result; + result.reserve(symbols.size()); + for (auto sym : symbols) + result.push_back((*this)[sym]); + return result; + } + + SymbolStr operator[](Symbol s) const { if (s.id == 0 || s.id > store.size()) abort(); - return store[s.id - 1]; + return SymbolStr(store[s.id - 1]); } size_t size() const diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index c46dd4b73..58a8a56a0 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -55,7 +55,7 @@ struct Env; struct Expr; struct ExprLambda; struct PrimOp; -class SymbolIdx; +class Symbol; class PosIdx; struct Pos; class StorePath; @@ -251,11 +251,6 @@ public: void mkStringMove(const char * s, const PathSet & context); - inline void mkString(const Symbol & s) - { - mkString(std::string_view(s).data()); - } - inline void mkPath(const char * s) { clearValue(); @@ -410,12 +405,12 @@ public: #if HAVE_BOEHMGC typedef std::vector > ValueVector; -typedef std::map, traceable_allocator > > ValueMap; -typedef std::map, traceable_allocator > > ValueVectorMap; +typedef std::map, traceable_allocator > > ValueMap; +typedef std::map, traceable_allocator > > ValueVectorMap; #else typedef std::vector ValueVector; -typedef std::map ValueMap; -typedef std::map ValueVectorMap; +typedef std::map ValueMap; +typedef std::map ValueVectorMap; #endif diff --git a/src/nix/app.cc b/src/nix/app.cc index 95ac1cf5c..eec53ad7c 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -66,7 +66,7 @@ UnresolvedApp Installable::toApp(EvalState & state) auto type = cursor->getAttr("type")->getString(); - std::string expected = !attrPath.empty() && attrPath[0] == "apps" ? "app" : "derivation"; + std::string expected = !attrPath.empty() && state.symbols[attrPath[0]] == "apps" ? "app" : "derivation"; if (type != expected) throw Error("attribute '%s' should have type '%s'", cursor->getAttrPathStr(), expected); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index c8d8461e4..2c4a64c85 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -311,7 +311,7 @@ struct CmdFlakeCheck : FlakeCommand return state->positions[p]; }; - auto argHasName = [&] (SymbolIdx arg, std::string_view expected) { + auto argHasName = [&] (Symbol arg, std::string_view expected) { std::string_view name = state->symbols[arg]; return name == expected @@ -986,8 +986,11 @@ struct CmdFlakeShow : FlakeCommand, MixJSON { auto j = nlohmann::json::object(); + auto attrPathS = state->symbols.resolve(attrPath); + Activity act(*logger, lvlInfo, actUnknown, - fmt("evaluating '%s'", concatStringsSep(".", attrPath))); + fmt("evaluating '%s'", concatStringsSep(".", attrPathS))); + try { auto recurse = [&]() { @@ -995,14 +998,15 @@ struct CmdFlakeShow : FlakeCommand, MixJSON logger->cout("%s", headerPrefix); auto attrs = visitor.getAttrs(); for (const auto & [i, attr] : enumerate(attrs)) { + const auto & attrName = state->symbols[attr]; bool last = i + 1 == attrs.size(); - auto visitor2 = visitor.getAttr(attr); + auto visitor2 = visitor.getAttr(attrName); auto attrPath2(attrPath); attrPath2.push_back(attr); auto j2 = visit(*visitor2, attrPath2, - fmt(ANSI_GREEN "%s%s" ANSI_NORMAL ANSI_BOLD "%s" ANSI_NORMAL, nextPrefix, last ? treeLast : treeConn, attr), + fmt(ANSI_GREEN "%s%s" ANSI_NORMAL ANSI_BOLD "%s" ANSI_NORMAL, nextPrefix, last ? treeLast : treeConn, attrName), nextPrefix + (last ? treeNull : treeLine)); - if (json) j.emplace(attr, std::move(j2)); + if (json) j.emplace(attrName, std::move(j2)); } }; @@ -1022,10 +1026,10 @@ struct CmdFlakeShow : FlakeCommand, MixJSON } else { logger->cout("%s: %s '%s'", headerPrefix, - attrPath.size() == 2 && attrPath[0] == "devShell" ? "development environment" : - attrPath.size() >= 2 && attrPath[0] == "devShells" ? "development environment" : - attrPath.size() == 3 && attrPath[0] == "checks" ? "derivation" : - attrPath.size() >= 1 && attrPath[0] == "hydraJobs" ? "derivation" : + attrPath.size() == 2 && attrPathS[0] == "devShell" ? "development environment" : + attrPath.size() >= 2 && attrPathS[0] == "devShells" ? "development environment" : + attrPath.size() == 3 && attrPathS[0] == "checks" ? "derivation" : + attrPath.size() >= 1 && attrPathS[0] == "hydraJobs" ? "derivation" : "package", name); } @@ -1033,27 +1037,27 @@ struct CmdFlakeShow : FlakeCommand, MixJSON if (attrPath.size() == 0 || (attrPath.size() == 1 && ( - attrPath[0] == "defaultPackage" - || attrPath[0] == "devShell" - || attrPath[0] == "formatter" - || attrPath[0] == "nixosConfigurations" - || attrPath[0] == "nixosModules" - || attrPath[0] == "defaultApp" - || attrPath[0] == "templates" - || attrPath[0] == "overlays")) + attrPathS[0] == "defaultPackage" + || attrPathS[0] == "devShell" + || attrPathS[0] == "formatter" + || attrPathS[0] == "nixosConfigurations" + || attrPathS[0] == "nixosModules" + || attrPathS[0] == "defaultApp" + || attrPathS[0] == "templates" + || attrPathS[0] == "overlays")) || ((attrPath.size() == 1 || attrPath.size() == 2) - && (attrPath[0] == "checks" - || attrPath[0] == "packages" - || attrPath[0] == "devShells" - || attrPath[0] == "apps")) + && (attrPathS[0] == "checks" + || attrPathS[0] == "packages" + || attrPathS[0] == "devShells" + || attrPathS[0] == "apps")) ) { recurse(); } else if ( - (attrPath.size() == 2 && (attrPath[0] == "defaultPackage" || attrPath[0] == "devShell" || attrPath[0] == "formatter")) - || (attrPath.size() == 3 && (attrPath[0] == "checks" || attrPath[0] == "packages" || attrPath[0] == "devShells")) + (attrPath.size() == 2 && (attrPathS[0] == "defaultPackage" || attrPathS[0] == "devShell" || attrPathS[0] == "formatter")) + || (attrPath.size() == 3 && (attrPathS[0] == "checks" || attrPathS[0] == "packages" || attrPathS[0] == "devShells")) ) { if (visitor.isDerivation()) @@ -1062,14 +1066,14 @@ struct CmdFlakeShow : FlakeCommand, MixJSON throw Error("expected a derivation"); } - else if (attrPath.size() > 0 && attrPath[0] == "hydraJobs") { + else if (attrPath.size() > 0 && attrPathS[0] == "hydraJobs") { if (visitor.isDerivation()) showDerivation(); else recurse(); } - else if (attrPath.size() > 0 && attrPath[0] == "legacyPackages") { + else if (attrPath.size() > 0 && attrPathS[0] == "legacyPackages") { if (attrPath.size() == 1) recurse(); else if (!showLegacy) @@ -1084,8 +1088,8 @@ struct CmdFlakeShow : FlakeCommand, MixJSON } else if ( - (attrPath.size() == 2 && attrPath[0] == "defaultApp") || - (attrPath.size() == 3 && attrPath[0] == "apps")) + (attrPath.size() == 2 && attrPathS[0] == "defaultApp") || + (attrPath.size() == 3 && attrPathS[0] == "apps")) { auto aType = visitor.maybeGetAttr("type"); if (!aType || aType->getString() != "app") @@ -1098,8 +1102,8 @@ struct CmdFlakeShow : FlakeCommand, MixJSON } else if ( - (attrPath.size() == 1 && attrPath[0] == "defaultTemplate") || - (attrPath.size() == 2 && attrPath[0] == "templates")) + (attrPath.size() == 1 && attrPathS[0] == "defaultTemplate") || + (attrPath.size() == 2 && attrPathS[0] == "templates")) { auto description = visitor.getAttr("description")->getString(); if (json) { @@ -1112,11 +1116,11 @@ struct CmdFlakeShow : FlakeCommand, MixJSON else { auto [type, description] = - (attrPath.size() == 1 && attrPath[0] == "overlay") - || (attrPath.size() == 2 && attrPath[0] == "overlays") ? std::make_pair("nixpkgs-overlay", "Nixpkgs overlay") : - attrPath.size() == 2 && attrPath[0] == "nixosConfigurations" ? std::make_pair("nixos-configuration", "NixOS configuration") : - (attrPath.size() == 1 && attrPath[0] == "nixosModule") - || (attrPath.size() == 2 && attrPath[0] == "nixosModules") ? std::make_pair("nixos-module", "NixOS module") : + (attrPath.size() == 1 && attrPathS[0] == "overlay") + || (attrPath.size() == 2 && attrPathS[0] == "overlays") ? std::make_pair("nixpkgs-overlay", "Nixpkgs overlay") : + attrPath.size() == 2 && attrPathS[0] == "nixosConfigurations" ? std::make_pair("nixos-configuration", "NixOS configuration") : + (attrPath.size() == 1 && attrPathS[0] == "nixosModule") + || (attrPath.size() == 2 && attrPathS[0] == "nixosModules") ? std::make_pair("nixos-module", "NixOS module") : std::make_pair("unknown", "unknown"); if (json) { j.emplace("type", type); @@ -1125,7 +1129,7 @@ struct CmdFlakeShow : FlakeCommand, MixJSON } } } catch (EvalError & e) { - if (!(attrPath.size() > 0 && attrPath[0] == "legacyPackages")) + if (!(attrPath.size() > 0 && attrPathS[0] == "legacyPackages")) throw; } diff --git a/src/nix/repl.cc b/src/nix/repl.cc index 998ff7328..2967632ed 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -73,7 +73,7 @@ struct NixRepl void initEnv(); void reloadFiles(); void addAttrsToScope(Value & attrs); - void addVarToScope(const SymbolIdx name, Value & v); + void addVarToScope(const Symbol name, Value & v); Expr * parseString(std::string s); void evalString(std::string s, Value & v); @@ -711,7 +711,7 @@ void NixRepl::addAttrsToScope(Value & attrs) } -void NixRepl::addVarToScope(const SymbolIdx name, Value & v) +void NixRepl::addVarToScope(const Symbol name, Value & v) { if (displ >= envSize) throw Error("environment full; cannot add more variables"); diff --git a/src/nix/search.cc b/src/nix/search.cc index 8b1e9ae6c..6febc0a55 100644 --- a/src/nix/search.cc +++ b/src/nix/search.cc @@ -77,13 +77,15 @@ struct CmdSearch : InstallableCommand, MixJSON visit = [&](eval_cache::AttrCursor & cursor, const std::vector & attrPath, bool initialRecurse) { + auto attrPathS = state->symbols.resolve(attrPath); + Activity act(*logger, lvlInfo, actUnknown, - fmt("evaluating '%s'", concatStringsSep(".", attrPath))); + fmt("evaluating '%s'", concatStringsSep(".", attrPathS))); try { auto recurse = [&]() { for (const auto & attr : cursor.getAttrs()) { - auto cursor2 = cursor.getAttr(attr); + auto cursor2 = cursor.getAttr(state->symbols[attr]); auto attrPath2(attrPath); attrPath2.push_back(attr); visit(*cursor2, attrPath2, false); @@ -97,7 +99,7 @@ struct CmdSearch : InstallableCommand, MixJSON auto aDescription = aMeta ? aMeta->maybeGetAttr("description") : nullptr; auto description = aDescription ? aDescription->getString() : ""; std::replace(description.begin(), description.end(), '\n', ' '); - auto attrPath2 = concatStringsSep(".", attrPath); + auto attrPath2 = concatStringsSep(".", attrPathS); std::vector attrPathMatches; std::vector descriptionMatches; @@ -146,21 +148,21 @@ struct CmdSearch : InstallableCommand, MixJSON else if ( attrPath.size() == 0 - || (attrPath[0] == "legacyPackages" && attrPath.size() <= 2) - || (attrPath[0] == "packages" && attrPath.size() <= 2)) + || (attrPathS[0] == "legacyPackages" && attrPath.size() <= 2) + || (attrPathS[0] == "packages" && attrPath.size() <= 2)) recurse(); else if (initialRecurse) recurse(); - else if (attrPath[0] == "legacyPackages" && attrPath.size() > 2) { + else if (attrPathS[0] == "legacyPackages" && attrPath.size() > 2) { auto attr = cursor.maybeGetAttr("recurseForDerivations"); if (attr && attr->getBool()) recurse(); } } catch (EvalError & e) { - if (!(attrPath.size() > 0 && attrPath[0] == "legacyPackages")) + if (!(attrPath.size() > 0 && attrPathS[0] == "legacyPackages")) throw; } };