@NinjaTrappeur Thanks so much for this! I actually think we should fail in both directions, not succeed... That is, we should have a copy of parse-fail-dup-attrs-6.nix in the opposite direction and then get that one to fail to parse as well 😄

(I think the existence of that test case is further evidence that #2077 should be a bug 😀 )

Mmh, I am a bit confused here.

I though that a.b=1; <=> a={b=1;}; [1]. Is this a false assumption?

No, you're correct. But mixing declaration styles like the test suite example does is confusing and unnecessary, it's better not to support it at all. Either use one or the other.

This becomes especially apparent in larger sets. If I see a = { b = 1; }; in a set, I shouldn't have to look at the entire rest of the set to ensure that set.a == { b = 1; }. If we allow mixed declarations, then this becomes possible.

Alright, I understand, it practically makes sense. However, I had a bad intuition about this, I just wrote a short patch roughly implementing this behavior.

--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -178,6 +178,7 @@ struct ExprOpHasAttr : Expr
 struct ExprAttrs : Expr
 {
     bool recursive;
+    bool sugarDefined;
     struct AttrDef {
         bool inherited;
         Expr * e;
@@ -197,7 +198,7 @@ struct ExprAttrs : Expr
     };
     typedef std::vector<DynamicAttrDef> DynamicAttrDefs;
     DynamicAttrDefs dynamicAttrs;
-    ExprAttrs() : recursive(false) { };
+    ExprAttrs() : recursive(false), sugarDefined(false) { };
     COMMON_METHODS
 };
 
diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index ef11dd60..aec06752 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -95,6 +95,7 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath,
                     dupAttr(attrPath, pos, j->second.pos);
             } else {
                 ExprAttrs * nested = new ExprAttrs;
+                nested->sugarDefined = true;
                 attrs->attrs[i->symbol] = ExprAttrs::AttrDef(nested, pos);
                 attrs = nested;
             }
@@ -106,6 +107,9 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath,
     }
     if (i->symbol.set()) {
         ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
+        ExprAttrs* eAttrs = dynamic_cast<ExprAttrs*>(e);
+        if (attrPath.size() > 1 && !eAttrs && !attrs->sugarDefined)
+          throw ParseError("Mixed nested attrpath.");
         if (j != attrs->attrs.end()) {
             dupAttr(attrPath, pos, j->second.pos);
         } else {

Note I did not implement a proper error handling, I was just trying to detect the missing case.

Basically, in order to detect those cases, we need to differentiate {x.x=1} from {x={x=1;};} in the AST. In my patch, this is materialized by the sugarDefined Boolean. You need to do this because while you want to prohibit {x={y=1;}; x.z=2;}, {x.y=1; z.x=2;} still needs to be valid. As the AST is currently implemented, you cannot tell the difference between those two expressions while parsing the z.x=2 binding.

Personally, I find making a difference between those two notations highly confusing. I don't like the idea of having two different semantics depending on the fact we are using or not a syntactic sugar form, I am more in favor of allowing mixed nested attrpaths altogether.

In the end, I don't really care, this is a corner case I never encountered so far :)

[edit april 2019] I actually encountered this corner case several times in the last year. I changed my mind: I care about this.

If we decide to prohibit mixed nested attrpaths, I'll close this pr, implement a proper error handling and will submit the above patch in another PR. That way, we'll still have this PR if we change our mind at some point in the future. I think it would also be nice to document this corner case in the nix manual if we decide to go that way.

@edolstra Can you make a call here? IMO mixing declarations is just begging for confusion.

Probably it would have been better to disallow both cases from the start, but we probably don't want to break backwards compatibility. So from that perspective, it's cleaner to accept both.

Does this PR merge nested attrsets at arbitrary depths, e.g.

{
  services.ssh.enable = true;
  services.ssh = { port = 123; };
  services = {
    httpd.enable = true;
  };

?

Yes. I just added this test case to this PR.

src/nix/nix eval -f tests/lang/parse-okay-mixed-nested-attrs-3.nix services
{ httpd = { enable = true; }; ssh = { enable = true; port = 123; }; }

I have a loosely-related question: is there any way to run a CI (hydra?) on a PR?

I have a loosely-related question: is there any way to run a CI (hydra?) on a PR?

Unfortunately not unless you have powers over Hydra. There's a thread on getting Travis support (#1152) but it's stalled.

What's the status of this? Is this good to merge?

It is.

The feature is tested. I can rebase the branch if needed.

I've been bitten by this particular issue several times since I pushed this PR.

Thanks, merged!