From d384ecd553aa997270b79ee98d02f7cf7e1849e6 Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Mon, 29 Jan 2024 06:19:23 +0100
Subject: [PATCH 1/6] keep copies of parser inputs that are in-memory only

the parser modifies its inputs, which means that sharing them between
the error context reporting system and the parser itself can confuse the
reporting system. usually this led to early truncation of error context
reports which, while not dangerous, can be quite confusing.
---
 src/libexpr/eval.cc                              | 16 +++++++++++-----
 .../lang/parse-fail-dup-attrs-1.err.exp          |  1 +
 .../lang/parse-fail-dup-attrs-2.err.exp          |  1 +
 .../lang/parse-fail-dup-attrs-3.err.exp          |  1 +
 .../lang/parse-fail-dup-attrs-4.err.exp          |  1 +
 .../lang/parse-fail-dup-attrs-7.err.exp          |  1 +
 .../lang/parse-fail-undef-var-2.err.exp          |  3 ++-
 tests/functional/lang/parse-fail-utf8.err.exp    |  3 ++-
 8 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 722ff6908..3d22723b3 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -2777,9 +2777,12 @@ Expr * EvalState::parseExprFromFile(const SourcePath & path, std::shared_ptr<Sta
 
 Expr * EvalState::parseExprFromString(std::string s_, const SourcePath & basePath, std::shared_ptr<StaticEnv> & staticEnv)
 {
-    auto s = make_ref<std::string>(std::move(s_));
-    s->append("\0\0", 2);
-    return parse(s->data(), s->size(), Pos::String{.source = s}, basePath, staticEnv);
+    // NOTE this method (and parseStdin) must take care to *fully copy* their input
+    // into their respective Pos::Origin until the parser stops overwriting its input
+    // data.
+    auto s = make_ref<std::string>(s_);
+    s_.append("\0\0", 2);
+    return parse(s_.data(), s_.size(), Pos::String{.source = s}, basePath, staticEnv);
 }
 
 
@@ -2791,12 +2794,15 @@ Expr * EvalState::parseExprFromString(std::string s, const SourcePath & basePath
 
 Expr * EvalState::parseStdin()
 {
+    // NOTE this method (and parseExprFromString) must take care to *fully copy* their
+    // input into their respective Pos::Origin until the parser stops overwriting its
+    // input data.
     //Activity act(*logger, lvlTalkative, "parsing standard input");
     auto buffer = drainFD(0);
     // drainFD should have left some extra space for terminators
     buffer.append("\0\0", 2);
-    auto s = make_ref<std::string>(std::move(buffer));
-    return parse(s->data(), s->size(), Pos::Stdin{.source = s}, rootPath("."), staticBaseEnv);
+    auto s = make_ref<std::string>(buffer);
+    return parse(buffer.data(), buffer.size(), Pos::Stdin{.source = s}, rootPath("."), staticBaseEnv);
 }
 
 
diff --git a/tests/functional/lang/parse-fail-dup-attrs-1.err.exp b/tests/functional/lang/parse-fail-dup-attrs-1.err.exp
index 6c3a3510c..ffb5198c1 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-1.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-1.err.exp
@@ -3,3 +3,4 @@ error: attribute 'x' already defined at «stdin»:1:3
             2|   y = 456;
             3|   x = 789;
              |   ^
+            4| }
diff --git a/tests/functional/lang/parse-fail-dup-attrs-2.err.exp b/tests/functional/lang/parse-fail-dup-attrs-2.err.exp
index fecdece20..4607a5d59 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-2.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-2.err.exp
@@ -3,3 +3,4 @@ error: attribute 'x' already defined at «stdin»:9:5
             9|     x = 789;
            10|     inherit (as) x;
              |                 ^
+           11|   };
diff --git a/tests/functional/lang/parse-fail-dup-attrs-3.err.exp b/tests/functional/lang/parse-fail-dup-attrs-3.err.exp
index fecdece20..4607a5d59 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-3.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-3.err.exp
@@ -3,3 +3,4 @@ error: attribute 'x' already defined at «stdin»:9:5
             9|     x = 789;
            10|     inherit (as) x;
              |                 ^
+           11|   };
diff --git a/tests/functional/lang/parse-fail-dup-attrs-4.err.exp b/tests/functional/lang/parse-fail-dup-attrs-4.err.exp
index f85ffea51..c98a8f8d0 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-4.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-4.err.exp
@@ -3,3 +3,4 @@ error: attribute 'services.ssh.port' already defined at «stdin»:2:3
             2|   services.ssh.port = 22;
             3|   services.ssh.port = 23;
              |   ^
+            4| }
diff --git a/tests/functional/lang/parse-fail-dup-attrs-7.err.exp b/tests/functional/lang/parse-fail-dup-attrs-7.err.exp
index 98cea9dae..2daddf380 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-7.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-7.err.exp
@@ -3,3 +3,4 @@ error: attribute 'x' already defined at «stdin»:6:12
             6|     inherit x;
             7|     inherit x;
              |            ^
+            8|   };
diff --git a/tests/functional/lang/parse-fail-undef-var-2.err.exp b/tests/functional/lang/parse-fail-undef-var-2.err.exp
index a58d8dca4..393c454dd 100644
--- a/tests/functional/lang/parse-fail-undef-var-2.err.exp
+++ b/tests/functional/lang/parse-fail-undef-var-2.err.exp
@@ -1,5 +1,6 @@
 error: syntax error, unexpected ':', expecting '}'
        at «stdin»:3:13:
             2|
-            3|   f = {x, y :
+            3|   f = {x, y : ["baz" "bar" z "bat"]}: x + y;
              |             ^
+            4|
diff --git a/tests/functional/lang/parse-fail-utf8.err.exp b/tests/functional/lang/parse-fail-utf8.err.exp
index e83abdb9e..1c83f6eb3 100644
--- a/tests/functional/lang/parse-fail-utf8.err.exp
+++ b/tests/functional/lang/parse-fail-utf8.err.exp
@@ -1,4 +1,5 @@
 error: syntax error, unexpected invalid token, expecting end of file
        at «stdin»:1:5:
-            1| 123 Ã
+            1| 123 é 4
              |     ^
+            2|

From 4147ecfb1c51f3fe3b4adcbd4e753fd487dab645 Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Mon, 29 Jan 2024 06:19:23 +0100
Subject: [PATCH 2/6] normalize formal order on ExprLambda::show

we already normalize attr order to lexicographic, doing the same for
formals makes sense. doubly so because the order of formals would
otherwise depend on the context of the expression, which is not quite as
useful as one might expect.
---
 doc/manual/rl-next/formal-order.md              | 7 +++++++
 src/libexpr/nixexpr.cc                          | 5 ++++-
 tests/functional/lang/parse-okay-subversion.exp | 2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)
 create mode 100644 doc/manual/rl-next/formal-order.md

