Skip to content

libexpr: store ExprLambda data in Expr::alloc #14384

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

libexpr: store ExprLambda data in Expr::alloc #14384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23d9d8b
libexpr: store ExprLambda data in Expr::alloc
Radvendii Oct 27, 2025
4673db3
add test for no formals case
Radvendii Oct 27, 2025
5db63f3
restore proper handling of no formals vs. 0 formals
Radvendii Oct 27, 2025
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
2 changes: 1 addition & 1 deletion src/libexpr-tests/primops.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ TEST_F(PrimOpTest, derivation)
ASSERT_EQ(v.type(), nFunction);
ASSERT_TRUE(v.isLambda());
ASSERT_NE(v.lambda().fun, nullptr);
ASSERT_TRUE(v.lambda().fun->hasFormals());
ASSERT_TRUE(v.lambda().fun->hasFormals);
}

TEST_F(PrimOpTest, currentTime)
Expand Down
4 changes: 2 additions & 2 deletions src/libexpr-tests/value/print.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ TEST_F(ValuePrintingTests, vLambda)
auto body = ExprInt(0);
auto formals = Formals{};

ExprLambda eLambda(posIdx, createSymbol("a"), &formals, &body);
ExprLambda eLambda(state.mem.exprs.alloc, posIdx, createSymbol("a"), formals, &body);

Value vLambda;
vLambda.mkLambda(&env, &eLambda);
Expand Down Expand Up @@ -502,7 +502,7 @@ TEST_F(ValuePrintingTests, ansiColorsLambda)
auto body = ExprInt(0);
auto formals = Formals{};

ExprLambda eLambda(posIdx, createSymbol("a"), &formals, &body);
ExprLambda eLambda(state.mem.exprs.alloc, posIdx, createSymbol("a"), formals, &body);

Value vLambda;
vLambda.mkLambda(&env, &eLambda);
Expand Down
20 changes: 10 additions & 10 deletions src/libexpr/eval.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1496,13 +1496,13 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,

ExprLambda & lambda(*vCur.lambda().fun);

auto size = (!lambda.arg ? 0 : 1) + (lambda.hasFormals() ? lambda.formals->formals.size() : 0);
auto size = (!lambda.arg ? 0 : 1) + (lambda.hasFormals ? lambda.getFormals().size() : 0);
Env & env2(mem.allocEnv(size));
env2.up = vCur.lambda().env;

Displacement displ = 0;

