From 3e45b40d6646f9298a6810998124b90d500117c3 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 30 May 2025 17:31:34 +0200 Subject: [PATCH] Add position info to path values (Actually, this adds a position field to *all* values.) This allows improving the "inefficient double copy" warning by showing where the source path came from in the source, e.g. warning: Performing inefficient double copy of path '/home/eelco/Dev/patchelf/' to the store at /home/eelco/Dev/patchelf/flake.nix:30:17. This can typically be avoided by rewriting an attribute like `src = ./.` to `src = builtins.path { path = ./.; name = "source"; }`. --- src/libexpr/eval.cc | 10 ++++++---- src/libexpr/include/nix/expr/eval.hh | 4 ++-- src/libexpr/include/nix/expr/nixexpr.hh | 4 ++-- src/libexpr/include/nix/expr/value.hh | 11 ++++++++--- src/libexpr/parser.y | 6 +++--- src/libexpr/paths.cc | 7 ++++--- src/libexpr/primops.cc | 2 +- src/libexpr/value-to-json.cc | 2 +- src/libutil/include/nix/util/pos-idx.hh | 7 ++++++- 9 files changed, 33 insertions(+), 20 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 868933b95..fcc935add 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -149,6 +149,8 @@ PosIdx Value::determinePos(const PosIdx pos) const // Allow selecting a subset of enum values #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wswitch-enum" + if (this->pos != 0) + return PosIdx(this->pos); switch (internalType) { case tAttrs: return attrs()->pos; case tLambda: return payload.lambda.fun->pos; @@ -906,7 +908,7 @@ void Value::mkStringMove(const char * s, const NixStringContext & context) void Value::mkPath(const SourcePath & path) { - mkPath(&*path.accessor, makeImmutableString(path.path.abs())); + mkPath(&*path.accessor, makeImmutableString(path.path.abs()), noPos.get()); } @@ -2356,7 +2358,7 @@ BackedStringView EvalState::coerceToString( // slash, as in /foo/${x}. v.payload.path.path : copyToStore - ? store->printStorePath(copyPathToStore(context, v.path())) + ? store->printStorePath(copyPathToStore(context, v.path(), v.determinePos(pos))) : ({ auto path = v.path(); if (path.accessor == rootFS && store->isInStore(path.path.abs())) { @@ -2434,7 +2436,7 @@ BackedStringView EvalState::coerceToString( } -StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePath & path) +StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePath & path, PosIdx pos) { if (nix::isDerivation(path.path.abs())) error("file names are not allowed to end in '%1%'", drvExtension).debugThrow(); @@ -2448,7 +2450,7 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat *store, path.resolveSymlinks(SymlinkResolution::Ancestors), settings.readOnlyMode ? FetchMode::DryRun : FetchMode::Copy, - computeBaseName(path), + computeBaseName(path, pos), ContentAddressMethod::Raw::NixArchive, nullptr, repair); diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index d82baddb1..58f88a5a3 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -594,7 +594,7 @@ public: bool coerceMore = false, bool copyToStore = true, bool canonicalizePath = true); - StorePath copyPathToStore(NixStringContext & context, const SourcePath & path); + StorePath copyPathToStore(NixStringContext & context, const SourcePath & path, PosIdx pos); /** @@ -607,7 +607,7 @@ public: * materialize /nix/store/-source though. Still, this * requires reading/hashing the path twice. */ - std::string computeBaseName(const SourcePath & path); + std::string computeBaseName(const SourcePath & path, PosIdx pos); /** * Path coercion. diff --git a/src/libexpr/include/nix/expr/nixexpr.hh b/src/libexpr/include/nix/expr/nixexpr.hh index a5ce0fd89..090681470 100644 --- a/src/libexpr/include/nix/expr/nixexpr.hh +++ b/src/libexpr/include/nix/expr/nixexpr.hh @@ -138,9 +138,9 @@ struct ExprPath : Expr ref accessor; std::string s; Value v; - ExprPath(ref accessor, std::string s) : accessor(accessor), s(std::move(s)) + ExprPath(ref accessor, std::string s, PosIdx pos) : accessor(accessor), s(std::move(s)) { - v.mkPath(&*accessor, this->s.c_str()); + v.mkPath(&*accessor, this->s.c_str(), pos.get()); } Value * maybeThunk(EvalState & state, Env & env) override; COMMON_METHODS diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index e9cc1cd3f..6fe9b6b6f 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -167,6 +167,7 @@ struct Value { private: InternalType internalType = tUninitialized; + uint32_t pos{0}; friend std::string showType(const Value & v); @@ -289,10 +290,11 @@ public: unreachable(); } - inline void finishValue(InternalType newType, Payload newPayload) + inline void finishValue(InternalType newType, Payload newPayload, uint32_t newPos = 0) { payload = newPayload; internalType = newType; + pos = newPos; } /** @@ -339,9 +341,9 @@ public: void mkPath(const SourcePath & path); void mkPath(std::string_view path); - inline void mkPath(SourceAccessor * accessor, const char * path) + inline void mkPath(SourceAccessor * accessor, const char * path, uint32_t pos) { - finishValue(tPath, { .path = { .accessor = accessor, .path = path } }); + finishValue(tPath, { .path = { .accessor = accessor, .path = path } }, pos); } inline void mkNull() @@ -482,6 +484,9 @@ public: NixFloat fpoint() const { return payload.fpoint; } + + inline uint32_t getPos() const + { return pos; } }; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 8878b86c2..e9be2837c 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -374,8 +374,8 @@ path_start root filesystem accessor, rather than the accessor of the current Nix expression. */ literal.front() == '/' - ? new ExprPath(state->rootFS, std::move(path)) - : new ExprPath(state->basePath.accessor, std::move(path)); + ? new ExprPath(state->rootFS, std::move(path), CUR_POS) + : new ExprPath(state->basePath.accessor, std::move(path), CUR_POS); } | HPATH { if (state->settings.pureEval) { @@ -385,7 +385,7 @@ path_start ); } Path path(getHome() + std::string($1.p + 1, $1.l - 1)); - $$ = new ExprPath(ref(state->rootFS), std::move(path)); + $$ = new ExprPath(ref(state->rootFS), std::move(path), CUR_POS); } ; diff --git a/src/libexpr/paths.cc b/src/libexpr/paths.cc index 3aaca2328..e7dfa549c 100644 --- a/src/libexpr/paths.cc +++ b/src/libexpr/paths.cc @@ -52,15 +52,16 @@ std::string EvalState::devirtualize(std::string_view s, const NixStringContext & return rewriteStrings(std::string(s), rewrites); } -std::string EvalState::computeBaseName(const SourcePath & path) +std::string EvalState::computeBaseName(const SourcePath & path, PosIdx pos) { if (path.accessor == rootFS) { if (auto storePath = store->maybeParseStorePath(path.path.abs())) { warn( - "Performing inefficient double copy of path '%s' to the store. " + "Performing inefficient double copy of path '%s' to the store at %s. " "This can typically be avoided by rewriting an attribute like `src = ./.` " "to `src = builtins.path { path = ./.; name = \"source\"; }`.", - path); + path, + positions[pos]); return std::string(fetchToStore(*store, path, FetchMode::DryRun, storePath->name()).to_string()); } } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 586952386..bd4168a44 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2620,7 +2620,7 @@ static void prim_filterSource(EvalState & state, const PosIdx pos, Value * * arg "while evaluating the second argument (the path to filter) passed to 'builtins.filterSource'"); state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filterSource"); - addPath(state, pos, state.computeBaseName(path), path, args[0], ContentAddressMethod::Raw::NixArchive, std::nullopt, v, context); + addPath(state, pos, state.computeBaseName(path, pos), path, args[0], ContentAddressMethod::Raw::NixArchive, std::nullopt, v, context); } static RegisterPrimOp primop_filterSource({ diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index f51108459..e05d52693 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -39,7 +39,7 @@ json printValueAsJSON(EvalState & state, bool strict, case nPath: if (copyToStore) out = state.store->printStorePath( - state.copyPathToStore(context, v.path())); + state.copyPathToStore(context, v.path(), v.determinePos(pos))); else out = v.path().path.abs(); break; diff --git a/src/libutil/include/nix/util/pos-idx.hh b/src/libutil/include/nix/util/pos-idx.hh index c1749ba69..4f305bdd8 100644 --- a/src/libutil/include/nix/util/pos-idx.hh +++ b/src/libutil/include/nix/util/pos-idx.hh @@ -15,12 +15,12 @@ class PosIdx private: uint32_t id; +public: explicit PosIdx(uint32_t id) : id(id) { } -public: PosIdx() : id(0) { @@ -45,6 +45,11 @@ public: { return std::hash{}(id); } + + uint32_t get() const + { + return id; + } }; inline PosIdx noPos = {};