Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parser-state: fix attribute merging #11294

Merged
merged 1 commit into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions src/libexpr-tests/trivial.cc
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,57 @@ namespace nix {
)
);

// The following macros ultimately define 48 tests (16 variations on three
// templates). Each template tests an expression that can be written in 2^4
// different ways, by making four choices about whether to write a particular
// attribute path segment as `x.y = ...;` (collapsed) or `x = { y = ...; };`
// (expanded).
//
// The nestedAttrsetMergeXXXX tests check that the expression
// `{ a.b.c = 1; a.b.d = 2; }` has the same value regardless of how it is
// expanded. (That exact expression is exercised in test
// nestedAttrsetMerge0000, because it is fully collapsed. The test
// nestedAttrsetMerge1001 would instead examine
// `{ a = { b.c = 1; }; a.b = { d = 2; }; }`.)
//
// The nestedAttrsetMergeDupXXXX tests check that the expression
// `{ a.b.c = 1; a.b.c = 2; }` throws a duplicate attribute error, again
// regardless of how it is expanded.
//
// The nestedAttrsetMergeLetXXXX tests check that the expression
// `let a.b.c = 1; a.b.d = 2; in a` has the same value regardless of how it is
// expanded.
#define X_EXPAND_IF0(k, v) k "." v
#define X_EXPAND_IF1(k, v) k " = { " v " };"
#define X4(w, x, y, z) \
TEST_F(TrivialExpressionTest, nestedAttrsetMerge##w##x##y##z) { \
auto v = eval("{ a.b = { c = 1; d = 2; }; } == { " \
X_EXPAND_IF##w("a", X_EXPAND_IF##x("b", "c = 1;")) " " \
X_EXPAND_IF##y("a", X_EXPAND_IF##z("b", "d = 2;")) " }"); \
ASSERT_THAT(v, IsTrue()); \
}; \
TEST_F(TrivialExpressionTest, nestedAttrsetMergeDup##w##x##y##z) { \
ASSERT_THROW(eval("{ " \
X_EXPAND_IF##w("a", X_EXPAND_IF##x("b", "c = 1;")) " " \
X_EXPAND_IF##y("a", X_EXPAND_IF##z("b", "c = 2;")) " }"), Error); \
}; \
TEST_F(TrivialExpressionTest, nestedAttrsetMergeLet##w##x##y##z) { \
auto v = eval("{ b = { c = 1; d = 2; }; } == (let " \
X_EXPAND_IF##w("a", X_EXPAND_IF##x("b", "c = 1;")) " " \
X_EXPAND_IF##y("a", X_EXPAND_IF##z("b", "d = 2;")) " in a)"); \
ASSERT_THAT(v, IsTrue()); \
};
#define X3(...) X4(__VA_ARGS__, 0) X4(__VA_ARGS__, 1)
#define X2(...) X3(__VA_ARGS__, 0) X3(__VA_ARGS__, 1)
#define X1(...) X2(__VA_ARGS__, 0) X2(__VA_ARGS__, 1)
X1(0) X1(1)
#undef X_EXPAND_IF0
#undef X_EXPAND_IF1
#undef X1
#undef X2
#undef X3
#undef X4

TEST_F(TrivialExpressionTest, functor) {
auto v = eval("{ __functor = self: arg: self.v + arg; v = 10; } 5");
ASSERT_THAT(v, IsIntEq(15));
Expand Down
108 changes: 64 additions & 44 deletions src/libexpr/parser-state.hh
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ struct ParserState
void dupAttr(const AttrPath & attrPath, const PosIdx pos, const PosIdx prevPos);
void dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos);
void addAttr(ExprAttrs * attrs, AttrPath && attrPath, const ParserLocation & loc, Expr * e, const ParserLocation & exprLoc);
void addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symbol, ExprAttrs::AttrDef && def);
Formals * validateFormals(Formals * formals, PosIdx pos = noPos, Symbol arg = {});
Expr * stripIndentation(const PosIdx pos,
std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>> && es);
Expand Down Expand Up @@ -120,64 +121,29 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, const
// Checking attrPath validity.
// ===========================
for (i = attrPath.begin(); i + 1 < attrPath.end(); i++) {
ExprAttrs * nested;
if (i->symbol) {
ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
if (j != attrs->attrs.end()) {
if (j->second.kind != ExprAttrs::AttrDef::Kind::Inherited) {
ExprAttrs * attrs2 = dynamic_cast<ExprAttrs *>(j->second.e);
if (!attrs2) dupAttr(attrPath, pos, j->second.pos);
attrs = attrs2;
} else
nested = dynamic_cast<ExprAttrs *>(j->second.e);
if (!nested) {
attrPath.erase(i + 1, attrPath.end());
dupAttr(attrPath, pos, j->second.pos);
}
} else {
ExprAttrs * nested = new ExprAttrs;
nested = new ExprAttrs;
attrs->attrs[i->symbol] = ExprAttrs::AttrDef(nested, pos);
attrs = nested;
}
} else {
ExprAttrs *nested = new ExprAttrs;
nested = new ExprAttrs;
attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, nested, pos));
attrs = nested;
}
attrs = nested;
}
// Expr insertion.
// ==========================
if (i->symbol) {
ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
if (j != attrs->attrs.end()) {
// This attr path is already defined. However, if both
// e and the expr pointed by the attr path are two attribute sets,
// we want to merge them.
// Otherwise, throw an error.
auto ae = dynamic_cast<ExprAttrs *>(e);
auto jAttrs = dynamic_cast<ExprAttrs *>(j->second.e);
if (jAttrs && ae) {
if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
for (auto & ad : ae->attrs) {
auto j2 = jAttrs->attrs.find(ad.first);
if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error.
dupAttr(ad.first, j2->second.pos, ad.second.pos);
jAttrs->attrs.emplace(ad.first, ad.second);
if (ad.second.kind == ExprAttrs::AttrDef::Kind::InheritedFrom) {
auto & sel = dynamic_cast<ExprSelect &>(*ad.second.e);
auto & from = dynamic_cast<ExprInheritFrom &>(*sel.e);
from.displ += jAttrs->inheritFromExprs->size();
}
}
jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(), ae->dynamicAttrs.begin(), ae->dynamicAttrs.end());
if (ae->inheritFromExprs) {
jAttrs->inheritFromExprs->insert(jAttrs->inheritFromExprs->end(),
ae->inheritFromExprs->begin(), ae->inheritFromExprs->end());
}
} else {
dupAttr(attrPath, pos, j->second.pos);
}
} else {
// This attr path is not defined. Let's create it.
attrs->attrs.emplace(i->symbol, ExprAttrs::AttrDef(e, pos));
e->setName(i->symbol);
}
addAttr(attrs, attrPath, i->symbol, ExprAttrs::AttrDef(e, pos));
} else {
attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, e, pos));
}
Expand All @@ -189,6 +155,60 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, const
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this stuff should be moved to the new overload or if it should be left here; I don't really understand what it's doing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What stuff are you referring to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was referring to the above five lines involving lexerState.positionToDocComment; it's the same ‘stuff’ I'm wondering about in #11294 (comment).

}

