attr path parser: fix bug in not rejecting empty attr paths, add unparser

The following behaviour was previously present and has been fixed:

lix/lix2 » nix eval --expr '{x."" = 2;}' 'x.""'
{ "" = 2; }
lix/lix2 » nix eval --expr '{x."".y = 2;}' 'x."".y'
error: empty attribute name in selection path 'x."".y'

Change-Id: Iad21988f1191c33a3661c72a6b7f01a8b8b3e6eb

lix/libexpr/attr-path.cc

@@ -1,45 +1,83 @@
 #include "lix/libexpr/attr-path.hh"
 #include "lix/libexpr/eval-inline.hh"
+#include <algorithm>
+#include <sstream>
 
 
 namespace nix {
 
 
-Strings parseAttrPath(std::string_view s)
+std::vector<std::string> parseAttrPath(std::string_view const s)
 {
-    Strings res;
+    std::vector<std::string> res;
     std::string cur;
+    bool haveData = false;
     auto i = s.begin();
     while (i != s.end()) {
         if (*i == '.') {
             res.push_back(cur);
+            haveData = false;
             cur.clear();
         } else if (*i == '"') {
+            // If there is a quote there *will* be a named term even if it is empty.
             ++i;
+            haveData = true;
+
             while (1) {
                 if (i == s.end())
                     throw ParseError("missing closing quote in selection path '%1%'", s);
                 if (*i == '"') break;
                 cur.push_back(*i++);
             }
-        } else
+        } else {
             cur.push_back(*i);
+            haveData = true;
+        }
         ++i;
     }
-    if (!cur.empty()) res.push_back(cur);
+    if (haveData) res.push_back(cur);
     return res;
 }
 
 
