libexpr/flakes: Replace the AST checks with maxCallDepth = 0

Change-Id: I7130cc941b4b7432df76a3995d298edfb28e6a01

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.

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<ExprAttrs>()) {
-        state.ctx.errors.make<EvalError>("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<EvalError>("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);

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<ExprSet>()
-                && static_cast<ExprSet *>(thunk().expr)->dynamicAttrs.empty())
-            || thunk().expr->try_cast<ExprLambda>() || thunk().expr->try_cast<ExprList>());
-}
-
 Value::Value(string_t, Str * s, const NixStringContext & context)
     : Value(NewValueAs::string, s, copyContext(context))
 {

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

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

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
+    )