From 481fc30ae7b45d82b41489ae844adae5d8023988 Mon Sep 17 00:00:00 2001 From: piegames Date: Thu, 30 Apr 2026 16:51:23 +0200 Subject: [PATCH] libexpr/flakes: Replace the AST checks with maxCallDepth = 0 Change-Id: I7130cc941b4b7432df76a3995d298edfb28e6a01 --- doc/manual/rl-next/trivial-flakes.md | 18 +++++ lix/libexpr/flake/flake.cc | 29 ++++++-- lix/libexpr/value.cc | 9 --- lix/libexpr/value.hh | 7 -- pyproject.toml | 2 +- tests/functional2/flakes/test_trivial.py | 89 ++++++++++++++++++++++++ 6 files changed, 131 insertions(+), 23 deletions(-) create mode 100644 doc/manual/rl-next/trivial-flakes.md create mode 100644 tests/functional2/flakes/test_trivial.py diff --git a/doc/manual/rl-next/trivial-flakes.md b/doc/manual/rl-next/trivial-flakes.md new file mode 100644 index 000000000..ac9dc142b --- /dev/null +++ b/doc/manual/rl-next/trivial-flakes.md @@ -0,0 +1,18 @@ +--- +synopsis: "Changes to `flake.nix` validation" +cls: [5523] +category: "Breaking Changes" +credits: [piegames, Qyriad, horrors] +issues: [gh#4945] +--- + +Flakes try to keep their inputs and metadata "simple", to make sure no unbounded computation may happen when calling e.g. `nix flake show`. +Those checks were haphazard, a maintenance burden, and also easily circumventable. + +Lix has now replaced all the old checks by a simple rule: **No function calls outside of `outputs`.** +This is easier to reason about than the previous set of inconsistent rules, and crucially now also allows syntax features that users felt like they *should* have worked in the past, like let bindings. +However, some warts still remain for now: Some syntax constructs like `-1` internally desugar to `__sub 0 1`, which is a function call and thus remains forbidden. +This will be rectified as soon as the deprecation period of the respective anti-features has been completed. + +This change is **breaking** in the sense that flakes which are written with the newly allowed language features will not evaluate with an older Lix version which still uses the old, more restrictive checks. +Crucially, this also affects **all transitive dependants** of such Flakes. diff --git a/lix/libexpr/flake/flake.cc b/lix/libexpr/flake/flake.cc index 9e3711a48..7cce06889 100644 --- a/lix/libexpr/flake/flake.cc +++ b/lix/libexpr/flake/flake.cc @@ -78,10 +78,21 @@ try { co_return result::current_exception(); } +/** Force a value that cannot contain any function calls */ static void forceTrivialValue(EvalState & state, Value & value, const PosIdx pos) { - if (value.isThunk() && value.isTrivial()) - state.forceValue(value, pos); + /* /piegames sighs at the settings API */ + auto prevOverridden = evalSettings.maxCallDepth.overridden; + auto prevMaxCallDepth = evalSettings.maxCallDepth; + evalSettings.maxCallDepth.override(0); + + /* /piegames sighs at C++ */ + KJ_DEFER({ + evalSettings.maxCallDepth.override(prevMaxCallDepth); + evalSettings.maxCallDepth.overridden = prevOverridden; + }); + + state.forceValue(value, pos); } @@ -327,11 +338,17 @@ static Flake getFlake( Expr & flakeExpr = state.ctx.parseExprFromFile(resolvedFlakeFile); // Enforce that 'flake.nix' is a direct attrset, not a computation. - if (!flakeExpr.try_cast()) { - state.ctx.errors.make("file '%s' must be an attribute set", resolvedFlakeFile).debugThrow(); - } + // We do this by disallowing any function calls (maxCallDepth 0) and checking that the resulting value is + // an attrset. + // The logic is already implemented in `forceTrivialValue`, but that takes a value instead of an Expr, so + // we wrap the expression in a thunk to be able to call it. + Value vInfo = flakeExpr.maybeThunk(state, state.ctx.builtins.env); + forceTrivialValue(state, vInfo, flakeExpr.getPos()); - Value vInfo = state.eval(flakeExpr); + if (vInfo.type() != nAttrs) { + state.ctx.errors.make("file '%s' must be an attribute set", resolvedFlakeFile) + .debugThrow(); + } if (auto description = vInfo.attrs()->get(state.ctx.symbols.sym_description)) { expectType(state, nString, description->value, description->pos); diff --git a/lix/libexpr/value.cc b/lix/libexpr/value.cc index 2a5474bed..8a2d32ff7 100644 --- a/lix/libexpr/value.cc +++ b/lix/libexpr/value.cc @@ -53,15 +53,6 @@ void Value::print(EvalState & state, std::ostream & str, PrintOptions options) printValue(state, str, *this, options); } -bool Value::isTrivial() const -{ - return internalType() != tApp - && (internalType() != tThunk - || (thunk().expr->try_cast() - && static_cast(thunk().expr)->dynamicAttrs.empty()) - || thunk().expr->try_cast() || thunk().expr->try_cast()); -} - Value::Value(string_t, Str * s, const NixStringContext & context) : Value(NewValueAs::string, s, copyContext(context)) { diff --git a/lix/libexpr/value.hh b/lix/libexpr/value.hh index 0e92e08d5..e4a8bf032 100644 --- a/lix/libexpr/value.hh +++ b/lix/libexpr/value.hh @@ -778,13 +778,6 @@ public: size_t listSize() const; - /** - * Check whether forcing this value requires a trivial amount of - * computation. In particular, function applications are - * non-trivial. - */ - bool isTrivial() const; - auto listItems() const { struct ListIterable diff --git a/pyproject.toml b/pyproject.toml index b017e590b..e09dc9d99 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -141,7 +141,7 @@ task-tags = ["TODO", "FIXME", "XXX"] # FURB: covered by other rule sets # TRY: try and raise related things, not helpful as we only do testing select = ["E4", "E7", "E9", "F", "ERA", "ASYNC", "ANN0", "ANN2", "A", "C4", "ISC", "INP", "LOG", "G", "PIE", "T20", "PT", "Q", "RSE", "RET", "SIM", "TID251", "TD", "ARG", "PTH", "N", "PERF", "PLC", "PLE", "UP", "RUF"] -ignore = ["ANN002", "ANN003", "TD001", "TD003", "PLE1", "RUF005"] +ignore = ["ANN002", "ANN003", "TD001", "TD003", "PLE1", "RUF005", "RUF003"] [tool.ruff.lint.per-file-ignores] # ignore open() and os.path.join() calls in test_evil_nars, as that file is working with raw bytes diff --git a/tests/functional2/flakes/test_trivial.py b/tests/functional2/flakes/test_trivial.py new file mode 100644 index 000000000..9d56af6a2 --- /dev/null +++ b/tests/functional2/flakes/test_trivial.py @@ -0,0 +1,89 @@ +from testlib.fixtures.nix import Nix +from testlib.fixtures.file_helper import with_files, File +from pathlib import Path +import pytest + + +@pytest.fixture(autouse=True) +def common_init(nix: Nix): + nix.settings.add_xp_feature("nix-command", "flakes") + + +# Trivial let bindings should work within a flake +@with_files( + { + "flake.nix": File(""" + let + description = "meow"; + in { + inherit description; + inputs = { }; + + outputs = { self }: { }; + } + """) + } +) +def test_trivial_let(nix: Nix, files: Path): + assert nix.nix(["flake", "show", "--json", files]).run().ok().json() == {} + + +# Interpolation is not a function call and thus allowed (it desugars to string concatenation) +@with_files( + { + "dependency": {"flake.nix": File("{ outputs = _: {}; }")}, + "flake.nix": File(""" + let + src = "path:."; + in { + inputs = { + lix.url = "${src}/dependency"; + }; + + outputs = { self, lix }: { + }; + } + """), + } +) +def test_trivial_interpolation(nix: Nix, files: Path): + assert nix.nix(["flake", "show", "--json", files]).run().ok().json() == {} + + +# `-1` desugars to `__sub 0 1` and thus is –stupidly– forbidden. +# Once we have finalized the deprecation of shadowing of internal symbols, we will be able to change subtraction and division +# to use proper AST nodes. Like they should have in the first place. +@with_files( + { + "flake.nix": File(""" + { + inputs = -1; + + outputs = { self, lix }: { }; + } + """) + } +) +def test_trivial_implicit_function(nix: Nix, files: Path): + assert ( + "error: stack overflow" + in nix.nix(["flake", "show", "--json", files]).run().expect(1).stderr_plain + ) + + +@with_files( + { + "flake.nix": File(""" + { + inputs = builtins.seq true { }; + + outputs = { self, lix }: { }; + } + """) + } +) +def test_trivial_explicit_function(nix: Nix, files: Path): + assert ( + "error: stack overflow" + in nix.nix(["flake", "show", "--json", files]).run().expect(1).stderr_plain + )