diff --git a/doc/manual/rl-next/formal-order.md b/doc/manual/rl-next/formal-order.md
new file mode 100644
index 000000000..12628e318
--- /dev/null
+++ b/doc/manual/rl-next/formal-order.md
@@ -0,0 +1,7 @@
+---
+synopsis: consistent order of lambda formals in printed expressions
+prs: 9874
+---
+
+Always print lambda formals in lexicographic order rather than the internal, creation-time based symbol order.
+This makes printed formals independent of the context they appear in.
diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc
index 4b805d710..9a8b9616b 100644
--- a/src/libexpr/nixexpr.cc
+++ b/src/libexpr/nixexpr.cc
@@ -149,7 +149,10 @@ void ExprLambda::show(const SymbolTable & symbols, std::ostream & str) const
     if (hasFormals()) {
         str << "{ ";
         bool first = true;
-        for (auto & i : formals->formals) {
+        // the natural Symbol ordering is by creation time, which can lead to the
+        // same expression being printed in two different ways depending on its
+        // context. always use lexicographic ordering to avoid this.
+        for (auto & i : formals->lexicographicOrder(symbols)) {
             if (first) first = false; else str << ", ";
             str << symbols[i.name];
             if (i.def) {
diff --git a/tests/functional/lang/parse-okay-subversion.exp b/tests/functional/lang/parse-okay-subversion.exp
index 2303932c4..32fbba3c5 100644
--- a/tests/functional/lang/parse-okay-subversion.exp
+++ b/tests/functional/lang/parse-okay-subversion.exp
@@ -1 +1 @@
-({ fetchurl, localServer ? false, httpServer ? false, sslSupport ? false, pythonBindings ? false, javaSwigBindings ? false, javahlBindings ? false, stdenv, openssl ? null, httpd ? null, db4 ? null, expat, swig ? null, j2sdk ? null }: assert (expat != null); assert (localServer -> (db4 != null)); assert (httpServer -> ((httpd != null) && ((httpd).expat == expat))); assert (sslSupport -> ((openssl != null) && (httpServer -> ((httpd).openssl == openssl)))); assert (pythonBindings -> ((swig != null) && (swig).pythonSupport)); assert (javaSwigBindings -> ((swig != null) && (swig).javaSupport)); assert (javahlBindings -> (j2sdk != null)); ((stdenv).mkDerivation { inherit expat httpServer javaSwigBindings javahlBindings localServer pythonBindings sslSupport; builder = /foo/bar; db4 = (if localServer then db4 else null); httpd = (if httpServer then httpd else null); j2sdk = (if javaSwigBindings then (swig).j2sdk else (if javahlBindings then j2sdk else null)); name = "subversion-1.1.1"; openssl = (if sslSupport then openssl else null); patches = (if javahlBindings then [ (/javahl.patch) ] else [ ]); python = (if pythonBindings then (swig).python else null); src = (fetchurl { md5 = "a180c3fe91680389c210c99def54d9e0"; url = "http://subversion.tigris.org/tarballs/subversion-1.1.1.tar.bz2"; }); swig = (if (pythonBindings || javaSwigBindings) then swig else null); }))
+({ db4 ? null, expat, fetchurl, httpServer ? false, httpd ? null, j2sdk ? null, javaSwigBindings ? false, javahlBindings ? false, localServer ? false, openssl ? null, pythonBindings ? false, sslSupport ? false, stdenv, swig ? null }: assert (expat != null); assert (localServer -> (db4 != null)); assert (httpServer -> ((httpd != null) && ((httpd).expat == expat))); assert (sslSupport -> ((openssl != null) && (httpServer -> ((httpd).openssl == openssl)))); assert (pythonBindings -> ((swig != null) && (swig).pythonSupport)); assert (javaSwigBindings -> ((swig != null) && (swig).javaSupport)); assert (javahlBindings -> (j2sdk != null)); ((stdenv).mkDerivation { inherit expat httpServer javaSwigBindings javahlBindings localServer pythonBindings sslSupport; builder = /foo/bar; db4 = (if localServer then db4 else null); httpd = (if httpServer then httpd else null); j2sdk = (if javaSwigBindings then (swig).j2sdk else (if javahlBindings then j2sdk else null)); name = "subversion-1.1.1"; openssl = (if sslSupport then openssl else null); patches = (if javahlBindings then [ (/javahl.patch) ] else [ ]); python = (if pythonBindings then (swig).python else null); src = (fetchurl { md5 = "a180c3fe91680389c210c99def54d9e0"; url = "http://subversion.tigris.org/tarballs/subversion-1.1.1.tar.bz2"; }); swig = (if (pythonBindings || javaSwigBindings) then swig else null); }))

From 1edd6fada53553b89847ac3981ac28025857ca02 Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Mon, 29 Jan 2024 06:19:23 +0100
Subject: [PATCH 3/6] report inherit attr errors at the duplicate name

previously we reported the error at the beginning of the binding
block (for plain inherits) or the beginning of the attr list (for
inherit-from), effectively hiding where exactly the error happened.

this also carries over to runtime positions of attributes in sets as
reported by unsafeGetAttrPos. we're not worried about this changing
observable eval behavior because it *is* marked unsafe, and the new
behavior is much more useful.
---
 doc/manual/rl-next/inherit-error-positions.md |  6 +++++
 src/libexpr/parser.y                          | 25 ++++++++++---------
 .../lang/eval-okay-inherit-attr-pos.exp       |  1 +
 .../lang/eval-okay-inherit-attr-pos.nix       | 12 +++++++++
 .../lang/parse-fail-dup-attrs-2.err.exp       |  4 +--
 .../lang/parse-fail-dup-attrs-3.err.exp       |  4 +--
 .../lang/parse-fail-dup-attrs-7.err.exp       |  6 ++---
 .../parse-fail-regression-20060610.err.exp    |  6 ++---
 8 files changed, 42 insertions(+), 22 deletions(-)
 create mode 100644 doc/manual/rl-next/inherit-error-positions.md
 create mode 100644 tests/functional/lang/eval-okay-inherit-attr-pos.exp
 create mode 100644 tests/functional/lang/eval-okay-inherit-attr-pos.nix

diff --git a/doc/manual/rl-next/inherit-error-positions.md b/doc/manual/rl-next/inherit-error-positions.md
new file mode 100644
index 000000000..643080e9e
--- /dev/null
+++ b/doc/manual/rl-next/inherit-error-positions.md
@@ -0,0 +1,6 @@
+---
+synopsis: fix duplicate attribute error positions for `inherit`
+prs: 9874
+---
+
+When an inherit caused a duplicate attribute error the position of the error was not reported correctly, placing the error with the inherit itself or at the start of the bindings block instead of the offending attribute name.
diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index b0aee7b41..9a543d636 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -87,6 +87,7 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char *
   nix::StringToken uri;
   nix::StringToken str;
   std::vector<nix::AttrName> * attrNames;
+  std::vector<std::pair<nix::AttrName, nix::PosIdx>> * inheritAttrs;
   std::vector<std::pair<nix::PosIdx, nix::Expr *>> * string_parts;
   std::vector<std::pair<nix::PosIdx, std::variant<nix::Expr *, nix::StringToken>>> * ind_string_parts;
 }
@@ -97,7 +98,8 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char *
 %type <attrs> binds
 %type <formals> formals
 %type <formal> formal
-%type <attrNames> attrs attrpath
+%type <attrNames> attrpath
+%type <inheritAttrs> attrs
 %type <string_parts> string_parts_interpolated
 %type <ind_string_parts> ind_string_parts
 %type <e> path_start string_parts string_attr
@@ -309,13 +311,12 @@ binds
   : binds attrpath '=' expr ';' { $$ = $1; state->addAttr($$, std::move(*$2), $4, state->at(@2)); delete $2; }
   | binds INHERIT attrs ';'
     { $$ = $1;
-      for (auto & i : *$3) {
+      for (auto & [i, iPos] : *$3) {
           if ($$->attrs.find(i.symbol) != $$->attrs.end())
-              state->dupAttr(i.symbol, state->at(@3), $$->attrs[i.symbol].pos);
-          auto pos = state->at(@3);
+              state->dupAttr(i.symbol, iPos, $$->attrs[i.symbol].pos);
           $$->attrs.emplace(
               i.symbol,
-              ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, ExprAttrs::AttrDef::Kind::Inherited));
+              ExprAttrs::AttrDef(new ExprVar(iPos, i.symbol), iPos, ExprAttrs::AttrDef::Kind::Inherited));
       }
       delete $3;
     }
@@ -325,14 +326,14 @@ binds
           $$->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
       $$->inheritFromExprs->push_back($4);
       auto from = new nix::ExprInheritFrom(state->at(@4), $$->inheritFromExprs->size() - 1);
-      for (auto & i : *$6) {
+      for (auto & [i, iPos] : *$6) {
           if ($$->attrs.find(i.symbol) != $$->attrs.end())
-              state->dupAttr(i.symbol, state->at(@6), $$->attrs[i.symbol].pos);
+              state->dupAttr(i.symbol, iPos, $$->attrs[i.symbol].pos);
           $$->attrs.emplace(
               i.symbol,
               ExprAttrs::AttrDef(
-                  new ExprSelect(CUR_POS, from, i.symbol),
-                  state->at(@6),
+                  new ExprSelect(iPos, from, i.symbol),
+                  iPos,
                   ExprAttrs::AttrDef::Kind::InheritedFrom));
       }
       delete $6;
@@ -341,12 +342,12 @@ binds
   ;
 
 attrs
-  : attrs attr { $$ = $1; $1->push_back(AttrName(state->symbols.create($2))); }
+  : attrs attr { $$ = $1; $1->emplace_back(AttrName(state->symbols.create($2)), state->at(@2)); }
   | attrs string_attr
     { $$ = $1;
       ExprString * str = dynamic_cast<ExprString *>($2);
       if (str) {
-          $$->push_back(AttrName(state->symbols.create(str->s)));
+          $$->emplace_back(AttrName(state->symbols.create(str->s)), state->at(@2));
           delete str;
       } else
           throw ParseError({
@@ -354,7 +355,7 @@ attrs
               .pos = state->positions[state->at(@2)]
           });
     }
-  | { $$ = new AttrPath; }
+  | { $$ = new std::vector<std::pair<AttrName, PosIdx>>; }
   ;
 
 attrpath
diff --git a/tests/functional/lang/eval-okay-inherit-attr-pos.exp b/tests/functional/lang/eval-okay-inherit-attr-pos.exp
new file mode 100644
index 000000000..e87d037c6
--- /dev/null
+++ b/tests/functional/lang/eval-okay-inherit-attr-pos.exp
@@ -0,0 +1 @@
+[ { column = 17; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 4; } { column = 19; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 4; } { column = 21; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 5; } { column = 23; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 5; } ]
diff --git a/tests/functional/lang/eval-okay-inherit-attr-pos.nix b/tests/functional/lang/eval-okay-inherit-attr-pos.nix
new file mode 100644
index 000000000..017ab1d36
--- /dev/null
+++ b/tests/functional/lang/eval-okay-inherit-attr-pos.nix
@@ -0,0 +1,12 @@
+let
+  d = 0;
+  x = 1;
+  y = { inherit d x; };
+  z = { inherit (y) d x; };
+in
+  [
+    (builtins.unsafeGetAttrPos "d" y)
+    (builtins.unsafeGetAttrPos "x" y)
+    (builtins.unsafeGetAttrPos "d" z)
+    (builtins.unsafeGetAttrPos "x" z)
+  ]
diff --git a/tests/functional/lang/parse-fail-dup-attrs-2.err.exp b/tests/functional/lang/parse-fail-dup-attrs-2.err.exp
index 4607a5d59..3105e60de 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-2.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-2.err.exp
@@ -1,6 +1,6 @@
 error: attribute 'x' already defined at «stdin»:9:5
-       at «stdin»:10:17:
+       at «stdin»:10:18:
             9|     x = 789;
            10|     inherit (as) x;
-             |                 ^
+             |                  ^
            11|   };
diff --git a/tests/functional/lang/parse-fail-dup-attrs-3.err.exp b/tests/functional/lang/parse-fail-dup-attrs-3.err.exp
index 4607a5d59..3105e60de 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-3.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-3.err.exp
@@ -1,6 +1,6 @@
 error: attribute 'x' already defined at «stdin»:9:5
-       at «stdin»:10:17:
+       at «stdin»:10:18:
             9|     x = 789;
            10|     inherit (as) x;
-             |                 ^
+             |                  ^
            11|   };
diff --git a/tests/functional/lang/parse-fail-dup-attrs-7.err.exp b/tests/functional/lang/parse-fail-dup-attrs-7.err.exp
index 2daddf380..4e0a48eff 100644
--- a/tests/functional/lang/parse-fail-dup-attrs-7.err.exp
+++ b/tests/functional/lang/parse-fail-dup-attrs-7.err.exp
@@ -1,6 +1,6 @@
-error: attribute 'x' already defined at «stdin»:6:12
-       at «stdin»:7:12:
+error: attribute 'x' already defined at «stdin»:6:13
+       at «stdin»:7:13:
             6|     inherit x;
             7|     inherit x;
-             |            ^
+             |             ^
             8|   };
