Better eval error locations for interpolation and +

Previously, type or coercion errors for string interpolation, path interpolation, and plus expressions were always reported at the beginning of the outer expression. This leads to confusing evaluation error messages making it hard to accurately diagnose and then fix the error. For example, errors were reported as follows. ``` cannot coerce an integer to a string 1| let foo = 7; in "bar" + foo | ^ cannot add a string to an integer 1| let foo = "bar"; in 4 + foo | ^ cannot coerce an integer to a string 1| let foo = 7; in "x${foo}" | ^ ``` This commit changes the ExprConcatStrings expression vector to store a sequence of expressions *and* their expansion locations so that error locations can be reported accurately. For interpolation, the error is reported at the beginning of the entire `${foo}`, not at the beginning of `foo` because I thought this was slightly clearer. The previous errors are now reported as: ``` cannot coerce an integer to a string 1| let foo = 7; in "bar" + foo | ^ cannot add a string to an integer 1| let foo = "bar"; in 4 + foo | ^ cannot coerce an integer to a string 1| let foo = 7; in "x${foo}" | ^ ``` The error is reported at this kind of precise location even for multi-line indented strings. This probably helps with at least some of the cases mentioned in #561
NixOS · Sep 23, 2021 · 9d67332 · 9d67332
1 parent bcd73eb
commit 9d67332
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 27 deletions.
diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
@@ -1577,7 +1577,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
  bool first = !forceString;
  ValueType firstType = nString;
 
- for (auto & i : *es) {
+ for (auto & [i_pos, i] : *es) {
  Value vTmp;
  i->eval(state, env, vTmp);
 
@@ -1598,19 +1598,19 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
  nf = n;
  nf += vTmp.fpoint;
  } else
- throwEvalError(pos, "cannot add %1% to an integer", showType(vTmp));
+ throwEvalError(i_pos, "cannot add %1% to an integer", showType(vTmp));
  } else if (firstType == nFloat) {
  if (vTmp.type() == nInt) {
  nf += vTmp.integer;
  } else if (vTmp.type() == nFloat) {
  nf += vTmp.fpoint;
  } else
- throwEvalError(pos, "cannot add %1% to a float", showType(vTmp));
+ throwEvalError(i_pos, "cannot add %1% to a float", showType(vTmp));
  } else
  /* skip canonization of first path, which would only be not
  canonized in the first place if it's coming from a ./${foo} type
  path */
- s << state.coerceToString(pos, vTmp, context, false, firstType == nString, !first);
+ s << state.coerceToString(i_pos, vTmp, context, false, firstType == nString, !first);
 
  first = false;
  }

diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc
@@ -181,7 +181,7 @@ void ExprConcatStrings::show(std::ostream & str) const
  str << "(";
  for (auto & i : *es) {
  if (first) first = false; else str << " + ";
- str << *i;
+ str << i.second;
  }
  str << ")";
 }
@@ -413,7 +413,7 @@ void ExprOpNot::bindVars(const StaticEnv & env)
 void ExprConcatStrings::bindVars(const StaticEnv & env)
 {
  for (auto & i : *es)
- i->bindVars(env);
+ i.second->bindVars(env);
 }
 
 void ExprPos::bindVars(const StaticEnv & env)

diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
@@ -321,8 +321,8 @@ struct ExprConcatStrings : Expr
 {
  Pos pos;
  bool forceString;
- vector<Expr *> * es;
- ExprConcatStrings(const Pos & pos, bool forceString, vector<Expr *> * es)
+ vector<std::pair<Pos, Expr *> > * es;
+ ExprConcatStrings(const Pos & pos, bool forceString, vector<std::pair<Pos, Expr *> > * es)
  : pos(pos), forceString(forceString), es(es) { };
  COMMON_METHODS
 };

diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
@@ -154,7 +154,7 @@ static void addFormal(const Pos & pos, Formals * formals, const Formal & formal)
 }
 
 
