* Store attribute positions in the AST and report duplicate attribute

  errors with position info.
* For all positions, use the position of the first character of the
  first token, rather than the last character of the first token plus
  one.

6 files changed, 56 insertions, 56 deletions

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index b4f12c8a9c..cb124ab8ba 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -192,11 +192,6 @@ LocalNoInlineNoReturn(void throwAssertionError(const char * s, const Pos & pos))
     throw AssertionError(format(s) % pos);
 }
 
-LocalNoInline(void addErrorPrefix(Error & e, const char * s))
-{
-    e.addPrefix(s);
-}
-
 LocalNoInline(void addErrorPrefix(Error & e, const char * s, const string & s2))
 {
     e.addPrefix(format(s) % s2);
@@ -207,11 +202,6 @@ LocalNoInline(void addErrorPrefix(Error & e, const char * s, const Pos & pos))
     e.addPrefix(format(s) % pos);
 }
 
-LocalNoInline(void addErrorPrefix(Error & e, const char * s, const string & s2, const string & s3))
-{
-    e.addPrefix(format(s) % s2 % s3);
-}
-
 
 void mkString(Value & v, const char * s)
 {
@@ -426,7 +416,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
         foreach (Attrs::iterator, i, attrs) {
             Value & v2 = (*v.attrs)[i->first];
             mkCopy(v2, env2.values[displ]);
-            mkThunk(env2.values[displ++], env2, i->second);
+            mkThunk(env2.values[displ++], env2, i->second.first);
         }
 
         /* The inherited attributes, on the other hand, are
@@ -443,7 +433,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
     else {
         foreach (Attrs::iterator, i, attrs) {
             Value & v2 = (*v.attrs)[i->first];
-            mkThunk(v2, env, i->second);
+            mkThunk(v2, env, i->second.first);
         }
 
         foreach (list<VarRef>::iterator, i, inherited) {
@@ -466,7 +456,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
     /* The recursive attributes are evaluated in the new
        environment. */
     foreach (ExprAttrs::Attrs::iterator, i, attrs->attrs)
-        mkThunk(env2.values[displ++], env2, i->second);
+        mkThunk(env2.values[displ++], env2, i->second.first);
 
     /* The inherited attributes, on the other hand, are evaluated in
        the original environment. */
diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l
index 00de57a7ba..f29f9b6843 100644
--- a/src/libexpr/lexer.l
+++ b/src/libexpr/lexer.l
@@ -19,13 +19,16 @@ namespace nix {
     
 static void initLoc(YYLTYPE * loc)
 {
-    loc->first_line = 1;
-    loc->first_column = 1;
+    loc->first_line = loc->last_line = 1;
+    loc->first_column = loc->last_column = 1;
 }
 
     
 static void adjustLoc(YYLTYPE * loc, const char * s, size_t len)
 {
+    loc->first_line = loc->last_line;
+    loc->first_column = loc->last_column;
+
     while (len--) {
        switch (*s++) {
        case '\r':
@@ -33,11 +36,11 @@ static void adjustLoc(YYLTYPE * loc, const char * s, size_t len)
                s++;
            /* fall through */
        case '\n': 
-           ++loc->first_line;
-           loc->first_column = 1;
+           ++loc->last_line;
+           loc->last_column = 1;
            break;
        default:
-           ++loc->first_column;
+           ++loc->last_column;
        }
     }
 }
diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc
index a9c83108e9..af0632a94d 100644
--- a/src/libexpr/nixexpr.cc
+++ b/src/libexpr/nixexpr.cc
@@ -58,7 +58,7 @@ void ExprAttrs::show(std::ostream & str)
     foreach (list<VarRef>::iterator, i, inherited)
         str << "inherit " << i->name << "; ";
     foreach (Attrs::iterator, i, attrs)
-        str << i->first << " = " << *i->second << "; ";
+        str << i->first << " = " << *i->second.first << "; ";
     str << "}";
 }
 
@@ -94,7 +94,7 @@ void ExprLet::show(std::ostream & str)
     foreach (list<VarRef>::iterator, i, attrs->inherited)
         str << "inherit " << i->name << "; ";
     foreach (ExprAttrs::Attrs::iterator, i, attrs->attrs)
-        str << i->first << " = " << *i->second << "; ";
+        str << i->first << " = " << *i->second.first << "; ";
     str << "in " << *body;
 }
 
