-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Refactoring for private Value type #4355
Conversation
527d65d
to
597b369
Compare
src/libexpr/eval.cc
Outdated
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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
src/libexpr/eval.cc
Outdated
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
case nFunction: { | ||
if (!v.isLambda()) { | ||
// TODO: Serialize primops and other functions | ||
doc.writeEmptyElement("unevaluated"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ValueType type; | ||
|
||
friend std::string showType(const Value & v); | ||
friend void printValue(std::ostream & str, std::set<const Value *> & active, const Value & v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
597b369
to
730b152
Compare
src/libexpr/eval-inline.hh
Outdated
v.setThunk(); | ||
v.thunk.env = env; | ||
v.thunk.expr = expr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/libexpr/value.hh
Outdated
inline void setList1() { type = tList1; }; | ||
inline void setList2() { type = tList2; }; | ||
inline void setListN() { type = tListN; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These setters break the abstraction of having a single list type. They should be unnecessary if mkList
is a friend or member of Value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are now replaced with Value::mkList(size_t)
// grouping together implementation details like tList*, different function | ||
// types, and types in non-normal form (so thunks and co.) | ||
typedef enum { | ||
nThunk, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NormalType
might be a bit of a confusing name since thunks are not normal forms. Maybe AbstractType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should probably also use Value::type()
instead of Value::normalType()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
a7cfcd8
to
b70d22b
Compare
Addressed all comments. Note that this will be a backwards incompatible change for Nix plugins, which now instead of setting |
This updates hydra to be compatible with Nix NixOS/nix#4355. Along with NixOS#840 needed for NixOS/nixpkgs#107909 /cc @edolstra
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 |
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 |
This is a relatively simple but large refactoring for making
Value::type
private. This is done because that field exposes implementation details such as:tList1
,tList2
,tListN
tThunk
,tApp
andtBlackhole
tLambda
,tPrimOp
andtPrimOpApp
While lists already have a generic interface (
Value::isList()
,Value::listElems()
andValue::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()
andValue::isBlackhole()
Value::isLambda()
,Value::isPrimOp()
andValue::isPrimOpApp()
In addition, a new type for the normal form type of a value was introduced,
NormalType
, which can be determined usingValue::normalType()
. This returns the same types asbuiltins.typeOf
does, plusnThunk
for thunk values. With this refactoring, this type is the canonical way to check the result type of an evaluation (plusnThunk
for when it hasn't been evaluated yet).And finally, in order to set the underlying
type
of aValue
, setters for all theValueType
's are introduced. In the future this should probably be handled only byValue::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