diff --git a/tests/functional/lang/parse-fail-regression-20060610.err.exp b/tests/functional/lang/parse-fail-regression-20060610.err.exp
index d8875a6a5..6ae7c01bf 100644
--- a/tests/functional/lang/parse-fail-regression-20060610.err.exp
+++ b/tests/functional/lang/parse-fail-regression-20060610.err.exp
@@ -1,6 +1,6 @@
 error: undefined variable 'gcc'
-       at «stdin»:8:12:
-            7|
+       at «stdin»:9:13:
             8|   body = ({
-             |            ^
             9|     inherit gcc;
+             |             ^
+           10|   }).gcc;

From 2be6b143289e5479cc4a2667bb84e879116c2447 Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Mon, 29 Jan 2024 06:19:23 +0100
Subject: [PATCH 4/6] match line endings used by parser and error reports

the parser treats a plain \r as a newline, error reports do not. this
can lead to interesting divergences if anything makes use of this
feature, with error reports pointing to wrong locations in the input (or
even outside the input altogether).
---
 src/libutil/position.cc                       | 55 +++++++++++--------
 src/libutil/position.hh                       | 42 ++++++++++++++
 tests/functional/lang/eval-fail-eol-1.err.exp |  6 ++
 tests/functional/lang/eval-fail-eol-1.nix     |  3 +
 tests/functional/lang/eval-fail-eol-2.err.exp |  6 ++
 tests/functional/lang/eval-fail-eol-2.nix     |  2 +
 tests/functional/lang/eval-fail-eol-3.err.exp |  6 ++
 tests/functional/lang/eval-fail-eol-3.nix     |  3 +
 8 files changed, 99 insertions(+), 24 deletions(-)
 create mode 100644 tests/functional/lang/eval-fail-eol-1.err.exp
 create mode 100644 tests/functional/lang/eval-fail-eol-1.nix
 create mode 100644 tests/functional/lang/eval-fail-eol-2.err.exp
 create mode 100644 tests/functional/lang/eval-fail-eol-2.nix
 create mode 100644 tests/functional/lang/eval-fail-eol-3.err.exp
 create mode 100644 tests/functional/lang/eval-fail-eol-3.nix