-static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector<Expr *> & es)
+static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector<std::pair<Pos, Expr *> > & es)
 {
  if (es.empty()) return new ExprString(symbols.create(""));
 
@@ -164,7 +164,7 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector<Ex
  bool atStartOfLine = true; /* = seen only whitespace in the current line */
  size_t minIndent = 1000000;
  size_t curIndent = 0;
- for (auto & i : es) {
+ for (auto & [i_pos, i] : es) {
  ExprIndStr * e = dynamic_cast<ExprIndStr *>(i);
  if (!e) {
  /* Anti-quotations end the current start-of-line whitespace. */
@@ -194,12 +194,12 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector<Ex
  }
 
  /* Strip spaces from each line. */
- vector<Expr *> * es2 = new vector<Expr *>;
+ vector<std::pair<Pos, Expr *> > * es2 = new vector<std::pair<Pos, Expr *> >;
  atStartOfLine = true;
  size_t curDropped = 0;
  size_t n = es.size();
- for (vector<Expr *>::iterator i = es.begin(); i != es.end(); ++i, --n) {
- ExprIndStr * e = dynamic_cast<ExprIndStr *>(*i);
+ for (vector<std::pair<Pos, Expr *> >::iterator i = es.begin(); i != es.end(); ++i, --n) {
+ ExprIndStr * e = dynamic_cast<ExprIndStr *>(i->second);
  if (!e) {
  atStartOfLine = false;
  curDropped = 0;
@@ -236,11 +236,11 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector<Ex
  s2 = string(s2, 0, p + 1);
  }
 
- es2->push_back(new ExprString(symbols.create(s2)));
+ es2->emplace_back(i->first, new ExprString(symbols.create(s2)));
  }
 
  /* If this is a single string, then don't do a concatenation. */
- return es2->size() == 1 && dynamic_cast<ExprString *>((*es2)[0]) ? (*es2)[0] : new ExprConcatStrings(pos, true, es2);
+ return es2->size() == 1 && dynamic_cast<ExprString *>((*es2)[0].second) ? (*es2)[0].second : new ExprConcatStrings(pos, true, es2);
 }
 
 
@@ -279,7 +279,7 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParseData * data, const char * err
  char * path;
  char * uri;
  std::vector<nix::AttrName> * attrNames;
- std::vector<nix::Expr *> * string_parts;
+ std::vector<std::pair<nix::Pos, nix::Expr *> > * string_parts;
 }
 
 %type <e> start expr expr_function expr_if expr_op
@@ -366,7 +366,7 @@ expr_op
  | expr_op UPDATE expr_op { $$ = new ExprOpUpdate(CUR_POS, $1, $3); }
  | expr_op '?' attrpath { $$ = new ExprOpHasAttr($1, *$3); }
  | expr_op '+' expr_op
- { $$ = new ExprConcatStrings(CUR_POS, false, new vector<Expr *>({$1, $3})); }
+ { $$ = new ExprConcatStrings(CUR_POS, false, new vector<std::pair<Pos, Expr *> >({{makeCurPos(@1, data), $1}, {makeCurPos(@3, data), $3}})); }
  | expr_op '-' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__sub")), $1), $3); }
  | expr_op '*' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__mul")), $1), $3); }
  | expr_op '/' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__div")), $1), $3); }
@@ -407,7 +407,7 @@ expr_simple
  }
  | path_start PATH_END { $$ = $1; }
  | path_start string_parts_interpolated PATH_END {
- $2->insert($2->begin(), $1);
+ $2->insert($2->begin(), {makeCurPos(@1, data), $1});
  $$ = new ExprConcatStrings(CUR_POS, false, $2);
  }
  | SPATH {
@@ -445,13 +445,13 @@ string_parts
  ;
 
 string_parts_interpolated
- : string_parts_interpolated STR { $$ = $1; $1->push_back($2); }
- | string_parts_interpolated DOLLAR_CURLY expr '}' { $$ = $1; $1->push_back($3); }
- | DOLLAR_CURLY expr '}' { $$ = new vector<Expr *>; $$->push_back($2); }
+ : string_parts_interpolated STR { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $2); }
+ | string_parts_interpolated DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $3); }
+ | DOLLAR_CURLY expr '}' { $$ = new vector<std::pair<Pos, Expr *> >; $$->emplace_back(makeCurPos(@1, data), $2); }
  | STR DOLLAR_CURLY expr '}' {
- $$ = new vector<Expr *>;
- $$->push_back($1);
- $$->push_back($3);
+ $$ = new vector<std::pair<Pos, Expr *> >;
+ $$->emplace_back(makeCurPos(@1, data), $1);
+ $$->emplace_back(makeCurPos(@2, data), $3);
  }
  ;
 
@@ -470,9 +470,9 @@ path_start
  ;
 
 ind_string_parts
- : ind_string_parts IND_STR { $$ = $1; $1->push_back($2); }
- | ind_string_parts DOLLAR_CURLY expr '}' { $$ = $1; $1->push_back($3); }
- | { $$ = new vector<Expr *>; }
+ : ind_string_parts IND_STR { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $2); }
+ | ind_string_parts DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $3); }
+ | { $$ = new vector<std::pair<Pos, Expr *> >; }
  ;
 
 binds