From c0792b1546ceed1c02a02ca1286ead55f79d5798 Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 3 Mar 2022 10:50:35 +0100 Subject: [PATCH 1/8] Implement a suggestions mechanism Each `Error` class now includes a set of suggestions, and these are printed by the top-level handler. --- src/libcmd/installables.cc | 11 +++- src/libexpr/attr-path.cc | 10 +++- src/libutil/args.cc | 9 ++- src/libutil/error.cc | 7 +++ src/libutil/error.hh | 8 +++ src/libutil/suggestions.cc | 111 +++++++++++++++++++++++++++++++++++++ src/libutil/suggestions.hh | 41 ++++++++++++++ 7 files changed, 191 insertions(+), 6 deletions(-) create mode 100644 src/libutil/suggestions.cc create mode 100644 src/libutil/suggestions.hh diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 3209456bf..888d863ff 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -610,17 +610,24 @@ std::pair InstallableFlake::toValue(EvalState & state) auto emptyArgs = state.allocBindings(0); + Suggestions suggestions; + for (auto & attrPath : getActualAttrPaths()) { try { auto [v, pos] = findAlongAttrPath(state, attrPath, *emptyArgs, *vOutputs); state.forceValue(*v, pos); return {v, pos}; } catch (AttrPathNotFound & e) { + suggestions += e.info().suggestions; } } - throw Error("flake '%s' does not provide attribute %s", - flakeRef, showAttrPaths(getActualAttrPaths())); + throw Error( + suggestions, + "flake '%s' does not provide attribute %s", + flakeRef, + showAttrPaths(getActualAttrPaths()) + ); } std::vector, std::string>> diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index eb0e706c7..32deecfae 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -74,8 +74,14 @@ std::pair findAlongAttrPath(EvalState & state, const std::string & throw Error("empty attribute name in selection path '%1%'", attrPath); Bindings::iterator a = v->attrs->find(state.symbols.create(attr)); - if (a == v->attrs->end()) - throw AttrPathNotFound("attribute '%1%' in selection path '%2%' not found", attr, attrPath); + if (a == v->attrs->end()) { + std::set attrNames; + for (auto & attr : *v->attrs) + attrNames.insert(attr.name); + + auto suggestions = Suggestions::bestMatches(attrNames, attr); + throw AttrPathNotFound(suggestions, "attribute '%1%' in selection path '%2%' not found", attr, attrPath); + } v = &*a->value; pos = *a->pos; } diff --git a/src/libutil/args.cc b/src/libutil/args.cc index f970c0e9e..69aa0d094 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -328,8 +328,13 @@ MultiCommand::MultiCommand(const Commands & commands_) completions->add(name); } auto i = commands.find(s); - if (i == commands.end()) - throw UsageError("'%s' is not a recognised command", s); + if (i == commands.end()) { + std::set commandNames; + for (auto & [name, _] : commands) + commandNames.insert(name); + auto suggestions = Suggestions::bestMatches(commandNames, s); + throw UsageError(suggestions, "'%s' is not a recognised command", s); + } command = {s, i->second()}; command->second->parent = this; }} diff --git a/src/libutil/error.cc b/src/libutil/error.cc index dcd2f82a5..15ae7a759 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -282,6 +282,13 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s } } + auto suggestions = einfo.suggestions.trim(); + if (! suggestions.suggestions.empty()){ + oss << "Did you mean " << + suggestions.trim().pretty_print() << + "?" << std::endl; + } + // traces if (showTrace && !einfo.traces.empty()) { for (auto iter = einfo.traces.rbegin(); iter != einfo.traces.rend(); ++iter) { diff --git a/src/libutil/error.hh b/src/libutil/error.hh index d55e1d701..600e94888 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -1,5 +1,6 @@ #pragma once +#include "suggestions.hh" #include "ref.hh" #include "types.hh" #include "fmt.hh" @@ -112,6 +113,8 @@ struct ErrorInfo { std::optional errPos; std::list traces; + Suggestions suggestions; + static std::optional programName; }; @@ -141,6 +144,11 @@ public: : err { .level = lvlError, .msg = hintfmt(fs, args...) } { } + template + BaseError(const Suggestions & sug, const Args & ... args) + : err { .level = lvlError, .msg = hintfmt(args...), .suggestions = sug } + { } + BaseError(hintformat hint) : err { .level = lvlError, .msg = hint } { } diff --git a/src/libutil/suggestions.cc b/src/libutil/suggestions.cc new file mode 100644 index 000000000..96b48416b --- /dev/null +++ b/src/libutil/suggestions.cc @@ -0,0 +1,111 @@ +#include "suggestions.hh" +#include "ansicolor.hh" +#include "util.hh" +#include + +namespace nix { + +/** + * Return `some(distance)` where distance is an integer representing some + * notion of distance between both arguments. + * + * If the distance is too big, return none + */ +int distanceBetween(std::string_view first, std::string_view second) +{ + // Levenshtein distance. + // Implementation borrowed from + // https://en.wikipedia.org/wiki/Levenshtein_distance#Iterative_with_two_matrix_rows + + int m = first.size(); + int n = second.size(); + + auto v0 = std::vector(n+1); + auto v1 = std::vector(n+1); + + for (auto i = 0; i <= n; i++) + v0[i] = i; + + for (auto i = 0; i < m; i++) { + v1[0] = i+1; + + for (auto j = 0; j < n; j++) { + auto deletionCost = v0[j+1] + 1; + auto insertionCost = v1[j] + 1; + auto substitutionCost = first[i] == second[j] ? v0[j] : v0[j] + 1; + v1[j+1] = std::min({deletionCost, insertionCost, substitutionCost}); + } + + std::swap(v0, v1); + } + + return v0[n]; +} + +Suggestions Suggestions::bestMatches ( + std::set allMatches, + std::string query) +{ + std::set res; + for (const auto & possibleMatch : allMatches) { + res.insert(Suggestion { + .distance = distanceBetween(query, possibleMatch), + .suggestion = possibleMatch, + }); + } + return Suggestions { res }; +} + +Suggestions Suggestions::trim(int limit, int maxDistance) const +{ + std::set res; + + int count = 0; + + for (auto & elt : suggestions) { + if (count >= limit || elt.distance >= maxDistance) + break; + count++; + res.insert(elt); + } + + return Suggestions{res}; +} + +std::string Suggestion::pretty_print() const +{ + return ANSI_WARNING + filterANSIEscapes(suggestion) + ANSI_NORMAL; +} + +std::string Suggestions::pretty_print() const +{ + switch (suggestions.size()) { + case 0: + return ""; + case 1: + return suggestions.begin()->pretty_print(); + default: { + std::string res = "one of "; + auto iter = suggestions.begin(); + res += iter->pretty_print(); // Iter can’t be end() because the container isn’t null + iter++; + auto last = suggestions.end(); last--; + for ( ; iter != suggestions.end() ; iter++) { + res += (iter == last) ? " or " : ", "; + res += iter->pretty_print(); + } + return res; + } + } +} + +Suggestions & Suggestions::operator+=(const Suggestions & other) +{ + suggestions.insert( + other.suggestions.begin(), + other.suggestions.end() + ); + return *this; +} + +} diff --git a/src/libutil/suggestions.hh b/src/libutil/suggestions.hh new file mode 100644 index 000000000..3caed487a --- /dev/null +++ b/src/libutil/suggestions.hh @@ -0,0 +1,41 @@ +#pragma once + +#include "comparator.hh" +#include "types.hh" +#include + +namespace nix { + +/** + * A potential suggestion for the cli interface. + */ +class Suggestion { +public: + int distance; // The smaller the better + std::string suggestion; + + std::string pretty_print() const; + + GENERATE_CMP(Suggestion, me->distance, me->suggestion) +}; + +class Suggestions { +public: + std::set suggestions; + + std::string pretty_print() const; + + Suggestions trim( + int limit = 5, + int maxDistance = 2 + ) const; + + static Suggestions bestMatches ( + std::set allMatches, + std::string query + ); + + Suggestions& operator+=(const Suggestions & other); +}; + +} From 2405bbbb5edd204d6031edcd470a2057f4a25782 Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 3 Mar 2022 13:12:27 +0100 Subject: [PATCH 2/8] Add some tests for the suggestions --- src/libutil/suggestions.cc | 13 +++------- src/libutil/suggestions.hh | 2 ++ src/libutil/tests/suggestions.cc | 43 ++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 src/libutil/tests/suggestions.cc diff --git a/src/libutil/suggestions.cc b/src/libutil/suggestions.cc index 96b48416b..bcd93aa6b 100644 --- a/src/libutil/suggestions.cc +++ b/src/libutil/suggestions.cc @@ -5,15 +5,8 @@ namespace nix { -/** - * Return `some(distance)` where distance is an integer representing some - * notion of distance between both arguments. - * - * If the distance is too big, return none - */ -int distanceBetween(std::string_view first, std::string_view second) +int levenshteinDistance(std::string_view first, std::string_view second) { - // Levenshtein distance. // Implementation borrowed from // https://en.wikipedia.org/wiki/Levenshtein_distance#Iterative_with_two_matrix_rows @@ -49,7 +42,7 @@ Suggestions Suggestions::bestMatches ( std::set res; for (const auto & possibleMatch : allMatches) { res.insert(Suggestion { - .distance = distanceBetween(query, possibleMatch), + .distance = levenshteinDistance(query, possibleMatch), .suggestion = possibleMatch, }); } @@ -63,7 +56,7 @@ Suggestions Suggestions::trim(int limit, int maxDistance) const int count = 0; for (auto & elt : suggestions) { - if (count >= limit || elt.distance >= maxDistance) + if (count >= limit || elt.distance > maxDistance) break; count++; res.insert(elt); diff --git a/src/libutil/suggestions.hh b/src/libutil/suggestions.hh index 3caed487a..76063a261 100644 --- a/src/libutil/suggestions.hh +++ b/src/libutil/suggestions.hh @@ -6,6 +6,8 @@ namespace nix { +int levenshteinDistance(std::string_view first, std::string_view second); + /** * A potential suggestion for the cli interface. */ diff --git a/src/libutil/tests/suggestions.cc b/src/libutil/tests/suggestions.cc new file mode 100644 index 000000000..279994abc --- /dev/null +++ b/src/libutil/tests/suggestions.cc @@ -0,0 +1,43 @@ +#include "suggestions.hh" +#include + +namespace nix { + + struct LevenshteinDistanceParam { + std::string s1, s2; + int distance; + }; + + class LevenshteinDistanceTest : + public testing::TestWithParam { + }; + + TEST_P(LevenshteinDistanceTest, CorrectlyComputed) { + auto params = GetParam(); + + ASSERT_EQ(levenshteinDistance(params.s1, params.s2), params.distance); + ASSERT_EQ(levenshteinDistance(params.s2, params.s1), params.distance); + } + + INSTANTIATE_TEST_SUITE_P(LevenshteinDistance, LevenshteinDistanceTest, + testing::Values( + LevenshteinDistanceParam{"foo", "foo", 0}, + LevenshteinDistanceParam{"foo", "", 3}, + LevenshteinDistanceParam{"", "", 0}, + LevenshteinDistanceParam{"foo", "fo", 1}, + LevenshteinDistanceParam{"foo", "oo", 1}, + LevenshteinDistanceParam{"foo", "fao", 1}, + LevenshteinDistanceParam{"foo", "abc", 3} + ) + ); + + TEST(Suggestions, Trim) { + auto suggestions = Suggestions::bestMatches({"foooo", "bar", "fo", "gao"}, "foo"); + auto onlyOne = suggestions.trim(1); + ASSERT_EQ(onlyOne.suggestions.size(), 1); + ASSERT_TRUE(onlyOne.suggestions.begin()->suggestion == "fo"); + + auto closest = suggestions.trim(999, 2); + ASSERT_EQ(closest.suggestions.size(), 3); + } +} From 98e361ad4c1a26d4ffe4762a6f33bb9e39321a39 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 4 Mar 2022 09:44:00 +0100 Subject: [PATCH 3/8] Also display suggestions for the commands using the eval cache Make `nix build .#nix-armv8l-linux` work for example --- src/libcmd/installables.cc | 19 +++++++++---- src/libexpr/eval-cache.cc | 35 +++++++++++++++++------ src/libexpr/eval-cache.hh | 8 ++++-- src/libutil/suggestions.hh | 58 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 17 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 888d863ff..03f3bd409 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -272,9 +272,9 @@ void completeFlakeRefWithFragment( auto attr = root->findAlongAttrPath(attrPath); if (!attr) continue; - for (auto & attr2 : attr->getAttrs()) { + for (auto & attr2 : (*attr)->getAttrs()) { if (hasPrefix(attr2, lastAttr)) { - auto attrPath2 = attr->getAttrPath(attr2); + auto attrPath2 = (*attr)->getAttrPath(attr2); /* Strip the attrpath prefix. */ attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size()); completions->add(flakeRefS + "#" + concatStringsSep(".", attrPath2)); @@ -568,15 +568,22 @@ std::tuple InstallableF auto cache = openEvalCache(*state, lockedFlake); auto root = cache->getRoot(); + Suggestions suggestions; + for (auto & attrPath : getActualAttrPaths()) { debug("trying flake output attribute '%s'", attrPath); - auto attr = root->findAlongAttrPath( + auto attrOrSuggestions = root->findAlongAttrPath( parseAttrPath(*state, attrPath), true ); - if (!attr) continue; + if (!attrOrSuggestions) { + suggestions += attrOrSuggestions.getSuggestions(); + continue; + } + + auto attr = *attrOrSuggestions; if (!attr->isDerivation()) throw Error("flake output attribute '%s' is not a derivation", attrPath); @@ -591,7 +598,7 @@ std::tuple InstallableF return {attrPath, lockedFlake->flake.lockedRef, std::move(drvInfo)}; } - throw Error("flake '%s' does not provide attribute %s", + throw Error(suggestions, "flake '%s' does not provide attribute %s", flakeRef, showAttrPaths(getActualAttrPaths())); } @@ -642,7 +649,7 @@ InstallableFlake::getCursors(EvalState & state) for (auto & attrPath : getActualAttrPaths()) { auto attr = root->findAlongAttrPath(parseAttrPath(state, attrPath)); - if (attr) res.push_back({attr, attrPath}); + if (attr) res.push_back({*attr, attrPath}); } return res; diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 00d0749f9..188223957 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -406,6 +406,16 @@ Value & AttrCursor::forceValue() return v; } +Suggestions AttrCursor::getSuggestionsForAttr(Symbol name) +{ + auto attrNames = getAttrs(); + std::set strAttrNames; + for (auto & name : attrNames) + strAttrNames.insert(std::string(name)); + + return Suggestions::bestMatches(strAttrNames, name); +} + std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErrors) { if (root->db) { @@ -446,6 +456,11 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro return nullptr; //throw TypeError("'%s' is not an attribute set", getAttrPathStr()); + for (auto & attr : *v.attrs) { + if (root->db) + root->db->setPlaceholder({cachedValue->first, attr.name}); + } + auto attr = v.attrs->get(name); if (!attr) { @@ -464,7 +479,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro cachedValue2 = {root->db->setPlaceholder({cachedValue->first, name}), placeholder_t()}; } - return std::make_shared( + return make_ref( root, std::make_pair(shared_from_this(), name), attr->value, std::move(cachedValue2)); } @@ -473,27 +488,31 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name) return maybeGetAttr(root->state.symbols.create(name)); } -std::shared_ptr AttrCursor::getAttr(Symbol name, bool forceErrors) +ref AttrCursor::getAttr(Symbol name, bool forceErrors) { auto p = maybeGetAttr(name, forceErrors); if (!p) throw Error("attribute '%s' does not exist", getAttrPathStr(name)); - return p; + return ref(p); } -std::shared_ptr AttrCursor::getAttr(std::string_view name) +ref AttrCursor::getAttr(std::string_view name) { return getAttr(root->state.symbols.create(name)); } -std::shared_ptr AttrCursor::findAlongAttrPath(const std::vector & attrPath, bool force) +OrSuggestions> AttrCursor::findAlongAttrPath(const std::vector & attrPath, bool force) { auto res = shared_from_this(); for (auto & attr : attrPath) { - res = res->maybeGetAttr(attr, force); - if (!res) return {}; + auto child = res->maybeGetAttr(attr, force); + if (!child) { + auto suggestions = res->getSuggestionsForAttr(attr); + return OrSuggestions>::failed(suggestions); + } + res = child; } - return res; + return ref(res); } std::string AttrCursor::getString() diff --git a/src/libexpr/eval-cache.hh b/src/libexpr/eval-cache.hh index 43b34ebcb..40f1d4ffc 100644 --- a/src/libexpr/eval-cache.hh +++ b/src/libexpr/eval-cache.hh @@ -94,15 +94,17 @@ public: std::string getAttrPathStr(Symbol name) const; + Suggestions getSuggestionsForAttr(Symbol name); + std::shared_ptr maybeGetAttr(Symbol name, bool forceErrors = false); std::shared_ptr maybeGetAttr(std::string_view name); - std::shared_ptr getAttr(Symbol name, bool forceErrors = false); + ref getAttr(Symbol name, bool forceErrors = false); - std::shared_ptr getAttr(std::string_view name); + ref getAttr(std::string_view name); - std::shared_ptr findAlongAttrPath(const std::vector & attrPath, bool force = false); + OrSuggestions> findAlongAttrPath(const std::vector & attrPath, bool force = false); std::string getString(); diff --git a/src/libutil/suggestions.hh b/src/libutil/suggestions.hh index 76063a261..63426259d 100644 --- a/src/libutil/suggestions.hh +++ b/src/libutil/suggestions.hh @@ -40,4 +40,62 @@ public: Suggestions& operator+=(const Suggestions & other); }; +// Either a value of type `T`, or some suggestions +template +class OrSuggestions { +public: + using Raw = std::variant; + + Raw raw; + + T* operator ->() + { + return &**this; + } + + T& operator *() + { + if (auto elt = std::get_if(&raw)) + return *elt; + throw Error("Invalid access to a failed value"); + } + + operator bool() const noexcept + { + return std::holds_alternative(raw); + } + + OrSuggestions(T t) + : raw(t) + { + } + + OrSuggestions() + : raw(Suggestions{}) + { + } + + static OrSuggestions failed(const Suggestions & s) + { + auto res = OrSuggestions(); + res.raw = s; + return res; + } + + static OrSuggestions failed() + { + return OrSuggestions::failed(Suggestions{}); + } + + const Suggestions & get_suggestions() + { + static Suggestions noSuggestions; + if (const auto & suggestions = std::get_if(&raw)) + return *suggestions; + else + return noSuggestions; + } + +}; + } From 91635206c0d33f9862b788487aa8a056a791af94 Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 4 Mar 2022 10:29:48 +0100 Subject: [PATCH 4/8] Add some end-to-end tests for the suggestions --- tests/local.mk | 5 +++-- tests/suggestions.sh | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 tests/suggestions.sh diff --git a/tests/local.mk b/tests/local.mk index c3a6aa1fc..8032fc38a 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -92,8 +92,9 @@ nix_tests = \ bash-profile.sh \ pass-as-file.sh \ describe-stores.sh \ - store-ping.sh \ - nix-profile.sh + nix-profile.sh \ + suggestions.sh \ + store-ping.sh ifeq ($(HAVE_LIBCPUID), 1) nix_tests += compute-levels.sh diff --git a/tests/suggestions.sh b/tests/suggestions.sh new file mode 100644 index 000000000..16a5a7004 --- /dev/null +++ b/tests/suggestions.sh @@ -0,0 +1,36 @@ +source common.sh + +clearStore + +cd "$TEST_HOME" + +cat < flake.nix +{ + outputs = a: { + packages.$system = { + foo = 1; + fo1 = 1; + fo2 = 1; + fooo = 1; + foooo = 1; + fooooo = 1; + fooooo1 = 1; + fooooo2 = 1; + fooooo3 = 1; + fooooo4 = 1; + fooooo5 = 1; + fooooo6 = 1; + }; + }; +} +EOF + +# Probable typo in the requested attribute path. Suggest some close possibilities +NIX_BUILD_STDERR_WITH_SUGGESTIONS=$(! nix build .\#fob 2>&1 1>/dev/null) +[[ "$NIX_BUILD_STDERR_WITH_SUGGESTIONS" =~ "Did you mean one of fo1, fo2, foo or fooo?" ]] || \ + fail "The nix build stderr should suggest the three closest possiblities" + +# None of the possible attributes is close to `bar`, so shouldn’t suggest anything +NIX_BUILD_STDERR_WITH_NO_CLOSE_SUGGESTION=$(! nix build .\#bar 2>&1 1>/dev/null) +[[ ! "$NIX_BUILD_STDERR_WITH_NO_CLOSE_SUGGESTION" =~ "Did you mean" ]] || \ + fail "The nix build stderr shouldn’t suggest anything if there’s nothing relevant to suggest" From b44cebd1fd68f89bbc2317e2b7978042309e17bb Mon Sep 17 00:00:00 2001 From: regnat Date: Fri, 4 Mar 2022 10:47:58 +0100 Subject: [PATCH 5/8] Add a release-notes entry for the cli suggestions --- doc/manual/src/release-notes/rl-next.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md index 7dd8387d8..1a74dec95 100644 --- a/doc/manual/src/release-notes/rl-next.md +++ b/doc/manual/src/release-notes/rl-next.md @@ -26,3 +26,6 @@ * Templates can now define a `welcomeText` attribute, which is printed out by `nix flake {init,new} --template