/**
* Precondition: attrPath is used for error messages and should already contain
* symbol as its last element.
*/
inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symbol, ExprAttrs::AttrDef && def)
Mic92 marked this conversation as resolved.
Show resolved Hide resolved
{
ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(symbol);
if (j != attrs->attrs.end()) {
// This attr path is already defined. However, if both
// e and the expr pointed by the attr path are two attribute sets,
// we want to merge them.
// Otherwise, throw an error.
auto ae = dynamic_cast<ExprAttrs *>(def.e);
auto jAttrs = dynamic_cast<ExprAttrs *>(j->second.e);

// N.B. In a world in which we are less bound by our past mistakes, we
// would also test that jAttrs and ae are not recursive. The effect of
// not doing so is that any `rec` marker on ae is discarded, and any
// `rec` marker on jAttrs will apply to the attributes in ae.
// See https://github.com/NixOS/nix/issues/9020.
if (jAttrs && ae) {
if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
for (auto & ad : ae->attrs) {
if (ad.second.kind == ExprAttrs::AttrDef::Kind::InheritedFrom) {
auto & sel = dynamic_cast<ExprSelect &>(*ad.second.e);
auto & from = dynamic_cast<ExprInheritFrom &>(*sel.e);
from.displ += jAttrs->inheritFromExprs->size();
}
attrPath.emplace_back(AttrName(ad.first));
addAttr(jAttrs, attrPath, ad.first, std::move(ad.second));
attrPath.pop_back();
}
ae->attrs.clear();
jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(),
std::make_move_iterator(ae->dynamicAttrs.begin()),
std::make_move_iterator(ae->dynamicAttrs.end()));
ae->dynamicAttrs.clear();
if (ae->inheritFromExprs) {
jAttrs->inheritFromExprs->insert(jAttrs->inheritFromExprs->end(),
std::make_move_iterator(ae->inheritFromExprs->begin()),
std::make_move_iterator(ae->inheritFromExprs->end()));
ae->inheritFromExprs = nullptr;
}
Comment on lines +191 to +201
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end of this block, everything about ae worth retaining should have been merged into jAttrs. In principle I would think that means ae can be deleted. But because of the doc comment stuff lingering in the other overload, I don't think it's safe to do that. I settled for just clearing vectors instead. (I'm guessing at the right non-leaky stuff to do here, in general; are these move_iterators used correctly?)

I'm confused, though, about what good that doc comment stuff can do if it's being attached to an expression that essentially gets dropped on the floor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @roberth knows?

} else {
dupAttr(attrPath, def.pos, j->second.pos);
}
} else {
// This attr path is not defined. Let's create it.
attrs->attrs.emplace(symbol, def);
def.e->setName(symbol);
}
}

inline Formals * ParserState::validateFormals(Formals * formals, PosIdx pos, Symbol arg)
{
std::sort(formals->formals.begin(), formals->formals.end(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: undefined variable 'd'
at /pwd/lang/eval-fail-attrset-merge-drops-later-rec.nix:1:26:
1| { a.b = 1; a = rec { c = d + 2; d = 3; }; }.c
| ^
2|
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{ a.b = 1; a = rec { c = d + 2; d = 3; }; }.c
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
6
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# This is for backwards compatibility, not because we like it.
# See https://github.com/NixOS/nix/issues/9020.
{ a = rec { b = c + 1; d = 2; }; a.c = d + 3; }.a.b
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
error: attribute 'z' already defined at «stdin»:3:16
at «stdin»:2:3:
1| {
error: attribute 'x.z' already defined at «stdin»:2:3
at «stdin»:3:16:
2| x.z = 3;
| ^
3| x = { y = 3; z = 3; };
| ^
4| }
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
error: attribute 'y' already defined at «stdin»:3:9
at «stdin»:2:3:
1| {
error: attribute 'x.y.y' already defined at «stdin»:2:3
at «stdin»:3:9:
2| x.y.y = 3;
| ^
3| x = { y.y= 3; z = 3; };
| ^
4| }
Loading