From 2d5cfca98bf8b14e4e12d95c863042bc34fe8aae Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 2 Sep 2022 19:03:41 +0200 Subject: [PATCH] Fix accessing 'toString path' --- src/libexpr/attr-path.cc | 32 +++++++------------- src/libexpr/eval.cc | 21 ++++++++----- src/libexpr/eval.hh | 7 +++++ src/libexpr/paths.cc | 49 +++++++++++++++++++++++++++++++ src/libfetchers/input-accessor.cc | 2 +- tests/local.mk | 3 +- tests/toString-path.sh | 6 ++++ 7 files changed, 89 insertions(+), 31 deletions(-) create mode 100644 tests/toString-path.sh diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index dda650d8d..07f53c8f8 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -118,33 +118,21 @@ std::pair findPackageFilename(EvalState & state, Value & v // FIXME: is it possible to extract the Pos object instead of doing this // toString + parsing? - auto pos = state.forceString(*v2); + PathSet context; + auto path = state.coerceToPath(noPos, *v2, context); - auto fail = [pos]() { - throw ParseError("cannot parse 'meta.position' attribute '%s'", pos); + auto fn = path.path.abs(); + + auto fail = [fn]() { + throw ParseError("cannot parse 'meta.position' attribute '%s'", fn); }; try { - std::string_view prefix = "/virtual/"; - - if (!hasPrefix(pos, prefix)) fail(); - pos = pos.substr(prefix.size()); - - auto slash = pos.find('/'); - if (slash == std::string::npos) fail(); - size_t number = std::stoi(std::string(pos, 0, slash)); - pos = pos.substr(slash); - - auto accessor = state.inputAccessors.find(number); - if (accessor == state.inputAccessors.end()) fail(); - - auto colon = pos.rfind(':'); + auto colon = fn.rfind(':'); if (colon == std::string::npos) fail(); - std::string filename(pos, 0, colon); - auto lineno = std::stoi(std::string(pos, colon + 1, std::string::npos)); - - return {SourcePath{accessor->second, CanonPath(filename)}, lineno}; - + std::string filename(fn, 0, colon); + auto lineno = std::stoi(std::string(fn, colon + 1, std::string::npos)); + return {SourcePath{path.accessor, CanonPath(fn.substr(0, colon))}, lineno}; } catch (std::invalid_argument & e) { fail(); abort(); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 2e5727f19..bc597466b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1098,7 +1098,7 @@ void EvalState::mkPos(Value & v, PosIdx p) auto pos = positions[p]; if (auto path = std::get_if(&pos.origin)) { auto attrs = buildBindings(3); - attrs.alloc(sFile).mkString(fmt("/virtual/%d%s", path->accessor->number, path->path.abs())); + attrs.alloc(sFile).mkString(encodePath(*path)); attrs.alloc(sLine).mkInt(pos.line); attrs.alloc(sColumn).mkInt(pos.column); v.mkAttrs(attrs); @@ -1963,8 +1963,12 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) and none of the strings are allowed to have contexts. */ if (first) { firstType = vTmp->type(); - if (vTmp->type() == nPath) + if (vTmp->type() == nPath) { accessor = vTmp->path().accessor; + auto part = vTmp->path().path.abs(); + sSize += part.size(); + s.emplace_back(std::move(part)); + } } if (firstType == nInt) { @@ -1984,6 +1988,12 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) nf += vTmp->fpoint; } else state.throwEvalError(i_pos, "cannot add %1% to a float", showType(*vTmp), env, *this); + } else if (firstType == nPath) { + if (!first) { + auto part = state.coerceToString(i_pos, *vTmp, context, false, false); + sSize += part->size(); + s.emplace_back(std::move(part)); + } } else { if (s.empty()) s.reserve(es->size()); auto part = state.coerceToString(i_pos, *vTmp, context, false, firstType == nString); @@ -2205,7 +2215,7 @@ BackedStringView EvalState::coerceToString(const PosIdx pos, Value & v, PathSet auto path = v.path(); return copyToStore ? store->printStorePath(copyPathToStore(context, path)) - : BackedStringView((Path) path.path.abs()); + : encodePath(path); } if (v.type() == nAttrs) { @@ -2275,10 +2285,7 @@ SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, PathSet & contex if (v.type() == nString) { copyContext(v, context); - auto path = v.str(); - if (path == "" || path[0] != '/') - throwEvalError(pos, "string '%1%' doesn't represent an absolute path", path); - return {rootFS, CanonPath(path)}; + return decodePath(v.str(), pos); } if (v.type() == nPath) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 2d1c5e1f5..dbf7175cc 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -224,6 +224,13 @@ public: void registerAccessor(ref accessor); + /* Convert a path to a string representation of the format + `/__virtual__//`. */ + std::string encodePath(const SourcePath & path); + + /* Decode a path encoded by `encodePath()`. */ + SourcePath decodePath(std::string_view s, PosIdx pos = noPos); + /* Allow access to a path. */ void allowPath(const Path & path); diff --git a/src/libexpr/paths.cc b/src/libexpr/paths.cc index 4bdcfff78..222638bad 100644 --- a/src/libexpr/paths.cc +++ b/src/libexpr/paths.cc @@ -14,4 +14,53 @@ void EvalState::registerAccessor(ref accessor) inputAccessors.emplace(accessor->number, accessor); } +static constexpr std::string_view marker = "/__virtual__/"; + +std::string EvalState::encodePath(const SourcePath & path) +{ + /* For backward compatibility, return paths in the root FS + normally. Encoding any other path is not very reproducible (due + to /__virtual__/) and we should depreceate it eventually. So + print a warning about use of an encoded path in + decodePath(). */ + return path.accessor == rootFS + ? path.path.abs() + : std::string(marker) + std::to_string(path.accessor->number) + path.path.abs(); +} + +SourcePath EvalState::decodePath(std::string_view s, PosIdx pos) +{ + if (!hasPrefix(s, "/")) + throwEvalError(pos, "string '%1%' doesn't represent an absolute path", s); + + if (hasPrefix(s, marker)) { + auto fail = [s]() { + throw Error("cannot decode virtual path '%s'", s); + }; + + s = s.substr(marker.size()); + + try { + auto slash = s.find('/'); + if (slash == std::string::npos) fail(); + size_t number = std::stoi(std::string(s, 0, slash)); + s = s.substr(slash); + + auto accessor = inputAccessors.find(number); + if (accessor == inputAccessors.end()) fail(); + + SourcePath path {accessor->second, CanonPath(s)}; + + static bool warned = false; + warnOnce(warned, fmt("applying 'toString' to path '%s' and then accessing it is deprecated, at %s", path, positions[pos])); + + return path; + } catch (std::invalid_argument & e) { + fail(); + abort(); + } + } else + return {rootFS, CanonPath(s)}; +} + } diff --git a/src/libfetchers/input-accessor.cc b/src/libfetchers/input-accessor.cc index 4ded83bfe..f42281ecc 100644 --- a/src/libfetchers/input-accessor.cc +++ b/src/libfetchers/input-accessor.cc @@ -11,7 +11,7 @@ static std::atomic nextNumber{0}; InputAccessor::InputAccessor() : number(++nextNumber) - , displayPrefix{"/virtual/" + std::to_string(number)} + , displayPrefix{"«unknown»"} { } diff --git a/tests/local.mk b/tests/local.mk index 5e48ceae1..b2fcb620a 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -109,7 +109,8 @@ nix_tests = \ store-ping.sh \ fetchClosure.sh \ completions.sh \ - impure-derivations.sh + impure-derivations.sh \ + toString-path.sh ifeq ($(HAVE_LIBCPUID), 1) nix_tests += compute-levels.sh diff --git a/tests/toString-path.sh b/tests/toString-path.sh new file mode 100644 index 000000000..607e596f8 --- /dev/null +++ b/tests/toString-path.sh @@ -0,0 +1,6 @@ +source common.sh + +mkdir -p $TEST_ROOT/foo +echo bla > $TEST_ROOT/foo/bar + +[[ $(nix eval --raw --impure --expr "builtins.readFile (builtins.toString (builtins.fetchTree { type = \"path\"; path = \"$TEST_ROOT/foo\"; } + \"bar\"))") = bla ]]