Refactoring for private Value type #4355

infinisil · 2020-12-12T01:59:15Z

This is a relatively simple but large refactoring for making Value::type private. This is done because that field exposes implementation details such as:

Different kinds of lists: tList1, tList2, tListN
Different kinds of thunks: tThunk, tApp and tBlackhole
Different kinds of functions: tLambda, tPrimOp and tPrimOpApp

While lists already have a generic interface (Value::isList(), Value::listElems() and Value::listSize()), the same isn't true for thunks and functions. So in order to minimize the necessary changes, new functions were introduced to be able to distinguish between kinds of thunks and functions:

Value::isThunk(), Value::isApp() and Value::isBlackhole()
Value::isLambda(), Value::isPrimOp() and Value::isPrimOpApp()

In addition, a new type for the normal form type of a value was introduced, NormalType, which can be determined using Value::normalType(). This returns the same types as builtins.typeOf does, plus nThunk for thunk values. With this refactoring, this type is the canonical way to check the result type of an evaluation (plus nThunk for when it hasn't been evaluated yet).

And finally, in order to set the underlying type of a Value, setters for all the ValueType's are introduced. In the future this should probably be handled only by Value::mk<Type> functions, such that these setters can be removed.

This PR will be useful for #4090, as such a feature will introduce a new attribute set type and will need a generic interface for accessing them.

Best review commit-by-commit

infinisil · 2020-12-12T02:10:07Z

src/libexpr/eval.cc

+        case tThunk: return "a thunk";
+        case tApp: return "a function application";
+        case tBlackhole: return "a black hole";
    default:
-        return showType(v.type);
+        return showType(v.normalType());


Regarding showType, the function showType(ValueType) was removed and replaced with showType(NormalType), because it's not really possible to get a ValueType anymore. Since showType(ValueType) can't know about the different kinds of thunks (but actually it's not even used on thunks ever), the showType(ValueType) function now handles the different kinds of thunks before passing the normal type to showType(NormalType)

infinisil · 2020-12-12T02:12:13Z

src/libexpr/eval.cc

@@ -1575,36 +1573,36 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
           since paths are copied when they are used in a derivation),
           and none of the strings are allowed to have contexts. */
        if (first) {
-            firstType = vTmp.type;
+            firstType = vTmp.normalType();


The ExprConcatStrings::eval function is an interesting place where the new normal form type has to be used to ensure it keeps working the same way

infinisil · 2020-12-12T02:18:52Z

src/libexpr/value-to-xml.cc

+        case nFunction: {
+            if (!v.isLambda()) {
+                // TODO: Serialize primops and other functions
+                doc.writeEmptyElement("unevaluated");


This NormalType change actually discovered a small bug in the xml conversion: it prints unevaluated for tPrimOp and tPrimOpApp's. I didn't want to bother changing the format of that so I left it.

infinisil · 2020-12-12T02:19:48Z

src/libexpr/value.hh

    ValueType type;
+
+friend std::string showType(const Value & v);
+friend void printValue(std::ostream & str, std::set<const Value *> & active, const Value & v);


These function seemed best to make friends with for simplicity

This will be useful to abstract over the ValueType implementation details Make use of it already to replace the showType(ValueType) function

This is an implementation detail and shouldn't be used. Use normalType() and the various is<Type> functions instead

edolstra · 2020-12-14T10:34:17Z

src/libexpr/eval-inline.hh

+            v.setThunk();
            v.thunk.env = env;
            v.thunk.expr = expr;


If we're refactoring this stuff anyway, then it's better to have v.setThunk() take arguments to initialize the value properly, i.e.

v.mkThunk(env, expr);

(Also mkThunk instead of setThunk for consistency with all the existing mk... functions.)

Hiding the initialisation of thunks and other types is a good thing anyway if we ever want to revive the parallel evaluator, since (IIRC) it required fields to be set in a particular order. E.g. when transitioning from tBlackhole -> tString, you have to set the s field before setting type to tString, since otherwise another thread might think the value is initialized.

Sounds good, I'll create v.mk<Type> methods. I can also remove the global mkInt(Value & v, NixInt n) functions (though e.g. EvalState::mkAttrs should stay since it initializes memory)

I left most static mk* functions around since I don't have motivation to refactor this as well. But this could easily be done later.

edolstra · 2020-12-14T10:35:46Z

src/libexpr/value.hh

+    inline void setList1() { type = tList1; };
+    inline void setList2() { type = tList2; };
+    inline void setListN() { type = tListN; };


These setters break the abstraction of having a single list type. They should be unnecessary if mkList is a friend or member of Value.

These are now replaced with Value::mkList(size_t)

src/libexpr/attr-path.cc

edolstra · 2020-12-14T10:43:28Z

src/libexpr/value.hh

+// grouping together implementation details like tList*, different function
+// types, and types in non-normal form (so thunks and co.)
+typedef enum {
+    nThunk,


NormalType might be a bit of a confusing name since thunks are not normal forms. Maybe AbstractType?

How about this: We make ValueType type be an unnamed enum with

enum { tInt = 1, // ... } type;

And then let's use the ValueType name for the current NormalType.

Then we should probably also use Value::type() instead of Value::normalType()

Yeah that sounds good.

Wasn't able to easily use an anonymous enum because the showType and printValue friends wouldn't have access to the variants like that. Instead I renamed the original ValueType to InternalType, so that NormalType can be ValueType instead. Also renamed the field to Value::internalType which is needed for Value::type() to be a function.

And Value::type to Value::internalType, such that type() can be used in the next commit to get the new ValueType

Move clearValue inside Value mkInt instead of setInt mkBool instead of setBool mkString instead of setString mkPath instead of setPath mkNull instead of setNull mkAttrs instead of setAttrs mkList instead of setList* mkThunk instead of setThunk mkApp instead of setApp mkLambda instead of setLambda mkBlackhole instead of setBlackhole mkPrimOp instead of setPrimOp mkPrimOpApp instead of setPrimOpApp mkExternal instead of setExternal mkFloat instead of setFloat Add note that the static mk* function should be removed eventually

infinisil · 2020-12-18T20:51:56Z

Addressed all comments.

Note that this will be a backwards incompatible change for Nix plugins, which now instead of setting type = tExternal or another type have to use the Value::mk* functions. But since these are rarely used, I don't think that's a problem.

@edolstra

This updates hydra to be compatible with Nix NixOS/nix#4355. Along with NixOS#840 needed for NixOS/nixpkgs#107909 /cc @edolstra

nixos-discourse · 2021-01-26T09:17:58Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-6/11195/1

nixos-discourse · 2021-07-08T05:39:49Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-15/13975/1

infinisil force-pushed the private-value-type branch from 527d65d to 597b369 Compare December 12, 2020 02:09

infinisil commented Dec 12, 2020

View reviewed changes

infinisil added 5 commits December 12, 2020 03:31

Introduce NormalType for the normal type of a Value

fa30787

This will be useful to abstract over the ValueType implementation details Make use of it already to replace the showType(ValueType) function

Introduce Value type setters and make use of them

9f056f7

Use Value::normalType on all forced values instead of Value::type

22ead43

Add ValueType checking functions for types that have the same NormalType

bf98903

Make Value::type private

730b152

This is an implementation detail and shouldn't be used. Use normalType() and the various is<Type> functions instead

infinisil force-pushed the private-value-type branch from 597b369 to 730b152 Compare December 12, 2020 02:32

edolstra reviewed Dec 14, 2020

View reviewed changes

infinisil added 3 commits December 17, 2020 14:45

Rename ValueType -> InternalType, NormalType -> ValueType

d67e029

And Value::type to Value::internalType, such that type() can be used in the next commit to get the new ValueType

Rename Value::normalType() -> Value::type()

12e6507

infinisil force-pushed the private-value-type branch from a7cfcd8 to b70d22b Compare December 18, 2020 20:49

edolstra merged commit ec4a5c5 into NixOS:master Dec 21, 2020

infinisil deleted the private-value-type branch December 21, 2020 14:36

matthewbauer added a commit to matthewbauer/hydra that referenced this pull request Dec 29, 2020

Update for Nix Value::type API changes

d38ef44

This updates hydra to be compatible with Nix NixOS/nix#4355. Along with NixOS#840 needed for NixOS/nixpkgs#107909 /cc @edolstra

matthewbauer mentioned this pull request Dec 29, 2020

Update for Nix Value::type API changes NixOS/hydra#843

Merged

ldesgoui mentioned this pull request Aug 1, 2021

Write Nix 2.4 release notes #4521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring for private Value type #4355

Refactoring for private Value type #4355

infinisil commented Dec 12, 2020 •

edited

infinisil Dec 12, 2020 •

edited

infinisil Dec 12, 2020

infinisil Dec 12, 2020

infinisil Dec 12, 2020

edolstra Dec 14, 2020

edolstra Dec 14, 2020

infinisil Dec 14, 2020

infinisil Dec 18, 2020

edolstra Dec 14, 2020

infinisil Dec 18, 2020

edolstra Dec 14, 2020

infinisil Dec 14, 2020 •

edited

infinisil Dec 14, 2020

edolstra Dec 14, 2020

infinisil Dec 18, 2020

infinisil commented Dec 18, 2020 •

edited

nixos-discourse commented Jan 26, 2021

nixos-discourse commented Jul 8, 2021

Refactoring for private Value type #4355

Refactoring for private Value type #4355

Conversation

infinisil commented Dec 12, 2020 • edited

infinisil Dec 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil Dec 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Dec 18, 2020 • edited

nixos-discourse commented Jan 26, 2021

nixos-discourse commented Jul 8, 2021

infinisil commented Dec 12, 2020 •

edited

infinisil Dec 12, 2020 •

edited

infinisil Dec 14, 2020 •

edited

infinisil commented Dec 18, 2020 •

edited