diff --git a/src/libutil/position.cc b/src/libutil/position.cc
index b39a5a1d4..724e560b7 100644
--- a/src/libutil/position.cc
+++ b/src/libutil/position.cc
@@ -29,32 +29,17 @@ std::optional<LinesOfCode> Pos::getCodeLines() const
         return std::nullopt;
 
     if (auto source = getSource()) {
-
-        std::istringstream iss(*source);
-        // count the newlines.
-        int count = 0;
-        std::string curLine;
-        int pl = line - 1;
-
+        LinesIterator lines(*source), end;
         LinesOfCode loc;
 
-        do {
-            std::getline(iss, curLine);
-            ++count;
-            if (count < pl)
-                ;
-            else if (count == pl) {
-                loc.prevLineOfCode = curLine;
-            } else if (count == pl + 1) {
-                loc.errLineOfCode = curLine;
-            } else if (count == pl + 2) {
-                loc.nextLineOfCode = curLine;
-                break;
-            }
-
-            if (!iss.good())
-                break;
-        } while (true);
+        if (line > 1)
+            std::advance(lines, line - 2);
+        if (lines != end && line > 1)
+            loc.prevLineOfCode = *lines++;
+        if (lines != end)
+            loc.errLineOfCode = *lines++;
+        if (lines != end)
+            loc.nextLineOfCode = *lines++;
 
         return loc;
     }
@@ -109,4 +94,26 @@ std::ostream & operator<<(std::ostream & str, const Pos & pos)
     return str;
 }
 
+void Pos::LinesIterator::bump(bool atFirst)
+{
+    if (!atFirst) {
+        pastEnd = input.empty();
+        if (!input.empty() && input[0] == '\r')
+            input.remove_prefix(1);
+        if (!input.empty() && input[0] == '\n')
+            input.remove_prefix(1);
+    }
+
+    // nix line endings are not only \n as eg std::getline assumes, but also
+    // \r\n **and \r alone**. not treating them all the same causes error
+    // reports to not match with line numbers as the parser expects them.
+    auto eol = input.find_first_of("\r\n");
+
+    if (eol > input.size())
+        eol = input.size();
+
+    curLine = input.substr(0, eol);
+    input.remove_prefix(eol);
+}
+
 }
diff --git a/src/libutil/position.hh b/src/libutil/position.hh
index a184997ed..9bdf3b4b5 100644
--- a/src/libutil/position.hh
+++ b/src/libutil/position.hh
@@ -67,6 +67,48 @@ struct Pos
     bool operator==(const Pos & rhs) const = default;
     bool operator!=(const Pos & rhs) const = default;
     bool operator<(const Pos & rhs) const;