if (!lambda.hasFormals())
if (!lambda.hasFormals)
env2.values[displ++] = args[0];
else {
try {
Expand All @@ -1520,7 +1520,7 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,
there is no matching actual argument but the formal
argument has a default, use the default. */
size_t attrsUsed = 0;
for (auto & i : lambda.formals->formals) {
for (auto & i : lambda.getFormals()) {
auto j = args[0]->attrs()->get(i.name);
if (!j) {
if (!i.def) {
Expand All @@ -1542,13 +1542,13 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,

/* Check that each actual argument is listed as a formal
argument (unless the attribute match specifies a `...'). */
if (!lambda.formals->ellipsis && attrsUsed != args[0]->attrs()->size()) {
if (!lambda.ellipsis && attrsUsed != args[0]->attrs()->size()) {
/* Nope, so show the first unexpected argument to the
user. */
for (auto & i : *args[0]->attrs())
if (!lambda.formals->has(i.name)) {
if (!lambda.hasFormal(i.name)) {
StringSet formalNames;
for (auto & formal : lambda.formals->formals)
for (auto & formal : lambda.getFormals())
formalNames.insert(std::string(symbols[formal.name]));
auto suggestions = Suggestions::bestMatches(formalNames, symbols[i.name]);
error<TypeError>(
Expand Down Expand Up @@ -1747,22 +1747,22 @@ void EvalState::autoCallFunction(const Bindings & args, Value & fun, Value & res
}
}

if (!fun.isLambda() || !fun.lambda().fun->hasFormals()) {
if (!fun.isLambda() || !fun.lambda().fun->hasFormals) {
res = fun;
return;
}

auto attrs = buildBindings(std::max(static_cast<uint32_t>(fun.lambda().fun->formals->formals.size()), args.size()));
auto attrs = buildBindings(std::max(static_cast<uint32_t>(fun.lambda().fun->nFormals), args.size()));

if (fun.lambda().fun->formals->ellipsis) {
if (fun.lambda().fun->ellipsis) {
// If the formals have an ellipsis (eg the function accepts extra args) pass
// all available automatic arguments (which includes arguments specified on
// the command line via --arg/--argstr)
for (auto & v : args)
attrs.insert(v);
} else {
// Otherwise, only pass the arguments that the function accepts
for (auto & i : fun.lambda().fun->formals->formals) {
for (auto & i : fun.lambda().fun->getFormals()) {
auto j = args.get(i.name);
if (j) {
attrs.insert(*j);
Expand Down
62 changes: 43 additions & 19 deletions src/libexpr/include/nix/expr/nixexpr.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,53 +475,77 @@ struct Formals
formals.begin(), formals.end(), arg, [](const Formal & f, const Symbol & sym) { return f.name < sym; });
return it != formals.end() && it->name == arg;
}

std::vector<Formal> lexicographicOrder(const SymbolTable & symbols) const
{
std::vector<Formal> result(formals.begin(), formals.end());
std::sort(result.begin(), result.end(), [&](const Formal & a, const Formal & b) {
std::string_view sa = symbols[a.name], sb = symbols[b.name];
return sa < sb;
});
return result;
}
};

struct ExprLambda : Expr
{
PosIdx pos;
Symbol name;
Symbol arg;
Formals * formals;

bool ellipsis;
bool hasFormals;
uint16_t nFormals;
Formal * formalsStart;
Comment on lines +488 to +489
Copy link
Member

@Ericson2314 Ericson2314 Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
uint16_t nFormals;
Formal * formalsStart;
std::span<Formal> formals;

Can we do that?

Copy link
Member

Choose a reason for hiding this comment

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

oh we can't because padding. Fair enough. Add comment then?

Copy link
Member

Choose a reason for hiding this comment

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

oh and maybe make the underlying fields private too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That change would add 8 bytes to the struct.

  • std::span uses a uint32_t for the size (2 more bytes)
  • 6 bytes of padding

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the private fields to preserve packing replaces the original suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I'm not a big fan of private fields, but I know I'm in the minority here and it doesn't matter to me that much.

However, we haven't been making the fields private in any of the other Expr subtypes. I'd rather change that for all the relevant Expr sub-types in a separate MR, rather than have it be inconsistent.

Copy link
Member

@Ericson2314 Ericson2314 Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I will say from a language design perspective, putting private information in the public header / interface file is disgusting, and C++ should feel bad about it. Does that match your feelings about private fields? That said, I do think abstract data types are fine in principle, and since we want to manually control layout while not foisting these encoding details on other code, I think private fields are the best tool we have in C++ for that job.

I see you have fixed it with bool hasFormals. I think we should have

struct FormalsSpan{
    std::span<Formal> formals;
    bool ellipsis;
}

and then

[[gnu::always_inline]] std::optional<std::span<FormalsSpan>> getOptFormals() const

This will also enforce that the original bool ellipsis is never used if hasFormals is false, which further ensures correctness.

And overall the less trivial getOptFormals now becomes, the more I think the private field are worth it.

Does this sound OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will say from a language design perspective, putting private information in the public header / interface file is disgusting

No I think we're on opposite pages here 😆. I feel like fields should always be accessible, especially within a codebase like this. I understand the benefits of encapsulation it's just not the style I prefer.

I think your idea for structure is good. I'll implement it when I have a chance.


Expr * body;
DocComment docComment;

ExprLambda(PosIdx pos, Symbol arg, Formals * formals, Expr * body)
ExprLambda(
std::pmr::polymorphic_allocator<char> & alloc, PosIdx pos, Symbol arg, const Formals & formals, Expr * body)
: pos(pos)
, arg(arg)
, formals(formals)
, body(body) {};
, ellipsis(formals.ellipsis)
, hasFormals(true)
, nFormals(formals.formals.size())
, formalsStart(alloc.allocate_object<Formal>(nFormals))
, body(body)
{
std::ranges::copy(formals.formals, formalsStart);
};

ExprLambda(PosIdx pos, Formals * formals, Expr * body)
ExprLambda(std::pmr::polymorphic_allocator<char> & alloc, PosIdx pos, Symbol arg, Expr * body)
: pos(pos)
, formals(formals)
, body(body)
, arg(arg)
, hasFormals(false)
, formalsStart(nullptr)
, body(body) {};

ExprLambda(std::pmr::polymorphic_allocator<char> & alloc, PosIdx pos, Formals formals, Expr * body)
: ExprLambda(alloc, pos, Symbol(), formals, body) {};

bool hasFormal(Symbol arg) const
{
auto formals = getFormals();
auto it = std::lower_bound(
formals.begin(), formals.end(), arg, [](const Formal & f, const Symbol & sym) { return f.name < sym; });
return it != formals.end() && it->name == arg;
}

void setName(Symbol name) override;
std::string showNamePos(const EvalState & state) const;

inline bool hasFormals() const
std::vector<Formal> getFormalsLexicographic(const SymbolTable & symbols) const
{
return formals != nullptr;
std::vector<Formal> result(getFormals().begin(), getFormals().end());
std::sort(result.begin(), result.end(), [&](const Formal & a, const Formal & b) {
std::string_view sa = symbols[a.name], sb = symbols[b.name];
return sa < sb;
});
return result;
}

PosIdx getPos() const override
{
return pos;
}

std::span<Formal> getFormals() const
{
assert(hasFormals);
return {formalsStart, nFormals};
}

virtual void setDocComment(DocComment docComment) override;
COMMON_METHODS
};
Expand Down
16 changes: 7 additions & 9 deletions src/libexpr/include/nix/expr/parser-state.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ struct ParserState
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 = {});
void validateFormals(Formals & formals, PosIdx pos = noPos, Symbol arg = {});
Expr * stripIndentation(const PosIdx pos, std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>> && es);
PosIdx at(const ParserLocation & loc);
};
Expand Down Expand Up @@ -213,29 +213,27 @@ ParserState::addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symb
}
}

inline Formals * ParserState::validateFormals(Formals * formals, PosIdx pos, Symbol arg)
inline void ParserState::validateFormals(Formals & formals, PosIdx pos, Symbol arg)
{
std::sort(formals->formals.begin(), formals->formals.end(), [](const auto & a, const auto & b) {
std::sort(formals.formals.begin(), formals.formals.end(), [](const auto & a, const auto & b) {
return std::tie(a.name, a.pos) < std::tie(b.name, b.pos);
});

std::optional<std::pair<Symbol, PosIdx>> duplicate;
for (size_t i = 0; i + 1 < formals->formals.size(); i++) {
if (formals->formals[i].name != formals->formals[i + 1].name)
for (size_t i = 0; i + 1 < formals.formals.size(); i++) {
if (formals.formals[i].name != formals.formals[i + 1].name)
continue;
std::pair thisDup{formals->formals[i].name, formals->formals[i + 1].pos};
std::pair thisDup{formals.formals[i].name, formals.formals[i + 1].pos};
duplicate = std::min(thisDup, duplicate.value_or(thisDup));
}
if (duplicate)
throw ParseError(
{.msg = HintFmt("duplicate formal function argument '%1%'", symbols[duplicate->first]),
.pos = positions[duplicate->second]});

if (arg && formals->has(arg))
if (arg && formals.has(arg))
throw ParseError(
{.msg = HintFmt("duplicate formal function argument '%1%'", symbols[arg]), .pos = positions[pos]});

return formals;
}

inline Expr *
Expand Down
15 changes: 7 additions & 8 deletions src/libexpr/nixexpr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,13 @@ void ExprList::show(const SymbolTable & symbols, std::ostream & str) const
void ExprLambda::show(const SymbolTable & symbols, std::ostream & str) const
{
str << "(";
if (hasFormals()) {
if (hasFormals) {
str << "{ ";
bool first = true;
// 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)) {
for (auto & i : getFormalsLexicographic(symbols)) {
if (first)
first = false;
else
Expand All @@ -171,7 +171,7 @@ void ExprLambda::show(const SymbolTable & symbols, std::ostream & str) const
i.def->show(symbols, str);
}
}
if (formals->ellipsis) {
if (ellipsis) {
if (!first)
str << ", ";
str << "...";
Expand Down Expand Up @@ -451,21 +451,20 @@ void ExprLambda::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv>
if (es.debugRepl)
es.exprEnvs.insert(std::make_pair(this, env));

auto newEnv =
std::make_shared<StaticEnv>(nullptr, env, (hasFormals() ? formals->formals.size() : 0) + (!arg ? 0 : 1));
auto newEnv = std::make_shared<StaticEnv>(nullptr, env, (hasFormals ? getFormals().size() : 0) + (!arg ? 0 : 1));

Displacement displ = 0;

if (arg)
newEnv->vars.emplace_back(arg, displ++);

if (hasFormals()) {
for (auto & i : formals->formals)
if (hasFormals) {
for (auto & i : getFormals())
newEnv->vars.emplace_back(i.name, displ++);

newEnv->sort();

for (auto & i : formals->formals)
for (auto & i : getFormals())
if (i.def)
i.def->bindVars(es, newEnv);
}
Expand Down
28 changes: 16 additions & 12 deletions src/libexpr/parser.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ static Expr * makeCall(PosIdx pos, Expr * fn, Expr * arg) {
%type <nix::Expr *> expr_pipe_from expr_pipe_into
%type <nix::ExprList *> expr_list
%type <nix::ExprAttrs *> binds binds1
%type <nix::Formals *> formals formal_set
%type <nix::Formals> formals formal_set
%type <nix::Formal> formal
%type <std::vector<nix::AttrName>> attrpath
%type <std::vector<std::pair<nix::AttrName, nix::PosIdx>>> attrs
Expand Down Expand Up @@ -179,26 +179,30 @@ expr: expr_function;

expr_function
: ID ':' expr_function
{ auto me = new ExprLambda(CUR_POS, state->symbols.create($1), 0, $3);
{ auto me = new ExprLambda(state->alloc, CUR_POS, state->symbols.create($1), $3);
$$ = me;
SET_DOC_POS(me, @1);
}
| formal_set ':' expr_function[body]
{ auto me = new ExprLambda(CUR_POS, state->validateFormals($formal_set), $body);
{
state->validateFormals($formal_set);
auto me = new ExprLambda(state->alloc, CUR_POS, std::move($formal_set), $body);
$$ = me;
SET_DOC_POS(me, @1);
}
| formal_set '@' ID ':' expr_function[body]
{
auto arg = state->symbols.create($ID);
auto me = new ExprLambda(CUR_POS, arg, state->validateFormals($formal_set, CUR_POS, arg), $body);
state->validateFormals($formal_set, CUR_POS, arg);
auto me = new ExprLambda(state->alloc, CUR_POS, arg, std::move($formal_set), $body);
$$ = me;
SET_DOC_POS(me, @1);
}
| ID '@' formal_set ':' expr_function[body]
{
auto arg = state->symbols.create($ID);
auto me = new ExprLambda(CUR_POS, arg, state->validateFormals($formal_set, CUR_POS, arg), $body);
state->validateFormals($formal_set, CUR_POS, arg);
auto me = new ExprLambda(state->alloc, CUR_POS, arg, std::move($formal_set), $body);
$$ = me;
SET_DOC_POS(me, @1);
}
Expand Down Expand Up @@ -490,18 +494,18 @@ expr_list
;

formal_set
: '{' formals ',' ELLIPSIS '}' { $$ = $formals; $$->ellipsis = true; }
| '{' ELLIPSIS '}' { $$ = new Formals; $$->ellipsis = true; }
| '{' formals ',' '}' { $$ = $formals; $$->ellipsis = false; }
| '{' formals '}' { $$ = $formals; $$->ellipsis = false; }
| '{' '}' { $$ = new Formals; $$->ellipsis = false; }
: '{' formals ',' ELLIPSIS '}' { $$ = std::move($formals); $$.ellipsis = true; }
| '{' ELLIPSIS '}' { $$.ellipsis = true; }
| '{' formals ',' '}' { $$ = std::move($formals); $$.ellipsis = false; }
| '{' formals '}' { $$ = std::move($formals); $$.ellipsis = false; }
| '{' '}' { $$.ellipsis = false; }
;

formals
: formals[accum] ',' formal
{ $$ = $accum; $$->formals.emplace_back(std::move($formal)); }
{ $$ = std::move($accum); $$.formals.emplace_back(std::move($formal)); }
| formal
{ $$ = new Formals; $$->formals.emplace_back(std::move($formal)); }
{ $$.formals.emplace_back(std::move($formal)); }
;

formal
Expand Down
4 changes: 2 additions & 2 deletions src/libexpr/primops.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3363,12 +3363,12 @@ static void prim_functionArgs(EvalState & state, const PosIdx pos, Value ** args
if (!args[0]->isLambda())
state.error<TypeError>("'functionArgs' requires a function").atPos(pos).debugThrow();

if (!args[0]->lambda().fun->hasFormals()) {
if (!args[0]->lambda().fun->hasFormals) {
v.mkAttrs(&Bindings::emptyBindings);
return;
}

const auto & formals = args[0]->lambda().fun->formals->formals;
const auto & formals = args[0]->lambda().fun->getFormals();
auto attrs = state.buildBindings(formals.size());
for (auto & i : formals)
attrs.insert(i.name, state.getBool(i.def), i.pos);
Expand Down
6 changes: 3 additions & 3 deletions src/libexpr/value-to-xml.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,14 @@ static void printValueAsXML(
posToXML(state, xmlAttrs, state.positions[v.lambda().fun->pos]);
XMLOpenElement _(doc, "function", xmlAttrs);

if (v.lambda().fun->hasFormals()) {
if (v.lambda().fun->hasFormals) {
XMLAttrs attrs;
if (v.lambda().fun->arg)
attrs["name"] = state.symbols[v.lambda().fun->arg];
if (v.lambda().fun->formals->ellipsis)
if (v.lambda().fun->ellipsis)
attrs["ellipsis"] = "1";
XMLOpenElement _(doc, "attrspat", attrs);
for (auto & i : v.lambda().fun->formals->lexicographicOrder(state.symbols))
for (auto & i : v.lambda().fun->getFormalsLexicographic(state.symbols))
doc.writeEmptyElement("attr", singletonAttrs("name", state.symbols[i.name]));
} else
doc.writeEmptyElement("varpat", singletonAttrs("name", state.symbols[v.lambda().fun->arg]));
Expand Down
Loading
Loading