@@ -138,6 +138,9 @@ std::ostream & operator << (std::ostream & str, const Pos & pos)
 }
 
 
+Pos noPos;
+
+
 /* Computing levels/displacements for variables. */
 
 void Expr::bindVars(const StaticEnv & env)
@@ -218,12 +221,12 @@ void ExprAttrs::bindVars(const StaticEnv & env)
         }
 
         foreach (ExprAttrs::Attrs::iterator, i, attrs)
-            i->second->bindVars(newEnv);
+            i->second.first->bindVars(newEnv);
     }
 
     else {
         foreach (ExprAttrs::Attrs::iterator, i, attrs)
-            i->second->bindVars(env);
+            i->second.first->bindVars(env);
 
         foreach (list<VarRef>::iterator, i, inherited)
             i->bind(env);
@@ -270,7 +273,7 @@ void ExprLet::bindVars(const StaticEnv & env)
     }
 
     foreach (ExprAttrs::Attrs::iterator, i, attrs->attrs)
-        i->second->bindVars(newEnv);
+        i->second.first->bindVars(newEnv);
     
     body->bindVars(newEnv);
 }
diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
index 2d328d382c..36cb4e53cd 100644
--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -23,8 +23,13 @@ struct Pos
 {
     string file;
     unsigned int line, column;
+    Pos() : line(0), column(0) { };
+    Pos(const string & file, unsigned int line, unsigned int column)
+        : file(file), line(line), column(column) { };
 };
 
+extern Pos noPos;
+
 std::ostream & operator << (std::ostream & str, const Pos & pos);
 
 
@@ -125,10 +130,11 @@ struct ExprOpHasAttr : Expr
 struct ExprAttrs : Expr
 {
     bool recursive;
-    typedef std::map<Symbol, Expr *> Attrs;
+    typedef std::pair<Expr *, Pos> Attr;
+    typedef std::map<Symbol, Attr> Attrs;
     Attrs attrs;
     list<VarRef> inherited;
-    set<Symbol> attrNames; // used during parsing
+    std::map<Symbol, Pos> attrNames; // used during parsing
     ExprAttrs() : recursive(false) { };
     COMMON_METHODS
 };
diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index 66da769402..99980240f8 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -61,18 +61,18 @@ static string showAttrPath(const vector<Symbol> & attrPath)
 }
 
 
-static void dupAttr(const vector<Symbol> & attrPath, const Pos & pos)
+static void dupAttr(const vector<Symbol> & attrPath, const Pos & pos, const Pos & prevPos)
 {
-    throw ParseError(format("attribute `%1%' at %2% already defined at <SOMEWHERE>")
-        % showAttrPath(attrPath) % pos);
+    throw ParseError(format("attribute `%1%' at %2% already defined at %3%")
+        % showAttrPath(attrPath) % pos % prevPos);
 }
  
 
-static void dupAttr(Symbol attr, const Pos & pos)
+static void dupAttr(Symbol attr, const Pos & pos, const Pos & prevPos)
 {
     vector<Symbol> attrPath; attrPath.push_back(attr);
-    throw ParseError(format("attribute `%1%' at %2% already defined at <SOMEWHERE>")
-        % showAttrPath(attrPath) % pos);
+    throw ParseError(format("attribute `%1%' at %2% already defined at %3%")
+        % showAttrPath(attrPath) % pos % prevPos);
 }
  
 