+std::string unparseAttrPath(std::vector<std::string> const & attrPath)
+{
+    // FIXME(jade): can probably be rewritten with ranges once libc++ has a
+    // fully featured implementation
+    // https://github.com/llvm/llvm-project/pull/65536
+    auto ret = std::ostringstream{};
+    bool first = true;
+
+    for (auto const & part : attrPath) {
+        if (!first) {
+            ret << ".";
+        }
+        first = false;
+
+        bool mustQuote = std::ranges::any_of(part, [](char c) -> bool {
+            return c == '"' || c == '.' || c == ' ';
+        });
+
+        if (mustQuote || part.empty()) {
+            ret << '"' << part << '"';
+        } else {
+            ret << part;
+        }
+    }
+
+    return ret.str();
+}
+
+
 std::pair<Value *, PosIdx> findAlongAttrPath(EvalState & state, const std::string & attrPath,
     Bindings & autoArgs, Value & vIn)
 {
-    Strings tokens = parseAttrPath(attrPath);
+    auto tokens = parseAttrPath(attrPath);
 
     Value * v = &vIn;
     PosIdx pos = noPos;
 
-    for (auto & attr : tokens) {
+    for (auto [attrPathIdx, attr] : enumerate(tokens)) {
 
         /* Is i an index (integer) or a normal attribute name? */
         auto attrIndex = string2Int<unsigned int>(attr);
@@ -54,15 +92,18 @@ std::pair<Value *, PosIdx> findAlongAttrPath(EvalState & state, const std::strin
            according to what is specified in the attrPath. */
 
         if (!attrIndex) {
-
-            if (v->type() != nAttrs)
-                state.ctx.errors.make<TypeError>(
-                    "the expression selected by the selection path '%1%' should be a set but is %2%",
-                    attrPath,
-                    showType(*v)).debugThrow();
             if (attr.empty())
                 throw Error("empty attribute name in selection path '%1%'", attrPath);
 
+            if (v->type() != nAttrs) {
+                auto pathPart = std::vector<std::string>(tokens.begin(), tokens.begin() + attrPathIdx);
+                state.ctx.errors.make<TypeError>(
+                    "the value being indexed in the selection path '%1%' at '%2%' should be a set but is %3%",
+                    attrPath,
+                    unparseAttrPath(pathPart),
+                    showType(*v)).debugThrow();
+            }
+
             Bindings::iterator a = v->attrs->find(state.ctx.symbols.create(attr));
             if (a == v->attrs->end()) {
                 std::set<std::string> attrNames;

lix/libexpr/attr-path.hh

@@ -22,6 +22,18 @@ std::pair<Value *, PosIdx> findAlongAttrPath(
  */
 std::pair<SourcePath, uint32_t> findPackageFilename(EvalState & state, Value & v, std::string what);
 
-Strings parseAttrPath(std::string_view s);
+/**
+ * Parses an attr path (as used in nix-build -A foo.bar.baz) into a list of tokens.
+ *
+ * Such an attr path is a dot-separated sequence of attribute names, which are possibly quoted.
+ * No escaping is performed; attribute names containing double quotes are unrepresentable.
+ */
+std::vector<std::string> parseAttrPath(std::string_view const s);
+
+/**
+ * Converts an attr path from a list of strings into a string once more.
+ * The result returned is an attr path and is *not necessarily valid nix syntax*.
+ */
+std::string unparseAttrPath(std::vector<std::string> const & attrPath);
 
 }

lix/libexpr/eval-cache.cc

@@ -528,7 +528,7 @@ ref<AttrCursor> AttrCursor::getAttr(EvalState & state, const std::string & name)
     return ref(p);
 }
 
-OrSuggestions<ref<AttrCursor>> AttrCursor::findAlongAttrPath(EvalState & state, const Strings & attrPath)
+OrSuggestions<ref<AttrCursor>> AttrCursor::findAlongAttrPath(EvalState & state, const std::vector<std::string> & attrPath)
 {
     auto res = shared_from_this();
     for (auto & attr : attrPath) {

lix/libexpr/eval-cache.hh

@@ -128,7 +128,7 @@ public:
      * Get an attribute along a chain of attrsets. Note that this does
      * not auto-call functors or functions.
      */
-    OrSuggestions<ref<AttrCursor>> findAlongAttrPath(EvalState & state, const Strings & attrPath);
+    OrSuggestions<ref<AttrCursor>> findAlongAttrPath(EvalState & state, const std::vector<std::string> & attrPath);
 
     std::string getString(EvalState & state);
 

lix/libutil/types.hh

@@ -123,6 +123,9 @@ struct MaintainCount
  * A Rust/Python-like enumerate() iterator adapter.
  *
  * Borrowed from http://reedbeta.com/blog/python-like-enumerate-in-cpp17.
+ *
+ * FIXME(jade): remove once P2164R9 is implemented in libc++ and replace with
+ * std::views::enumerate: https://libcxx.llvm.org/Status/Cxx23.html
  */
 template <typename T,
           typename TIter = decltype(std::begin(std::declval<T>())),

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

@@ -8,7 +8,6 @@
 #include "lix/libexpr/nixexpr.hh"
 #include "lix/libexpr/eval.hh"
 #include "lix/libexpr/eval-inline.hh"
-#include "lix/libstore/store-api.hh"
 
 #include "tests/libstore.hh"
 
@@ -20,6 +19,10 @@ namespace nix {
                 initLibExpr();
             }
 
+            EvalState & evalState() {
+                return state;
+            }
+
         protected:
             LibExprTest()
                 : LibStoreTest()

tests/unit/libexpr/attr-path.cc

@@ -0,0 +1,46 @@
+#include "lix/libexpr/attr-path.hh"
+#include "lix/libexpr/attr-set.hh"
+#include "tests/libexpr.hh"
+#include <gtest/gtest.h>
+#include <rapidcheck/gen/Arbitrary.h>
+#include <rapidcheck/gen/Container.h>
+#include <rapidcheck/gen/Predicate.h>
+#include <rapidcheck/gtest.h>
+
+namespace nix {
+
+class AttrPathEval : public LibExprTest
+{
+public:
+    std::pair<Value *, PosIdx> testFindAlongAttrPath(std::string expr, std::string path);
+};
+
+RC_GTEST_PROP(AttrPath, prop_round_trip, ())
+{
+    auto strings = *rc::gen::container<std::vector<std::string>>(
+        rc::gen::container<std::string>(rc::gen::distinctFrom('"'))
+    );
+    auto const unparsed = unparseAttrPath(strings);
+    auto const unparsedReparsed = parseAttrPath(unparsed);
+
+    RC_ASSERT(strings == unparsedReparsed);
+}
+
+std::pair<Value *, PosIdx> AttrPathEval::testFindAlongAttrPath(std::string expr, std::string path)
+{
+    auto v = eval(expr);
+    auto bindings = evalState().ctx.buildBindings(0).finish();
+    return findAlongAttrPath(state, path, *bindings, v);
+}
+
+// n.b. I do not know why we throw for empty attrs but they are apparently
+// disallowed.
+TEST_F(AttrPathEval, emptyAttrsThrows)
+{
+    std::string expr = "{a.\"\".b = 2;}";
+    ASSERT_NO_THROW(testFindAlongAttrPath(expr, "a"));
+    ASSERT_THROW(testFindAlongAttrPath(expr, "a.\"\".b"), Error);
+    ASSERT_THROW(testFindAlongAttrPath(expr, "a.\"\""), Error);
+}
+
+}

tests/unit/meson.build

@@ -193,6 +193,7 @@ liblixexpr_test_support = declare_dependency(
 )
 
 libexpr_tests_sources = files(
+  'libexpr/attr-path.cc',
   'libexpr/derived-path.cc',
   'libexpr/error_traces.cc',
   'libexpr/flakeref.cc',