+
+    struct LinesIterator {
+        using difference_type = size_t;
+        using value_type = std::string_view;
+        using reference = const std::string_view &;
+        using pointer = const std::string_view *;
+        using iterator_category = std::input_iterator_tag;
+
+        LinesIterator(): pastEnd(true) {}
+        explicit LinesIterator(std::string_view input): input(input), pastEnd(input.empty()) {
+            if (!pastEnd)
+                bump(true);
+        }
+
+        LinesIterator & operator++() {
+            bump(false);
+            return *this;
+        }
+        LinesIterator operator++(int) {
+            auto result = *this;
+            ++*this;
+            return result;
+        }
+
+        reference operator*() const { return curLine; }
+        pointer operator->() const { return &curLine; }
+
+        bool operator!=(const LinesIterator & other) const {
+            return !(*this == other);
+        }
+        bool operator==(const LinesIterator & other) const {
+            return (pastEnd && other.pastEnd)
+                || (std::forward_as_tuple(input.size(), input.data())
+                    == std::forward_as_tuple(other.input.size(), other.input.data()));
+        }
+
+    private:
+        std::string_view input, curLine;
+        bool pastEnd = false;
+
+        void bump(bool atFirst);
+    };
 };
 
 std::ostream & operator<<(std::ostream & str, const Pos & pos);
diff --git a/tests/functional/lang/eval-fail-eol-1.err.exp b/tests/functional/lang/eval-fail-eol-1.err.exp
new file mode 100644
index 000000000..3f5a5c22c
--- /dev/null
+++ b/tests/functional/lang/eval-fail-eol-1.err.exp
@@ -0,0 +1,6 @@
+error: undefined variable 'invalid'
+       at /pwd/lang/eval-fail-eol-1.nix:2:1:
+            1| # foo
+            2| invalid
+             | ^
+            3| # bar
diff --git a/tests/functional/lang/eval-fail-eol-1.nix b/tests/functional/lang/eval-fail-eol-1.nix
new file mode 100644
index 000000000..476223919
--- /dev/null
+++ b/tests/functional/lang/eval-fail-eol-1.nix
@@ -0,0 +1,3 @@
+# foo
+invalid
+# bar
diff --git a/tests/functional/lang/eval-fail-eol-2.err.exp b/tests/functional/lang/eval-fail-eol-2.err.exp
new file mode 100644
index 000000000..ff13e2d55
--- /dev/null
+++ b/tests/functional/lang/eval-fail-eol-2.err.exp
@@ -0,0 +1,6 @@
+error: undefined variable 'invalid'
+       at /pwd/lang/eval-fail-eol-2.nix:2:1:
+            1| # foo
+            2| invalid
+             | ^
+            3| # bar
diff --git a/tests/functional/lang/eval-fail-eol-2.nix b/tests/functional/lang/eval-fail-eol-2.nix
new file mode 100644
index 000000000..0cf92a425
--- /dev/null
+++ b/tests/functional/lang/eval-fail-eol-2.nix
@@ -0,0 +1,2 @@
+# foo
invalid
+# bar
diff --git a/tests/functional/lang/eval-fail-eol-3.err.exp b/tests/functional/lang/eval-fail-eol-3.err.exp
new file mode 100644
index 000000000..ada3c5ecd
--- /dev/null
+++ b/tests/functional/lang/eval-fail-eol-3.err.exp
@@ -0,0 +1,6 @@
+error: undefined variable 'invalid'
+       at /pwd/lang/eval-fail-eol-3.nix:2:1:
+            1| # foo
+            2| invalid
+             | ^
+            3| # bar
diff --git a/tests/functional/lang/eval-fail-eol-3.nix b/tests/functional/lang/eval-fail-eol-3.nix
new file mode 100644
index 000000000..33422452d
--- /dev/null
+++ b/tests/functional/lang/eval-fail-eol-3.nix
@@ -0,0 +1,3 @@
+# foo
+invalid
+# bar

From 855fd5a1bb781e4f722c1d757ba43e866d370132 Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Mon, 29 Jan 2024 06:19:23 +0100
Subject: [PATCH 5/6] diagnose "unexpected EOF" at EOF

this needs a string comparison because there seems to be no other way to
get that information out of bison. usually the location info is going to
be correct (pointing at a bad token), but since EOF isn't a token as
such it'll be wrong in that this case.

this hasn't shown up much so far because a single line ending *is* a
token, so any file formatted in the usual manner (ie, ending in a line
ending) would have its EOF position reported correctly.
---
 src/libexpr/parser.y                                   | 4 ++++
 tests/functional/lang/parse-fail-eof-in-string.err.exp | 4 ++--
 tests/functional/lang/parse-fail-eof-pos.err.exp       | 5 +++++
 tests/functional/lang/parse-fail-eof-pos.nix           | 2 ++
 4 files changed, 13 insertions(+), 2 deletions(-)
 create mode 100644 tests/functional/lang/parse-fail-eof-pos.err.exp
 create mode 100644 tests/functional/lang/parse-fail-eof-pos.nix

diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index 9a543d636..59f088d53 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -64,6 +64,10 @@ using namespace nix;
 
 void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char * error)
 {
+    if (std::string_view(error).starts_with("syntax error, unexpected end of file")) {
+        loc->first_column = loc->last_column;
+        loc->first_line = loc->last_line;
+    }
     throw ParseError({
         .msg = HintFmt(error),
         .pos = state->positions[state->at(*loc)]
diff --git a/tests/functional/lang/parse-fail-eof-in-string.err.exp b/tests/functional/lang/parse-fail-eof-in-string.err.exp
index b28d35950..17f34b62d 100644
--- a/tests/functional/lang/parse-fail-eof-in-string.err.exp
+++ b/tests/functional/lang/parse-fail-eof-in-string.err.exp
@@ -1,5 +1,5 @@
 error: syntax error, unexpected end of file, expecting '"'
-       at «stdin»:3:5:
+       at «stdin»:3:6:
             2| # Note that this file must not end with a newline.
             3| a 1"$
-             |     ^
+             |      ^
diff --git a/tests/functional/lang/parse-fail-eof-pos.err.exp b/tests/functional/lang/parse-fail-eof-pos.err.exp
new file mode 100644
index 000000000..ef9ca381c
--- /dev/null
+++ b/tests/functional/lang/parse-fail-eof-pos.err.exp
@@ -0,0 +1,5 @@
+error: syntax error, unexpected end of file
+       at «stdin»:3:1:
+            2| # no content
+            3|
+             | ^
diff --git a/tests/functional/lang/parse-fail-eof-pos.nix b/tests/functional/lang/parse-fail-eof-pos.nix
new file mode 100644
index 000000000..bd66a2c98
--- /dev/null
+++ b/tests/functional/lang/parse-fail-eof-pos.nix
@@ -0,0 +1,2 @@
+(
+# no content

From 5d9fdab3de0ee17c71369ad05806b9ea06dfceda Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Mon, 29 Jan 2024 06:19:23 +0100
Subject: [PATCH 6/6] use byte indexed locations for PosIdx

we now keep not a table of all positions, but a table of all origins and
their sizes. position indices are now direct pointers into the virtual
concatenation of all parsed contents. this slightly reduces memory usage
and time spent in the parser, at the cost of not being able to report
positions if the total input size exceeds 4GiB. this limit is not unique
to nix though, rustc and clang also limit their input to 4GiB (although
at least clang refuses to process inputs that are larger, we will not).

this new 4GiB limit probably will not cause any problems for quite a
while, all of nixpkgs together is less than 100MiB in size and already
needs over 700MiB of memory and multiple seconds just to parse. 4GiB
worth of input will easily take multiple minutes and over 30GiB of
memory without even evaluating anything. if problems *do* arise we can
probably recover the old table-based system by adding some tracking to
Pos::Origin (or increasing the size of PosIdx outright), but for time
being this looks like more complexity than it's worth.

since we now need to read the entire input again to determine the
line/column of a position we'll make unsafeGetAttrPos slightly lazy:
mostly the set it returns is only used to determine the file of origin
of an attribute, not its exact location. the thunks do not add
measurable runtime overhead.

notably this change is necessary to allow changing the parser since
apparently nothing supports nix's very idiosyncratic line ending choice
of "anything goes", making it very hard to calculate line/column
positions in the parser (while byte offsets are very easy).
---
 src/libexpr/eval.cc               |  7 +--
 src/libexpr/flake/flake.cc        |  3 +-
 src/libexpr/lexer.l               | 23 +-------
 src/libexpr/nixexpr.cc            | 33 +++++++++++
 src/libexpr/nixexpr.hh            |  1 -
 src/libexpr/parser-state.hh       |  9 +--
 src/libexpr/parser.y              |  2 +-
 src/libexpr/pos-idx.hh            |  1 +
 src/libexpr/pos-table.hh          | 92 ++++++++++++++++---------------
 src/libexpr/primops.cc            | 48 ++++++++++++++++
 src/libexpr/primops.hh            |  2 +
 tests/unit/libexpr/primops.cc     |  6 +-
 tests/unit/libexpr/value/print.cc |  8 +--
 13 files changed, 150 insertions(+), 85 deletions(-)

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 3d22723b3..bbccfcd29 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -949,12 +949,11 @@ void EvalState::mkThunk_(Value & v, Expr * expr)
 
 void EvalState::mkPos(Value & v, PosIdx p)
 {
-    auto pos = positions[p];
-    if (auto path = std::get_if<SourcePath>(&pos.origin)) {
+    auto origin = positions.originOf(p);
+    if (auto path = std::get_if<SourcePath>(&origin)) {
         auto attrs = buildBindings(3);
         attrs.alloc(sFile).mkString(path->path.abs());
-        attrs.alloc(sLine).mkInt(pos.line);
-        attrs.alloc(sColumn).mkInt(pos.column);
+        makePositionThunks(*this, p, attrs.alloc(sLine), attrs.alloc(sColumn));
         v.mkAttrs(attrs);
     } else
         v.mkNull();
diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc
index fd9341504..dd8924859 100644
--- a/src/libexpr/flake/flake.cc
+++ b/src/libexpr/flake/flake.cc
@@ -212,11 +212,10 @@ static Flake readFlake(
 {
     auto flakePath = rootDir / CanonPath(resolvedRef.subdir) / "flake.nix";
 
+    // NOTE evalFile forces vInfo to be an attrset because mustBeTrivial is true.
     Value vInfo;
     state.evalFile(flakePath, vInfo, true);
 
-    expectType(state, nAttrs, vInfo, state.positions.add(Pos::Origin(rootDir), 1, 1));
-
     Flake flake {
         .originalRef = originalRef,
         .resolvedRef = resolvedRef,
diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l
index 5b26d6927..ee2b6b807 100644
--- a/src/libexpr/lexer.l
+++ b/src/libexpr/lexer.l
@@ -33,33 +33,16 @@ namespace nix {
 
 static void initLoc(YYLTYPE * loc)
 {
-    loc->first_line = loc->last_line = 1;
-    loc->first_column = loc->last_column = 1;
+    loc->first_line = loc->last_line = 0;
+    loc->first_column = loc->last_column = 0;
 }
 
 static void adjustLoc(YYLTYPE * loc, const char * s, size_t len)
 {
     loc->stash();
 
-    loc->first_line = loc->last_line;
     loc->first_column = loc->last_column;
-
-    for (size_t i = 0; i < len; i++) {
-       switch (*s++) {
-       case '\r':
-           if (*s == '\n') { /* cr/lf */
-               i++;
-               s++;
-           }
-           /* fall through */
-       case '\n':
-           ++loc->last_line;
-           loc->last_column = 1;
-           break;
-       default:
-           ++loc->last_column;
-       }
-    }
+    loc->last_column += len;
 }
 
 
diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc
index 9a8b9616b..5bdc466eb 100644
--- a/src/libexpr/nixexpr.cc
+++ b/src/libexpr/nixexpr.cc
@@ -583,6 +583,39 @@ std::string ExprLambda::showNamePos(const EvalState & state) const
 
 
 
+/* Position table. */
+
+Pos PosTable::operator[](PosIdx p) const
+{
+    auto origin = resolve(p);
+    if (!origin)
+        return {};
+
+    const auto offset = origin->offsetOf(p);
+
+    Pos result{0, 0, origin->origin};
+    auto lines = this->lines.lock();
+    auto linesForInput = (*lines)[origin->offset];
+
+    if (linesForInput.empty()) {
+        auto source = result.getSource().value_or("");
+        const char * begin = source.data();
+        for (Pos::LinesIterator it(source), end; it != end; it++)
+            linesForInput.push_back(it->data() - begin);
+        if (linesForInput.empty())
+            linesForInput.push_back(0);
+    }
+    // as above: the first line starts at byte 0 and is always present
+    auto lineStartOffset = std::prev(
+        std::upper_bound(linesForInput.begin(), linesForInput.end(), offset));
+
+    result.line = 1 + (lineStartOffset - linesForInput.begin());
+    result.column = 1 + (offset - *lineStartOffset);
+    return result;
+}
+
+
+
 /* Symbol table. */
 
 size_t SymbolTable::totalSize() const
diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
index 94356759b..e3cae8385 100644
--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -7,7 +7,6 @@
 #include "value.hh"
 #include "symbol-table.hh"
 #include "error.hh"
-#include "chunked-vector.hh"
 #include "position.hh"
 #include "eval-error.hh"
 #include "pos-idx.hh"
diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh
index 34aef661f..024e79c43 100644
--- a/src/libexpr/parser-state.hh
+++ b/src/libexpr/parser-state.hh
@@ -24,20 +24,15 @@ struct ParserLocation
     int last_line, last_column;
 
     // backup to recover from yyless(0)
-    int stashed_first_line, stashed_first_column;
-    int stashed_last_line, stashed_last_column;
+    int stashed_first_column, stashed_last_column;
 
     void stash() {
-        stashed_first_line = first_line;
         stashed_first_column = first_column;
-        stashed_last_line = last_line;
         stashed_last_column = last_column;
     }
 
     void unstash() {
-        first_line = stashed_first_line;
         first_column = stashed_first_column;
-        last_line = stashed_last_line;
         last_column = stashed_last_column;
     }
 };
@@ -276,7 +271,7 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos,
 
 inline PosIdx ParserState::at(const ParserLocation & loc)
 {
-    return positions.add(origin, loc.first_line, loc.first_column);
+    return positions.add(origin, loc.first_column);
 }
 
 }
diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index 59f088d53..bff066170 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -438,7 +438,7 @@ Expr * parseExprFromBuf(
         .symbols = symbols,
         .positions = positions,
         .basePath = basePath,
-        .origin = {origin},
+        .origin = positions.addOrigin(origin, length),
         .rootFS = rootFS,
         .s = astSymbols,
     };
diff --git a/src/libexpr/pos-idx.hh b/src/libexpr/pos-idx.hh
index 9949f1dc5..e94fd85c6 100644
--- a/src/libexpr/pos-idx.hh
+++ b/src/libexpr/pos-idx.hh
@@ -6,6 +6,7 @@ namespace nix {
 
 class PosIdx
 {
+    friend struct LazyPosAcessors;
     friend class PosTable;
 
 private:
diff --git a/src/libexpr/pos-table.hh b/src/libexpr/pos-table.hh
index 1decf3c85..8a0a3ba86 100644
--- a/src/libexpr/pos-table.hh
+++ b/src/libexpr/pos-table.hh
@@ -7,6 +7,7 @@
 #include "chunked-vector.hh"
 #include "pos-idx.hh"
 #include "position.hh"
+#include "sync.hh"
 
 namespace nix {
 
@@ -17,66 +18,69 @@ public:
     {
         friend PosTable;
     private:
-        // must always be invalid by default, add() replaces this with the actual value.
-        // subsequent add() calls use this index as a token to quickly check whether the
-        // current origins.back() can be reused or not.
-        mutable uint32_t idx = std::numeric_limits<uint32_t>::max();
+        uint32_t offset;
 
-        // Used for searching in PosTable::[].
-        explicit Origin(uint32_t idx)
-            : idx(idx)
-            , origin{std::monostate()}
-        {
-        }
+        Origin(Pos::Origin origin, uint32_t offset, size_t size):
+            offset(offset), origin(origin), size(size)
+        {}
 
     public:
         const Pos::Origin origin;
+        const size_t size;
 
-        Origin(Pos::Origin origin)
-            : origin(origin)
+        uint32_t offsetOf(PosIdx p) const
         {
+            return p.id - 1 - offset;
         }
     };
 
-    struct Offset
-    {
-        uint32_t line, column;
-    };
-
 private:
-    std::vector<Origin> origins;
-    ChunkedVector<Offset, 8192> offsets;
+    using Lines = std::vector<uint32_t>;
 
-public:
-    PosTable()
-        : offsets(1024)
-    {
-        origins.reserve(1024);
-    }
+    std::map<uint32_t, Origin> origins;
+    mutable Sync<std::map<uint32_t, Lines>> lines;
 
-    PosIdx add(const Origin & origin, uint32_t line, uint32_t column)
+    const Origin * resolve(PosIdx p) const
     {
-        const auto idx = offsets.add({line, column}).second;
-        if (origins.empty() || origins.back().idx != origin.idx) {
-            origin.idx = idx;
-            origins.push_back(origin);
-        }
-        return PosIdx(idx + 1);
-    }
+        if (p.id == 0)
+            return nullptr;
 
-    Pos operator[](PosIdx p) const
-    {
-        if (p.id == 0 || p.id > offsets.size())
-            return {};
         const auto idx = p.id - 1;
         /* we want the last key <= idx, so we'll take prev(first key > idx).
-           this is guaranteed to never rewind origin.begin because the first
-           key is always 0. */
-        const auto pastOrigin = std::upper_bound(
-            origins.begin(), origins.end(), Origin(idx), [](const auto & a, const auto & b) { return a.idx < b.idx; });
-        const auto origin = *std::prev(pastOrigin);
-        const auto offset = offsets[idx];
-        return {offset.line, offset.column, origin.origin};
+            this is guaranteed to never rewind origin.begin because the first
+            key is always 0. */
+        const auto pastOrigin = origins.upper_bound(idx);
+        return &std::prev(pastOrigin)->second;
+    }
+
+public:
+    Origin addOrigin(Pos::Origin origin, size_t size)
+    {
+        uint32_t offset = 0;
+        if (auto it = origins.rbegin(); it != origins.rend())
+            offset = it->first + it->second.size;
+        // +1 because all PosIdx are offset by 1 to begin with, and
+        // another +1 to ensure that all origins can point to EOF, eg
+        // on (invalid) empty inputs.
+        if (2 + offset + size < offset)
+            return Origin{origin, offset, 0};
+        return origins.emplace(offset, Origin{origin, offset, size}).first->second;
+    }
+
+    PosIdx add(const Origin & origin, size_t offset)
+    {
+        if (offset > origin.size)
+            return PosIdx();
+        return PosIdx(1 + origin.offset + offset);
+    }
+
+    Pos operator[](PosIdx p) const;
+
+    Pos::Origin originOf(PosIdx p) const
+    {
+        if (auto o = resolve(p))
+            return o->origin;
+        return std::monostate{};
     }
 };
 
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 78f7f71ed..a7687fa06 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -2524,6 +2524,54 @@ static RegisterPrimOp primop_unsafeGetAttrPos(PrimOp {
     .fun = prim_unsafeGetAttrPos,
 });
 
+// access to exact position information (ie, line and colum numbers) is deferred
+// due to the cost associated with calculating that information and how rarely
+// it is used in practice. this is achieved by creating thunks to otherwise
+// inaccessible primops that are not exposed as __op or under builtins to turn
+// the internal PosIdx back into a line and column number, respectively. exposing
+// these primops in any way would at best be not useful and at worst create wildly
+// indeterministic eval results depending on parse order of files.
+//
+// in a simpler world this would instead be implemented as another kind of thunk,
+// but each type of thunk has an associated runtime cost in the current evaluator.
+// as with black holes this cost is too high to justify another thunk type to check
+// for in the very hot path that is forceValue.
+static struct LazyPosAcessors {
+    PrimOp primop_lineOfPos{
+        .arity = 1,
+        .fun = [] (EvalState & state, PosIdx pos, Value * * args, Value & v) {
+            v.mkInt(state.positions[PosIdx(args[0]->integer)].line);
+        }
+    };
+    PrimOp primop_columnOfPos{
+        .arity = 1,
+        .fun = [] (EvalState & state, PosIdx pos, Value * * args, Value & v) {
+            v.mkInt(state.positions[PosIdx(args[0]->integer)].column);
+        }
+    };
+
+    Value lineOfPos, columnOfPos;
+
+    LazyPosAcessors()
+    {
+        lineOfPos.mkPrimOp(&primop_lineOfPos);
+        columnOfPos.mkPrimOp(&primop_columnOfPos);
+    }
+
+    void operator()(EvalState & state, const PosIdx pos, Value & line, Value & column)
+    {
+        Value * posV = state.allocValue();
+        posV->mkInt(pos.id);
+        line.mkApp(&lineOfPos, posV);
+        column.mkApp(&columnOfPos, posV);
+    }
+} makeLazyPosAccessors;
+
+void makePositionThunks(EvalState & state, const PosIdx pos, Value & line, Value & column)
+{
+    makeLazyPosAccessors(state, pos, line, column);
+}
+
 /* Dynamic version of the `?' operator. */
 static void prim_hasAttr(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh
index 45486608f..9f76975db 100644
--- a/src/libexpr/primops.hh
+++ b/src/libexpr/primops.hh
@@ -51,4 +51,6 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu
  */
 void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v);
 
+void makePositionThunks(EvalState & state, const PosIdx pos, Value & line, Value & column);
+
 }
diff --git a/tests/unit/libexpr/primops.cc b/tests/unit/libexpr/primops.cc
index 6d7649b3c..b1426edae 100644
--- a/tests/unit/libexpr/primops.cc
+++ b/tests/unit/libexpr/primops.cc
@@ -151,7 +151,7 @@ namespace nix {
     }
 
     TEST_F(PrimOpTest, unsafeGetAttrPos) {
-        state.corepkgsFS->addFile(CanonPath("foo.nix"), "{ y = \"x\"; }");
+        state.corepkgsFS->addFile(CanonPath("foo.nix"), "\n\r\n\r{ y = \"x\"; }");
 
         auto expr = "builtins.unsafeGetAttrPos \"y\" (import <nix/foo.nix>)";
         auto v = eval(expr);
@@ -165,10 +165,12 @@ namespace nix {
 
         auto line = v.attrs->find(createSymbol("line"));
         ASSERT_NE(line, nullptr);
-        ASSERT_THAT(*line->value, IsIntEq(1));
+        state.forceValue(*line->value, noPos);
+        ASSERT_THAT(*line->value, IsIntEq(4));
 
         auto column = v.attrs->find(createSymbol("column"));
         ASSERT_NE(column, nullptr);
+        state.forceValue(*column->value, noPos);
         ASSERT_THAT(*column->value, IsIntEq(3));
     }
 
diff --git a/tests/unit/libexpr/value/print.cc b/tests/unit/libexpr/value/print.cc
index aabf156c2..d2d699a64 100644
--- a/tests/unit/libexpr/value/print.cc
+++ b/tests/unit/libexpr/value/print.cc
@@ -110,8 +110,8 @@ TEST_F(ValuePrintingTests, vLambda)
         .up = nullptr,
         .values = { }
     };
-    PosTable::Origin origin((std::monostate()));
-    auto posIdx = state.positions.add(origin, 1, 1);
+    PosTable::Origin origin = state.positions.addOrigin(std::monostate(), 1);
+    auto posIdx = state.positions.add(origin, 0);
     auto body = ExprInt(0);
     auto formals = Formals {};
 
@@ -558,8 +558,8 @@ TEST_F(ValuePrintingTests, ansiColorsLambda)
         .up = nullptr,
         .values = { }
     };
-    PosTable::Origin origin((std::monostate()));
-    auto posIdx = state.positions.add(origin, 1, 1);
+    PosTable::Origin origin = state.positions.addOrigin(std::monostate(), 1);
+    auto posIdx = state.positions.add(origin, 0);
     auto body = ExprInt(0);
     auto formals = Formals {};