@@ -82,19 +82,20 @@ static void addAttr(ExprAttrs * attrs, const vector<Symbol> & attrPath,
     unsigned int n = 0;
     foreach (vector<Symbol>::const_iterator, i, attrPath) {
         n++;
-        if (attrs->attrs[*i]) {
-            ExprAttrs * attrs2 = dynamic_cast<ExprAttrs *>(attrs->attrs[*i]);
-            if (!attrs2 || n == attrPath.size()) dupAttr(attrPath, pos);
+        ExprAttrs::Attrs::iterator j = attrs->attrs.find(*i);
+        if (j != attrs->attrs.end()) {
+            ExprAttrs * attrs2 = dynamic_cast<ExprAttrs *>(j->second.first);
+            if (!attrs2 || n == attrPath.size()) dupAttr(attrPath, pos, j->second.second);
             attrs = attrs2;
         } else {
             if (attrs->attrNames.find(*i) != attrs->attrNames.end())
-                dupAttr(attrPath, pos);
-            attrs->attrNames.insert(*i);
+                dupAttr(attrPath, pos, attrs->attrNames[*i]);
+            attrs->attrNames[*i] = pos;
             if (n == attrPath.size())
-                attrs->attrs[*i] = e;
+                attrs->attrs[*i] = ExprAttrs::Attr(e, pos);
             else {
                 ExprAttrs * nested = new ExprAttrs;
-                attrs->attrs[*i] = nested;
+                attrs->attrs[*i] = ExprAttrs::Attr(nested, pos);
                 attrs = nested;
             }
         }
@@ -205,16 +206,12 @@ void backToString(yyscan_t scanner);
 void backToIndString(yyscan_t scanner);
 
 
-static Pos makeCurPos(YYLTYPE * loc, ParseData * data)
+static Pos makeCurPos(const YYLTYPE & loc, ParseData * data)
 {
-    Pos pos;
-    pos.file = data->path;
-    pos.line = loc->first_line;
-    pos.column = loc->first_column;
-    return pos;
+    return Pos(data->path, loc.first_line, loc.first_column);
 }
 
-#define CUR_POS makeCurPos(yylocp, data)
+#define CUR_POS makeCurPos(*yylocp, data)
 
 
 }
@@ -223,7 +220,7 @@ static Pos makeCurPos(YYLTYPE * loc, ParseData * data)
 void yyerror(YYLTYPE * loc, yyscan_t scanner, ParseData * data, const char * error)
 {
     data->error = (format("%1%, at %2%")
-        % error % makeCurPos(loc, data)).str();
+        % error % makeCurPos(*loc, data)).str();
 }
 
 
@@ -374,14 +371,14 @@ ind_string_parts
   ;
 
 binds
-  : binds attrpath '=' expr ';' { $$ = $1; addAttr($$, *$2, $4, CUR_POS); }
+  : binds attrpath '=' expr ';' { $$ = $1; addAttr($$, *$2, $4, makeCurPos(@2, data)); }
   | binds INHERIT ids ';'
     { $$ = $1;
       foreach (vector<Symbol>::iterator, i, *$3) {
           if ($$->attrNames.find(*i) != $$->attrNames.end())
-              dupAttr(*i, CUR_POS);
+              dupAttr(*i, makeCurPos(@3, data), $$->attrNames[*i]);
           $$->inherited.push_back(*i);
-          $$->attrNames.insert(*i);
+          $$->attrNames[*i] = makeCurPos(@3, data);
       }
     }
   | binds INHERIT '(' expr ')' ids ';'
@@ -389,11 +386,11 @@ binds
       /* !!! Should ensure sharing of the expression in $4. */
       foreach (vector<Symbol>::iterator, i, *$6) {
           if ($$->attrNames.find(*i) != $$->attrNames.end())
-              dupAttr(*i, CUR_POS);
-          $$->attrs[*i] = new ExprSelect($4, *i);
-          $$->attrNames.insert(*i);
-      }
-    }
+              dupAttr(*i, makeCurPos(@6, data), $$->attrNames[*i]);
+          $$->attrs[*i] = ExprAttrs::Attr(new ExprSelect($4, *i), makeCurPos(@6, data));
+          $$->attrNames[*i] = makeCurPos(@6, data);
+      }}
+
   | { $$ = new ExprAttrs; }
   ;
 
diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc
index 4a9df454d4..12a256a091 100644
--- a/src/nix-env/nix-env.cc
+++ b/src/nix-env/nix-env.cc
@@ -132,7 +132,7 @@ static void getAllExprs(EvalState & state,
             if (hasSuffix(attrName, ".nix"))
                 attrName = string(attrName, 0, attrName.size() - 4);
             attrs.attrs[state.symbols.create(attrName)] =
-                parseExprFromFile(state, absPath(path2));
+                ExprAttrs::Attr(parseExprFromFile(state, absPath(path2)), noPos);
         }
         else
             /* `path2' is a directory (with no default.nix in it);
@@ -153,7 +153,8 @@ static Expr * loadSourceExpr(EvalState & state, const Path & path)
        for a user to have a ~/.nix-defexpr directory that includes
        some system-wide directory). */
     ExprAttrs * attrs = new ExprAttrs;
-    attrs->attrs[state.symbols.create("_combineChannels")] = new ExprList();
+    attrs->attrs[state.symbols.create("_combineChannels")] =
+        ExprAttrs::Attr(new ExprList(), noPos);
     getAllExprs(state, path, *attrs);
     return attrs;
 }