From 6068e32aa790fc53e7123d8442282714af45770b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 15 Aug 2024 11:36:44 +0200 Subject: [PATCH 1/3] refactor: Extract EvalState::addCallDepth --- src/libexpr/eval-inline.hh | 8 ++++++++ src/libexpr/eval.cc | 19 +------------------ src/libexpr/eval.hh | 20 ++++++++++++++++++++ 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 6fa34b062..d5ce238b2 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -4,6 +4,7 @@ #include "print.hh" #include "eval.hh" #include "eval-error.hh" +#include "eval-settings.hh" namespace nix { @@ -138,5 +139,12 @@ inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view e } } +[[gnu::always_inline]] +inline CallDepth EvalState::addCallDepth(const PosIdx pos) { + if (callDepth > settings.maxCallDepth) + error("stack overflow; max-call-depth exceeded").atPos(pos).debugThrow(); + + return CallDepth(callDepth); +}; } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index c9101678c..0e70ca262 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1471,26 +1471,9 @@ void ExprLambda::eval(EvalState & state, Env & env, Value & v) v.mkLambda(&env, this); } -namespace { -/** Increments a count on construction and decrements on destruction. - */ -class CallDepth { - size_t & count; -public: - CallDepth(size_t & count) : count(count) { - ++count; - } - ~CallDepth() { - --count; - } -}; -}; - void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & vRes, const PosIdx pos) { - if (callDepth > settings.maxCallDepth) - error("stack overflow; max-call-depth exceeded").atPos(pos).debugThrow(); - CallDepth _level(callDepth); + auto _level = addCallDepth(pos); auto trace = settings.traceFunctionCalls ? std::make_unique(positions[pos]) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index ddf5dcf94..9fd31e904 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -41,6 +41,21 @@ namespace eval_cache { class EvalCache; } +/** + * Increments a count on construction and decrements on destruction. + */ +class CallDepth { + size_t & count; + +public: + CallDepth(size_t & count) : count(count) { + ++count; + } + ~CallDepth() { + --count; + } +}; + /** * Function that implements a primop. */ @@ -649,6 +664,11 @@ private: public: + /** + * Check that the call depth is within limits, and increment it, until the returned object is destroyed. + */ + inline CallDepth addCallDepth(const PosIdx pos); + /** * Do a deep equality test between two values. That is, list * elements and attributes are compared recursively. From 72a4d1f52d46f1472b79a04d3af2dee194f13d52 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 15 Aug 2024 12:29:59 +0200 Subject: [PATCH 2/3] Add :doc support for __functor --- src/libexpr/eval.cc | 14 +++ src/libexpr/eval.hh | 6 ++ tests/functional/repl/doc-functor.expected | 101 +++++++++++++++++++++ tests/functional/repl/doc-functor.in | 10 ++ tests/functional/repl/doc-functor.nix | 101 +++++++++++++++++++++ 5 files changed, 232 insertions(+) create mode 100644 tests/functional/repl/doc-functor.expected create mode 100644 tests/functional/repl/doc-functor.in create mode 100644 tests/functional/repl/doc-functor.nix diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 0e70ca262..b87d96be0 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -616,6 +616,20 @@ std::optional EvalState::getDoc(Value & v) strdup(ss.data()), }; } + if (isFunctor(v)) { + try { + Value & functor = *v.attrs()->find(sFunctor)->value; + Value * vp = &v; + Value partiallyApplied; + callFunction(functor, 1, &vp, partiallyApplied, noPos); + auto _level = addCallDepth(noPos); + return getDoc(partiallyApplied); + } + catch (Error & e) { + e.addTrace(nullptr, "while partially calling '%1%' to retrieve documentation", "__functor"); + throw; + } + } return {}; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 9fd31e904..da9dd2087 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -640,6 +640,12 @@ public: const char * doc; }; + /** + * Retrieve the documentation for a value. This will evaluate the value if + * it is a thunk, and it will partially apply __functor if applicable. + * + * @param v The value to get the documentation for. + */ std::optional getDoc(Value & v); private: diff --git a/tests/functional/repl/doc-functor.expected b/tests/functional/repl/doc-functor.expected new file mode 100644 index 000000000..8cb2706ef --- /dev/null +++ b/tests/functional/repl/doc-functor.expected @@ -0,0 +1,101 @@ +Nix +Type :? for help. + +nix-repl> :l doc-functor.nix +Added variables. + +nix-repl> :doc multiplier +Function `__functor`\ + … defined at /path/to/tests/functional/repl/doc-functor.nix:12:23 + + +Multiply the argument by the factor stored in the factor attribute. + +nix-repl> :doc doubler +Function `multiply`\ + … defined at /path/to/tests/functional/repl/doc-functor.nix:5:17 + + +Look, it's just like a function! + +nix-repl> :doc recursive +Function `__functor`\ + … defined at /path/to/tests/functional/repl/doc-functor.nix:77:23 + + +This looks bad, but the docs are ok because of the eta expansion. + +nix-repl> :doc recursive2 +error: + … while partially calling '__functor' to retrieve documentation + + … while calling '__functor' + at /path/to/tests/functional/repl/doc-functor.nix:85:17: + 84| */ + 85| __functor = self: self.__functor self; + | ^ + 86| }; + + … from call site + at /path/to/tests/functional/repl/doc-functor.nix:85:23: + 84| */ + 85| __functor = self: self.__functor self; + | ^ + 86| }; + + (19999 duplicate frames omitted) + + error: stack overflow; max-call-depth exceeded + at /path/to/tests/functional/repl/doc-functor.nix:85:23: + 84| */ + 85| __functor = self: self.__functor self; + | ^ + 86| }; + +nix-repl> :doc diverging +error: + … while partially calling '__functor' to retrieve documentation + + (10000 duplicate frames omitted) + + … while calling '__functor' + at /path/to/tests/functional/repl/doc-functor.nix:97:19: + 96| f = x: { + 97| __functor = self: (f (x + 1)); + | ^ + 98| }; + + error: stack overflow; max-call-depth exceeded + at /path/to/tests/functional/repl/doc-functor.nix:97:26: + 96| f = x: { + 97| __functor = self: (f (x + 1)); + | ^ + 98| }; + +nix-repl> :doc helper +Function `square`\ + … defined at /path/to/tests/functional/repl/doc-functor.nix:36:12 + + +Compute x^2 + +nix-repl> :doc helper2 +Function `__functor`\ + … defined at /path/to/tests/functional/repl/doc-functor.nix:45:23 + + +This is a function that can be overridden. + +nix-repl> :doc lib.helper3 +Function `__functor`\ + … defined at /path/to/tests/functional/repl/doc-functor.nix:45:23 + + +This is a function that can be overridden. + +nix-repl> :doc helper3 +Function `__functor`\ + … defined at /path/to/tests/functional/repl/doc-functor.nix:45:23 + + +This is a function that can be overridden. diff --git a/tests/functional/repl/doc-functor.in b/tests/functional/repl/doc-functor.in new file mode 100644 index 000000000..d2bb57a02 --- /dev/null +++ b/tests/functional/repl/doc-functor.in @@ -0,0 +1,10 @@ +:l doc-functor.nix +:doc multiplier +:doc doubler +:doc recursive +:doc recursive2 +:doc diverging +:doc helper +:doc helper2 +:doc lib.helper3 +:doc helper3 diff --git a/tests/functional/repl/doc-functor.nix b/tests/functional/repl/doc-functor.nix new file mode 100644 index 000000000..f526f453f --- /dev/null +++ b/tests/functional/repl/doc-functor.nix @@ -0,0 +1,101 @@ +rec { + /** + Look, it's just like a function! + */ + multiply = p: q: p * q; + + multiplier = { + factor = 2; + /** + Multiply the argument by the factor stored in the factor attribute. + */ + __functor = self: x: x * self.factor; + }; + + doubler = { + description = "bla"; + /** + Multiply by two. This doc probably won't be rendered because the + returned partial application won't have any reference to this location; + only pointing to the second lambda in the multiply function. + */ + __functor = self: multiply 2; + }; + + makeOverridable = f: { + /** + This is a function that can be overridden. + */ + __functor = self: f; + override = throw "not implemented"; + }; + + /** + Compute x^2 + */ + square = x: x * x; + + helper = makeOverridable square; + + # Somewhat analogous to the Nixpkgs makeOverridable function. + makeVeryOverridable = f: { + /** + This is a function that can be overridden. + */ + __functor = self: arg: f arg // { override = throw "not implemented"; overrideAttrs = throw "not implemented"; }; + override = throw "not implemented"; + }; + + helper2 = makeVeryOverridable square; + + # The RFC might be ambiguous here. The doc comment from makeVeryOverridable + # is "inner" in terms of values, but not inner in terms of expressions. + # Returning the following attribute comment might be allowed. + # TODO: I suppose we could look whether the attribute value expression + # contains a doc, and if not, return the attribute comment anyway? + + /** + Compute x^3 + */ + lib.helper3 = makeVeryOverridable (x: x * x * x); + + /** + Compute x^3... + */ + helper3 = makeVeryOverridable (x: x * x * x); + + + # ------ + + # getDoc traverses a potentially infinite structure in case of __functor, so + # we need to test with recursive inputs and diverging inputs. + + recursive = { + /** + This looks bad, but the docs are ok because of the eta expansion. + */ + __functor = self: x: self x; + }; + + recursive2 = { + /** + Docs probably won't work in this case, because the "partial" application + of self results in an infinite recursion. + */ + __functor = self: self.__functor self; + }; + + diverging = let + /** + Docs probably won't work in this case, because the "partial" application + of self results in an diverging computation that causes a stack overflow. + It's not an infinite recursion because each call is different. + This must be handled by the documentation retrieval logic, as it + reimplements the __functor invocation to be partial. + */ + f = x: { + __functor = self: (f (x + 1)); + }; + in f null; + +} From 77ddcbe12e169051f9bad1ab5e7581b148a94883 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 26 Aug 2024 16:15:13 +0200 Subject: [PATCH 3/3] getDoc: Explain why we partially apply __functor --- src/libexpr/eval.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index b87d96be0..ca41a9944 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -621,6 +621,11 @@ std::optional EvalState::getDoc(Value & v) Value & functor = *v.attrs()->find(sFunctor)->value; Value * vp = &v; Value partiallyApplied; + // The first paramater is not user-provided, and may be + // handled by code that is opaque to the user, like lib.const = x: y: y; + // So preferably we show docs that are relevant to the + // "partially applied" function returned by e.g. `const`. + // We apply the first argument: callFunction(functor, 1, &vp, partiallyApplied, noPos); auto _level = addCallDepth(noPos); return getDoc(partiallyApplied);