Merge pull request #9874 from pennae/error-reports

improve error reports somewhat

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.

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.

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();
@@ -2777,9 +2776,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 +2793,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);
 }
 
 

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,

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;
 }
 
 

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) {
@@ -580,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

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"

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);
 }
 
 }

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)]
@@ -87,6 +91,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 +102,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 +315,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 +330,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,20 +346,20 @@ 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({
  .msg = HintFmt("dynamic attributes not allowed in inherit"),
  .pos = state->positions[state->at(@2)]
  });
  }
- | { $$ = new AttrPath; }
+ | { $$ = new std::vector<std::pair<AttrName, PosIdx>>; }
  ;
 
 attrpath
@@ -433,7 +438,7 @@ Expr * parseExprFromBuf(
  .symbols = symbols,
  .positions = positions,
  .basePath = basePath,
- .origin = {origin},
+ .origin = positions.addOrigin(origin, length),
  .rootFS = rootFS,
  .s = astSymbols,
  };

src/libexpr/pos-idx.hh

@@ -6,6 +6,7 @@ namespace nix {
 
 class PosIdx
 {
+ friend struct LazyPosAcessors;
  friend class PosTable;
 
 private: