Skip to content
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.

Sign up for GitHub

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

Make TypeError structured #10199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make TypeError structured #10199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions src/libexpr/attr-path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,18 @@ std::pair<Value *, PosIdx> findAlongAttrPath(EvalState & state, const std::strin
Value * vNew = state.allocValue();
state.autoCallFunction(autoArgs, *v, *vNew);
v = vNew;
state.forceValue(*v, noPos);
state.forceValue(*v, pos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? FWIW it came from c1ca4f0


/* It should evaluate to either a set or an expression,
according to what is specified in the attrPath. */

if (!attrIndex) {

if (v->type() != nAttrs)
state.error<TypeError>(
"the expression selected by the selection path '%1%' should be a set but is %2%",
attrPath,
showType(*v)).debugThrow();
if (v->type() != nAttrs) {
state.error<TypeError>(nAttrs, *v)
.addTrace(pos, HintFmt("while evaluating the selection path %1%", attrPath))
.debugThrow();
}
if (attr.empty())
throw Error("empty attribute name in selection path '%1%'", attrPath);

Expand All @@ -88,10 +88,9 @@ std::pair<Value *, PosIdx> findAlongAttrPath(EvalState & state, const std::strin
else {

if (!v->isList())
state.error<TypeError>(
"the expression selected by the selection path '%1%' should be a list but is %2%",
attrPath,
showType(*v)).debugThrow();
state.error<TypeError>(nList, *v)
.addTrace(pos, HintFmt("while evaluating the selection path %1%", attrPath))
.debugThrow();
if (*attrIndex >= v->listSize())
throw AttrPathNotFound("list index %1% in selection path '%2%' is out of range", *attrIndex, attrPath);

Expand Down
29 changes: 15 additions & 14 deletions src/libexpr/eval-cache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,15 +500,15 @@ std::shared_ptr<AttrCursor> AttrCursor::maybeGetAttr(Symbol name, bool forceErro
// evaluate to see whether 'name' exists
} else
return nullptr;
//error<TypeError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
//error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
//error<TypeError>("nAttrs, getAttrPathStr()).debugThrow();

Is this better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess getAttrPathStr is not the value, however

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, unless we are sure we never catch a TypeError, I am a bit wary of changing this

}
}

auto & v = forceValue();

if (v.type() != nAttrs)
return nullptr;
//error<TypeError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
//error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved

auto attr = v.attrs->get(name);

Expand Down Expand Up @@ -574,14 +574,15 @@ std::string AttrCursor::getString()
debug("using cached string attribute '%s'", getAttrPathStr());
return s->first;
} else
root->state.error<TypeError>("'%s' is not a string", getAttrPathStr()).debugThrow();
// TODO: Is this an internal error? Where is literally any documentation for the eval cache?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There probably is no documentation :(

From seeing how this is used, I think this cursor thing can either just traverse a regular "uncached" value, or traverse a cache entry's serialized value. The getString part is just the caller asking for a specific type; it's not an internal error.

root->state.error<EvalError>("'%s' is not a string", getAttrPathStr()).debugThrow();
}
}

auto & v = forceValue();

if (v.type() != nString && v.type() != nPath)
root->state.error<TypeError>("'%s' is not a string but %s", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not a string but %s", getAttrPathStr()).debugThrow();

return v.type() == nString ? v.c_str() : v.path().to_string();
}
Expand Down Expand Up @@ -616,7 +617,7 @@ string_t AttrCursor::getStringWithContext()
return *s;
}
} else
root->state.error<TypeError>("'%s' is not a string", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not a string", getAttrPathStr()).debugThrow();
}
}

Expand All @@ -630,7 +631,7 @@ string_t AttrCursor::getStringWithContext()
else if (v.type() == nPath)
return {v.path().to_string(), {}};
else
root->state.error<TypeError>("'%s' is not a string but %s", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not a string but %s", getAttrPathStr()).debugThrow();
}

bool AttrCursor::getBool()
Expand All @@ -643,14 +644,14 @@ bool AttrCursor::getBool()
debug("using cached Boolean attribute '%s'", getAttrPathStr());
return *b;
} else
root->state.error<TypeError>("'%s' is not a Boolean", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not a Boolean", getAttrPathStr()).debugThrow();
}
}

auto & v = forceValue();

if (v.type() != nBool)
root->state.error<TypeError>("'%s' is not a Boolean", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not a Boolean", getAttrPathStr()).debugThrow();

return v.boolean;
}
Expand All @@ -665,14 +666,14 @@ NixInt AttrCursor::getInt()
debug("using cached integer attribute '%s'", getAttrPathStr());
return i->x;
} else
root->state.error<TypeError>("'%s' is not an integer", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not an integer", getAttrPathStr()).debugThrow();
}
}

auto & v = forceValue();

if (v.type() != nInt)
root->state.error<TypeError>("'%s' is not an integer", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not an integer", getAttrPathStr()).debugThrow();

return v.integer;
}
Expand All @@ -687,7 +688,7 @@ std::vector<std::string> AttrCursor::getListOfStrings()
debug("using cached list of strings attribute '%s'", getAttrPathStr());
return *l;
} else
root->state.error<TypeError>("'%s' is not a list of strings", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not a list of strings", getAttrPathStr()).debugThrow();
}
}

Expand All @@ -697,7 +698,7 @@ std::vector<std::string> AttrCursor::getListOfStrings()
root->state.forceValue(v, noPos);

if (v.type() != nList)
root->state.error<TypeError>("'%s' is not a list", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not a list", getAttrPathStr()).debugThrow();

std::vector<std::string> res;

Expand All @@ -720,14 +721,14 @@ std::vector<Symbol> AttrCursor::getAttrs()
debug("using cached attrset attribute '%s'", getAttrPathStr());
return *attrs;
} else
root->state.error<TypeError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
}
}

auto & v = forceValue();

if (v.type() != nAttrs)
root->state.error<TypeError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
root->state.error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();

std::vector<Symbol> attrs;
for (auto & attr : *getValue().attrs)
Expand Down
13 changes: 13 additions & 0 deletions src/libexpr/eval-error.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,18 @@ template class EvalErrorBuilder<MissingArgumentError>;
template class EvalErrorBuilder<InfiniteRecursionError>;
template class EvalErrorBuilder<CachedEvalError>;
template class EvalErrorBuilder<InvalidPathError>;
template class EvalErrorBuilder<MissingAttrError>;


MissingAttrError::MissingAttrError(EvalState & state, Symbol attr, Value & actual)
: EvalError(
state,
"attribute '%1%' missing: %2%",
state.symbols[attr],
ValuePrinter(state, actual, errorPrintOptions))
, attr(attr)
, actual(actual)
{
}

}
36 changes: 32 additions & 4 deletions src/libexpr/eval-error.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

#include "error.hh"
#include "pos-idx.hh"
#include "print-options.hh"
#include "print.hh"
#include "value.hh"

namespace nix {

Expand Down Expand Up @@ -40,12 +43,39 @@ MakeError(ParseError, Error);
MakeError(AssertionError, EvalError);
MakeError(ThrownError, AssertionError);
MakeError(Abort, EvalError);
MakeError(TypeError, EvalError);
MakeError(UndefinedVarError, EvalError);
MakeError(MissingArgumentError, EvalError);
MakeError(CachedEvalError, EvalError);
MakeError(InfiniteRecursionError, EvalError);

std::string_view showType(ValueType type, bool article);

Comment on lines +51 to +52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string_view showType(ValueType type, bool article);

It's in value.hh right?

struct TypeError : public EvalError
{
ValueType expected;
Value & actual;

TypeError(EvalState & state, ValueType expected, Value & actual)
: EvalError(
state,
"expected %1% but found %2%: %3%",
showType(expected),
showType(actual),
ValuePrinter(state, actual, errorPrintOptions))
, expected(expected)
, actual(actual)
{
}
};

struct MissingAttrError : public EvalError
{
Symbol attr;
Value & actual;

MissingAttrError(EvalState & state, Symbol attr, Value & actual);
};

struct InvalidPathError : public EvalError
{
public:
Expand All @@ -67,7 +97,7 @@ class EvalErrorBuilder final
friend class EvalState;

template<typename... Args>
explicit EvalErrorBuilder(EvalState & state, const Args &... args)
explicit EvalErrorBuilder(EvalState & state, Args && ... args)
: error(T(state, args...))
{
}
Expand All @@ -83,8 +113,6 @@ public:

[[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withTrace(PosIdx pos, const std::string_view text);

[[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withFrameTrace(PosIdx pos, const std::string_view text);

Comment on lines -86 to -87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated cleanup?

[[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withSuggestions(Suggestions & s);

[[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withFrame(const Env & e, const Expr & ex);
Expand Down
12 changes: 2 additions & 10 deletions src/libexpr/eval-inline.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,7 @@ inline void EvalState::forceAttrs(Value & v, Callable getPos, std::string_view e
PosIdx pos = getPos();
forceValue(v, pos);
if (v.type() != nAttrs) {
error<TypeError>(
"expected a set but found %1%: %2%",
showType(v),
ValuePrinter(*this, v, errorPrintOptions)
).withTrace(pos, errorCtx).debugThrow();
error<TypeError>(nAttrs, v).withTrace(pos, errorCtx).debugThrow();
}
}

Expand All @@ -130,11 +126,7 @@ inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view e
{
forceValue(v, pos);
if (!v.isList()) {
error<TypeError>(
"expected a list but found %1%: %2%",
showType(v),
ValuePrinter(*this, v, errorPrintOptions)
).withTrace(pos, errorCtx).debugThrow();
error<TypeError>(nList, v).withTrace(pos, errorCtx).debugThrow();
}
}

Expand Down
Loading
Loading