From 63f520fd00ec86f63a0e036f0be52cac822b2ac1 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 9 Jul 2024 11:49:18 +0200 Subject: [PATCH 01/19] doc/testing: Typo --- doc/manual/src/contributing/testing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/src/contributing/testing.md b/doc/manual/src/contributing/testing.md index a96ba997b..3949164d5 100644 --- a/doc/manual/src/contributing/testing.md +++ b/doc/manual/src/contributing/testing.md @@ -91,7 +91,7 @@ A environment variables that Google Test accepts are also worth knowing: Putting the two together, one might run ```bash -GTEST_BREIF=1 GTEST_FILTER='ErrorTraceTest.*' meson test nix-expr-tests -v +GTEST_BRIEF=1 GTEST_FILTER='ErrorTraceTest.*' meson test nix-expr-tests -v ``` for short but comprensive output. From e5af7cbeb9fdf5cff5f8cb25fbb99dccf13e47af Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 9 Jul 2024 11:43:07 +0200 Subject: [PATCH 02/19] libutil: Add Pos::getSnippetUpTo(Pos) --- src/libutil/position.cc | 46 +++++++++++++ src/libutil/position.hh | 2 + tests/unit/libutil/position.cc | 120 +++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+) create mode 100644 tests/unit/libutil/position.cc diff --git a/src/libutil/position.cc b/src/libutil/position.cc index 573efeeb2..508816d85 100644 --- a/src/libutil/position.cc +++ b/src/libutil/position.cc @@ -110,4 +110,50 @@ void Pos::LinesIterator::bump(bool atFirst) input.remove_prefix(eol); } +std::string Pos::getSnippetUpTo(const Pos & end) const { + assert(this->origin == end.origin); + + if (end.line < this->line) + return ""; + + if (auto source = getSource()) { + + auto firstLine = LinesIterator(*source); + for (auto i = 1; i < this->line; ++i) { + ++firstLine; + } + + auto lastLine = LinesIterator(*source); + for (auto i = 1; i < end.line; ++i) { + ++lastLine; + } + + LinesIterator linesEnd; + + std::string result; + for (auto i = firstLine; i != linesEnd; ++i) { + auto firstColumn = i == firstLine ? (this->column ? this->column - 1 : 0) : 0; + if (firstColumn > i->size()) + firstColumn = i->size(); + + auto lastColumn = i == lastLine ? (end.column ? end.column - 1 : 0) : std::numeric_limits::max(); + if (lastColumn < firstColumn) + lastColumn = firstColumn; + if (lastColumn > i->size()) + lastColumn = i->size(); + + result += i->substr(firstColumn, lastColumn - firstColumn); + + if (i == lastLine) { + break; + } else { + result += '\n'; + } + } + return result; + } + return ""; +} + + } diff --git a/src/libutil/position.hh b/src/libutil/position.hh index f8f34419b..729f2a523 100644 --- a/src/libutil/position.hh +++ b/src/libutil/position.hh @@ -63,6 +63,8 @@ struct Pos bool operator==(const Pos & rhs) const = default; auto operator<=>(const Pos & rhs) const = default; + std::string getSnippetUpTo(const Pos & end) const; + struct LinesIterator { using difference_type = size_t; using value_type = std::string_view; diff --git a/tests/unit/libutil/position.cc b/tests/unit/libutil/position.cc new file mode 100644 index 000000000..d38d2c538 --- /dev/null +++ b/tests/unit/libutil/position.cc @@ -0,0 +1,120 @@ +#include + +#include "position.hh" + +namespace nix { + +inline Pos::Origin makeStdin(std::string s) +{ + return Pos::Stdin{make_ref(s)}; +} + +TEST(Position, getSnippetUpTo_0) +{ + Pos::Origin o = makeStdin(""); + Pos p(1, 1, o); + ASSERT_EQ(p.getSnippetUpTo(p), ""); +} +TEST(Position, getSnippetUpTo_1) +{ + Pos::Origin o = makeStdin("x"); + { + // NOTE: line and column are actually 1-based indexes + Pos start(0, 0, o); + Pos end(99, 99, o); + ASSERT_EQ(start.getSnippetUpTo(start), ""); + ASSERT_EQ(start.getSnippetUpTo(end), "x"); + ASSERT_EQ(end.getSnippetUpTo(end), ""); + ASSERT_EQ(end.getSnippetUpTo(start), ""); + } + { + // NOTE: line and column are actually 1-based indexes + Pos start(0, 99, o); + Pos end(99, 0, o); + ASSERT_EQ(start.getSnippetUpTo(start), ""); + + // "x" might be preferable, but we only care about not crashing for invalid inputs + ASSERT_EQ(start.getSnippetUpTo(end), ""); + + ASSERT_EQ(end.getSnippetUpTo(end), ""); + ASSERT_EQ(end.getSnippetUpTo(start), ""); + } + { + Pos start(1, 1, o); + Pos end(1, 99, o); + ASSERT_EQ(start.getSnippetUpTo(start), ""); + ASSERT_EQ(start.getSnippetUpTo(end), "x"); + ASSERT_EQ(end.getSnippetUpTo(end), ""); + ASSERT_EQ(end.getSnippetUpTo(start), ""); + } + { + Pos start(1, 1, o); + Pos end(99, 99, o); + ASSERT_EQ(start.getSnippetUpTo(start), ""); + ASSERT_EQ(start.getSnippetUpTo(end), "x"); + ASSERT_EQ(end.getSnippetUpTo(end), ""); + ASSERT_EQ(end.getSnippetUpTo(start), ""); + } +} +TEST(Position, getSnippetUpTo_2) +{ + Pos::Origin o = makeStdin("asdf\njkl\nqwer"); + { + Pos start(1, 1, o); + Pos end(1, 2, o); + ASSERT_EQ(start.getSnippetUpTo(start), ""); + ASSERT_EQ(start.getSnippetUpTo(end), "a"); + ASSERT_EQ(end.getSnippetUpTo(end), ""); + ASSERT_EQ(end.getSnippetUpTo(start), ""); + } + { + Pos start(1, 2, o); + Pos end(1, 3, o); + ASSERT_EQ(start.getSnippetUpTo(end), "s"); + } + { + Pos start(1, 2, o); + Pos end(2, 2, o); + ASSERT_EQ(start.getSnippetUpTo(end), "sdf\nj"); + } + { + Pos start(1, 2, o); + Pos end(3, 2, o); + ASSERT_EQ(start.getSnippetUpTo(end), "sdf\njkl\nq"); + } + { + Pos start(1, 2, o); + Pos end(2, 99, o); + ASSERT_EQ(start.getSnippetUpTo(end), "sdf\njkl"); + } + { + Pos start(1, 4, o); + Pos end(2, 99, o); + ASSERT_EQ(start.getSnippetUpTo(end), "f\njkl"); + } + { + Pos start(1, 5, o); + Pos end(2, 99, o); + ASSERT_EQ(start.getSnippetUpTo(end), "\njkl"); + } + { + Pos start(1, 6, o); // invalid: starting column past last "line character", ie at the newline + Pos end(2, 99, o); + ASSERT_EQ(start.getSnippetUpTo(end), "\njkl"); // jkl might be acceptable for this invalid start position + } + { + Pos start(1, 1, o); + Pos end(2, 0, o); // invalid + ASSERT_EQ(start.getSnippetUpTo(end), "asdf\n"); + } +} + +TEST(Position, example_1) +{ + Pos::Origin o = makeStdin(" unambiguous = \n /** Very close */\n x: x;\n# ok\n"); + Pos start(2, 5, o); + Pos end(2, 22, o); + ASSERT_EQ(start.getSnippetUpTo(end), "/** Very close */"); +} + +} // namespace nix From 7fae378835534d2a17818fd7cd91aae91320aa03 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 8 Jul 2024 17:39:26 +0200 Subject: [PATCH 03/19] Track doc comments and render them in :doc --- .../rl-next/repl-doc-renders-doc-comments.md | 53 ++++++++++++++++ src/libexpr/eval.cc | 61 +++++++++++++++++++ src/libexpr/lexer.l | 56 +++++++++++++++-- src/libexpr/nixexpr.cc | 14 +++++ src/libexpr/nixexpr.hh | 53 +++++++++++++++- src/libexpr/parser-state.hh | 41 +++++++++++++ src/libexpr/parser.y | 51 +++++++++++++--- tests/functional/repl.sh | 30 +++++++++ tests/functional/repl/characterisation/empty | 0 .../repl/doc-comment-function.expected | 8 +++ tests/functional/repl/doc-comment-function.in | 1 + .../functional/repl/doc-comment-function.nix | 3 + tests/functional/repl/doc-comments.nix | 57 +++++++++++++++++ tests/functional/repl/doc-compact.expected | 11 ++++ tests/functional/repl/doc-compact.in | 2 + tests/functional/repl/doc-floatedIn.expected | 11 ++++ tests/functional/repl/doc-floatedIn.in | 2 + .../repl/doc-lambda-flavors.expected | 29 +++++++++ tests/functional/repl/doc-lambda-flavors.in | 5 ++ .../functional/repl/doc-measurement.expected | 11 ++++ tests/functional/repl/doc-measurement.in | 2 + tests/functional/repl/doc-multiply.expected | 15 +++++ tests/functional/repl/doc-multiply.in | 2 + .../functional/repl/doc-unambiguous.expected | 11 ++++ tests/functional/repl/doc-unambiguous.in | 2 + 25 files changed, 515 insertions(+), 16 deletions(-) create mode 100644 doc/manual/rl-next/repl-doc-renders-doc-comments.md create mode 100644 tests/functional/repl/characterisation/empty create mode 100644 tests/functional/repl/doc-comment-function.expected create mode 100644 tests/functional/repl/doc-comment-function.in create mode 100644 tests/functional/repl/doc-comment-function.nix create mode 100644 tests/functional/repl/doc-comments.nix create mode 100644 tests/functional/repl/doc-compact.expected create mode 100644 tests/functional/repl/doc-compact.in create mode 100644 tests/functional/repl/doc-floatedIn.expected create mode 100644 tests/functional/repl/doc-floatedIn.in create mode 100644 tests/functional/repl/doc-lambda-flavors.expected create mode 100644 tests/functional/repl/doc-lambda-flavors.in create mode 100644 tests/functional/repl/doc-measurement.expected create mode 100644 tests/functional/repl/doc-measurement.in create mode 100644 tests/functional/repl/doc-multiply.expected create mode 100644 tests/functional/repl/doc-multiply.in create mode 100644 tests/functional/repl/doc-unambiguous.expected create mode 100644 tests/functional/repl/doc-unambiguous.in diff --git a/doc/manual/rl-next/repl-doc-renders-doc-comments.md b/doc/manual/rl-next/repl-doc-renders-doc-comments.md new file mode 100644 index 000000000..c9213a88c --- /dev/null +++ b/doc/manual/rl-next/repl-doc-renders-doc-comments.md @@ -0,0 +1,53 @@ +--- +synopsis: "`nix-repl`'s `:doc` shows documentation comments" +significance: significant +issues: +- 3904 +- 10771 +prs: +- 1652 +- 9054 +- 11072 +--- + +`nix repl` has a `:doc` command that previously only rendered documentation for internally defined functions. +This feature has been extended to also render function documentation comments, in accordance with [RFC 145]. + +Example: + +``` +nix-repl> :doc lib.toFunction +Function toFunction + … defined at /home/user/h/nixpkgs/lib/trivial.nix:1072:5 + + Turns any non-callable values into constant functions. Returns + callable values as is. + +Inputs + + v + + : Any value + +Examples + + :::{.example} + +## lib.trivial.toFunction usage example + + | nix-repl> lib.toFunction 1 2 + | 1 + | + | nix-repl> lib.toFunction (x: x + 1) 2 + | 3 + + ::: +``` + +Known limitations: +- It currently only works for functions. We plan to extend this to attributes, which may contain arbitrary values. +- Some extensions to markdown are not yet supported, as you can see in the example above. + +We'd like to acknowledge Yingchi Long for proposing a proof of concept for this functionality in [#9054](https://github.com/NixOS/nix/pull/9054), as well as @sternenseemann and Johannes Kirschbauer for their contributions, proposals, and their work on [RFC 145]. + +[RFC 145]: https://github.com/NixOS/rfcs/pull/145 diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a4cf2e8c8..bd7bac0f1 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -559,6 +559,67 @@ std::optional EvalState::getDoc(Value & v) .doc = doc, }; } + if (v.isLambda()) { + auto exprLambda = v.payload.lambda.fun; + + std::stringstream s(std::ios_base::out); + std::string name; + auto pos = positions[exprLambda->getPos()]; + std::string docStr; + + if (exprLambda->name) { + name = symbols[exprLambda->name]; + } + + if (exprLambda->docComment) { + auto begin = positions[exprLambda->docComment.begin]; + auto end = positions[exprLambda->docComment.end]; + auto docCommentStr = begin.getSnippetUpTo(end); + + // Strip "/**" and "*/" + constexpr size_t prefixLen = 3; + constexpr size_t suffixLen = 2; + docStr = docCommentStr.substr(prefixLen, docCommentStr.size() - prefixLen - suffixLen); + if (docStr.empty()) + return {}; + // Turn the now missing "/**" into indentation + docStr = " " + docStr; + // Strip indentation (for the whole, potentially multi-line string) + docStr = stripIndentation(docStr); + } + + if (name.empty()) { + s << "Function "; + } + else { + s << "Function **" << name << "**"; + if (pos) + s << "\\\n … " ; + else + s << "\\\n"; + } + if (pos) { + s << "defined at " << pos; + } + if (!docStr.empty()) { + s << "\n\n"; + } + + s << docStr; + + s << '\0'; // for making a c string below + std::string ss = s.str(); + + return Doc { + .pos = pos, + .name = name, + .arity = 0, // FIXME: figure out how deep by syntax only? It's not semantically useful though... + .args = {}, + .doc = + // FIXME: this leaks; make the field std::string? + strdup(ss.data()), + }; + } return {}; } diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index 8c0f9d1f2..459f1d6f3 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -5,7 +5,7 @@ %option stack %option nodefault %option nounput noyy_top_state - +%option extra-type="::nix::LexerState *" %s DEFAULT %x STRING @@ -23,6 +23,12 @@ #include "nixexpr.hh" #include "parser-tab.hh" +// !!! FIXME !!! +#define YY_EXTRA_TYPE ::nix::LexerState * +int yylex_init_extra ( YY_EXTRA_TYPE user_defined, yyscan_t* scanner); +YY_EXTRA_TYPE yyget_extra ( yyscan_t yyscanner ); +#undef YY_EXTRA_TYPE + using namespace nix; namespace nix { @@ -35,10 +41,24 @@ static void initLoc(YYLTYPE * loc) loc->first_column = loc->last_column = 0; } -static void adjustLoc(YYLTYPE * loc, const char * s, size_t len) +static void adjustLoc(yyscan_t yyscanner, YYLTYPE * loc, const char * s, size_t len) { loc->stash(); + LexerState & lexerState = *yyget_extra(yyscanner); + + if (lexerState.docCommentDistance == 1) { + // Preceding token was a doc comment. + ParserLocation doc; + doc.first_column = lexerState.lastDocCommentLoc.first_column; + ParserLocation docEnd; + docEnd.first_column = lexerState.lastDocCommentLoc.last_column; + DocComment docComment{lexerState.at(doc), lexerState.at(docEnd)}; + PosIdx locPos = lexerState.at(*loc); + lexerState.positionToDocComment.emplace(locPos, docComment); + } + lexerState.docCommentDistance++; + loc->first_column = loc->last_column; loc->last_column += len; } @@ -79,7 +99,7 @@ static StringToken unescapeStr(SymbolTable & symbols, char * s, size_t length) #pragma GCC diagnostic ignored "-Wimplicit-fallthrough" #define YY_USER_INIT initLoc(yylloc) -#define YY_USER_ACTION adjustLoc(yylloc, yytext, yyleng); +#define YY_USER_ACTION adjustLoc(yyscanner, yylloc, yytext, yyleng); #define PUSH_STATE(state) yy_push_state(state, yyscanner) #define POP_STATE() yy_pop_state(yyscanner) @@ -279,9 +299,33 @@ or { return OR_KW; } {SPATH} { yylval->path = {yytext, (size_t) yyleng}; return SPATH; } {URI} { yylval->uri = {yytext, (size_t) yyleng}; return URI; } -[ \t\r\n]+ /* eat up whitespace */ -\#[^\r\n]* /* single-line comments */ -\/\*([^*]|\*+[^*/])*\*+\/ /* long comments */ +%{ +// Doc comment rule +// +// \/\*\* /** +// [^/*] reject /**/ (empty comment) and /*** +// ([^*]|\*+[^*/])*\*+\/ same as the long comment rule +// ( )* zero or more non-ending sequences +// \* end(1) +// \/ end(2) +%} +\/\*\*[^/*]([^*]|\*+[^*/])*\*+\/ /* doc comments */ { + LexerState & lexerState = *yyget_extra(yyscanner); + lexerState.docCommentDistance = 0; + lexerState.lastDocCommentLoc.first_line = yylloc->first_line; + lexerState.lastDocCommentLoc.first_column = yylloc->first_column; + lexerState.lastDocCommentLoc.last_column = yylloc->last_column; +} + + +%{ +// The following rules have docCommentDistance-- +// This compensates for the docCommentDistance++ which happens by default to +// make all the other rules invalidate the doc comment. +%} +[ \t\r\n]+ /* eat up whitespace */ { yyget_extra(yyscanner)->docCommentDistance--; } +\#[^\r\n]* /* single-line comments */ { yyget_extra(yyscanner)->docCommentDistance--; } +\/\*([^*]|\*+[^*/])*\*+\/ /* long comments */ { yyget_extra(yyscanner)->docCommentDistance--; } {ANY} { /* Don't return a negative number, as this will cause diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index c1ffe3435..6e705c314 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -583,6 +583,20 @@ std::string ExprLambda::showNamePos(const EvalState & state) const return fmt("%1% at %2%", id, state.positions[pos]); } +void ExprLambda::setDocComment(DocComment docComment) { + if (!this->docComment) { + this->docComment = docComment; + + // Curried functions are defined by putting a function directly + // in the body of another function. To render docs for those, we + // need to propagate the doc comment to the innermost function. + // + // If we have our own comment, we've already propagated it, so this + // belongs in the same conditional. + body->setDocComment(docComment); + } +}; + /* Position table. */ diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 5152e3119..913f46ff9 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -11,13 +11,56 @@ namespace nix { - -struct Env; -struct Value; class EvalState; +class PosTable; +struct Env; struct ExprWith; struct StaticEnv; +struct Value; +/** + * A documentation comment, in the sense of [RFC 145](https://github.com/NixOS/rfcs/blob/master/rfcs/0145-doc-strings.md) + * + * Note that this does not implement the following: + * - argument attribute names ("formals"): TBD + * - argument names: these are internal to the function and their names may not be optimal for documentation + * - function arity (degree of currying or number of ':'s): + * - Functions returning partially applied functions have a higher arity + * than can be determined locally and without evaluation. + * We do not want to present false data. + * - Some functions should be thought of as transformations of other + * functions. For instance `overlay -> overlay -> overlay` is the simplest + * way to understand `composeExtensions`, but its implementation looks like + * `f: g: final: prev: <...>`. The parameters `final` and `prev` are part + * of the overlay concept, while distracting from the function's purpose. + */ +struct DocComment { + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wcomment" // "nested comment start" is intentional + + /** + * Start of the comment, including `/**`. + */ + PosIdx begin; + +#pragma GCC diagnostic pop + + /** + * Position right after the final asterisk and `/` that terminate the comment. + */ + PosIdx end; + + /** + * Whether the comment is set. + * + * A `DocComment` is small enough that it makes sense to pass by value, and + * therefore baking optionality into it is also useful, to avoiding the memory + * overhead of `std::optional`. + */ + operator bool() const { return static_cast(begin); } + +}; /** * An attribute path is a sequence of attribute names. @@ -54,6 +97,7 @@ struct Expr virtual void eval(EvalState & state, Env & env, Value & v); virtual Value * maybeThunk(EvalState & state, Env & env); virtual void setName(Symbol name); + virtual void setDocComment(DocComment docComment) { }; virtual PosIdx getPos() const { return noPos; } }; @@ -278,6 +322,8 @@ struct ExprLambda : Expr Symbol arg; Formals * formals; Expr * body; + DocComment docComment; + ExprLambda(PosIdx pos, Symbol arg, Formals * formals, Expr * body) : pos(pos), arg(arg), formals(formals), body(body) { @@ -290,6 +336,7 @@ struct ExprLambda : Expr std::string showNamePos(const EvalState & state) const; inline bool hasFormals() const { return formals != nullptr; } PosIdx getPos() const override { return pos; } + virtual void setDocComment(DocComment docComment) override; COMMON_METHODS }; diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index cff6282fa..8dc910468 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -1,6 +1,8 @@ #pragma once ///@file +#include + #include "eval.hh" namespace nix { @@ -35,10 +37,44 @@ struct ParserLocation first_column = stashed_first_column; last_column = stashed_last_column; } + + /** Latest doc comment position, or 0. */ + int doc_comment_first_line, doc_comment_first_column, doc_comment_last_column; +}; + +struct LexerState +{ + /** + * Tracks the distance to the last doc comment, in terms of lexer tokens. + * + * The lexer sets this to 0 when reading a doc comment, and increments it + * for every matched rule; see `lexer-helpers.cc`. + * Whitespace and comment rules decrement the distance, so that they result + * in a net 0 change in distance. + */ + int docCommentDistance = std::numeric_limits::max(); + + /** + * The location of the last doc comment. + * + * (stashing fields are not used) + */ + ParserLocation lastDocCommentLoc; + + /** + * @brief Maps some positions to a DocComment, where the comment is relevant to the location. + */ + std::map positionToDocComment; + + PosTable & positions; + PosTable::Origin origin; + + PosIdx at(const ParserLocation & loc); }; struct ParserState { + const LexerState & lexerState; SymbolTable & symbols; PosTable & positions; Expr * result; @@ -270,6 +306,11 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos, return new ExprConcatStrings(pos, true, es2); } +inline PosIdx LexerState::at(const ParserLocation & loc) +{ + return positions.add(origin, loc.first_column); +} + inline PosIdx ParserState::at(const ParserLocation & loc) { return positions.add(origin, loc.first_column); diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 709a4532a..3c1bf95c8 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -74,6 +74,14 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char * }); } +#define SET_DOC_POS(lambda, pos) setDocPosition(state->lexerState, lambda, state->at(pos)) +static void setDocPosition(const LexerState & lexerState, ExprLambda * lambda, PosIdx start) { + auto it = lexerState.positionToDocComment.find(start); + if (it != lexerState.positionToDocComment.end()) { + lambda->setDocComment(it->second); + } +} + %} @@ -119,6 +127,7 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char * %token IND_STRING_OPEN IND_STRING_CLOSE %token ELLIPSIS + %right IMPL %left OR %left AND @@ -140,18 +149,28 @@ expr: expr_function; expr_function : ID ':' expr_function - { $$ = new ExprLambda(CUR_POS, state->symbols.create($1), 0, $3); } + { auto me = new ExprLambda(CUR_POS, state->symbols.create($1), 0, $3); + $$ = me; + SET_DOC_POS(me, @1); + } | '{' formals '}' ':' expr_function - { $$ = new ExprLambda(CUR_POS, state->validateFormals($2), $5); } + { auto me = new ExprLambda(CUR_POS, state->validateFormals($2), $5); + $$ = me; + SET_DOC_POS(me, @1); + } | '{' formals '}' '@' ID ':' expr_function { auto arg = state->symbols.create($5); - $$ = new ExprLambda(CUR_POS, arg, state->validateFormals($2, CUR_POS, arg), $7); + auto me = new ExprLambda(CUR_POS, arg, state->validateFormals($2, CUR_POS, arg), $7); + $$ = me; + SET_DOC_POS(me, @1); } | ID '@' '{' formals '}' ':' expr_function { auto arg = state->symbols.create($1); - $$ = new ExprLambda(CUR_POS, arg, state->validateFormals($4, CUR_POS, arg), $7); + auto me = new ExprLambda(CUR_POS, arg, state->validateFormals($4, CUR_POS, arg), $7); + $$ = me; + SET_DOC_POS(me, @1); } | ASSERT expr ';' expr_function { $$ = new ExprAssert(CUR_POS, $2, $4); } @@ -312,7 +331,20 @@ ind_string_parts ; binds - : binds attrpath '=' expr ';' { $$ = $1; state->addAttr($$, std::move(*$2), $4, state->at(@2)); delete $2; } + : binds attrpath '=' expr ';' { + $$ = $1; + + auto pos = state->at(@2); + { + auto it = state->lexerState.positionToDocComment.find(pos); + if (it != state->lexerState.positionToDocComment.end()) { + $4->setDocComment(it->second); + } + } + + state->addAttr($$, std::move(*$2), $4, pos); + delete $2; + } | binds INHERIT attrs ';' { $$ = $1; for (auto & [i, iPos] : *$3) { @@ -435,17 +467,22 @@ Expr * parseExprFromBuf( const Expr::AstSymbols & astSymbols) { yyscan_t scanner; + LexerState lexerState { + .positions = positions, + .origin = positions.addOrigin(origin, length), + }; ParserState state { + .lexerState = lexerState, .symbols = symbols, .positions = positions, .basePath = basePath, - .origin = positions.addOrigin(origin, length), + .origin = lexerState.origin, .rootFS = rootFS, .s = astSymbols, .settings = settings, }; - yylex_init(&scanner); + yylex_init_extra(&lexerState, &scanner); Finally _destroy([&] { yylex_destroy(scanner); }); yy_scan_buffer(text, length, scanner); diff --git a/tests/functional/repl.sh b/tests/functional/repl.sh index 86cd6f458..d356fe096 100755 --- a/tests/functional/repl.sh +++ b/tests/functional/repl.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash source common.sh +source characterisation/framework.sh testDir="$PWD" cd "$TEST_ROOT" @@ -244,3 +245,32 @@ testReplResponseNoRegex ' y = { a = 1 }; } ' + +# TODO: move init to characterisation/framework.sh +badDiff=0 +badExitCode=0 + +nixVersion="$(nix eval --impure --raw --expr 'builtins.nixVersion' --extra-experimental-features nix-command)" + +runRepl () { + # TODO: pass arguments to nix repl; see lang.sh + nix repl 2>&1 \ + | stripColors \ + | sed \ + -e "s@$testDir@/path/to/tests/functional@g" \ + -e "s@$nixVersion@@g" \ + -e "s@Added [0-9]* variables@Added variables@g" \ + | grep -vF $'warning: you don\'t have Internet access; disabling some network-dependent features' \ + ; +} + +for test in $(cd "$testDir/repl"; echo *.in); do + test="$(basename "$test" .in)" + in="$testDir/repl/$test.in" + actual="$testDir/repl/$test.actual" + expected="$testDir/repl/$test.expected" + (cd "$testDir/repl"; set +x; runRepl 2>&1) < "$in" > "$actual" + diffAndAcceptInner "$test" "$actual" "$expected" +done + +characterisationTestExit diff --git a/tests/functional/repl/characterisation/empty b/tests/functional/repl/characterisation/empty new file mode 100644 index 000000000..e69de29bb diff --git a/tests/functional/repl/doc-comment-function.expected b/tests/functional/repl/doc-comment-function.expected new file mode 100644 index 000000000..5ec465a96 --- /dev/null +++ b/tests/functional/repl/doc-comment-function.expected @@ -0,0 +1,8 @@ +Nix +Type :? for help. +Function defined at + /path/to/tests/functional/repl/doc-comment-function.nix:2:1 + + A doc comment for a file that only contains a function + + diff --git a/tests/functional/repl/doc-comment-function.in b/tests/functional/repl/doc-comment-function.in new file mode 100644 index 000000000..8f3c1388a --- /dev/null +++ b/tests/functional/repl/doc-comment-function.in @@ -0,0 +1 @@ +:doc import ./doc-comment-function.nix diff --git a/tests/functional/repl/doc-comment-function.nix b/tests/functional/repl/doc-comment-function.nix new file mode 100644 index 000000000..cdd241347 --- /dev/null +++ b/tests/functional/repl/doc-comment-function.nix @@ -0,0 +1,3 @@ +/** A doc comment for a file that only contains a function */ +{ ... }: +{ } diff --git a/tests/functional/repl/doc-comments.nix b/tests/functional/repl/doc-comments.nix new file mode 100644 index 000000000..a98f1c688 --- /dev/null +++ b/tests/functional/repl/doc-comments.nix @@ -0,0 +1,57 @@ +{ + /** + Perform *arithmetic* multiplication. It's kind of like repeated **addition**, very neat. + + ```nix + multiply 2 3 + => 6 + ``` + */ + multiply = x: y: x * y; + + /**👈 precisely this wide 👉*/ + measurement = x: x; + + floatedIn = /** This also works. */ + x: y: x; + + compact=/**boom*/x: x; + + /** Ignore!!! */ + unambiguous = + /** Very close */ + x: x; + + /** Firmly rigid. */ + constant = true; + + /** Immovably fixed. */ + lib.version = "9000"; + + /** Unchangeably constant. */ + lib.attr.empty = { }; + + lib.attr.undocumented = { }; + + nonStrict = /** My syntax is not strict, but I'm strict anyway. */ x: x; + strict = /** I don't have to be strict, but I am anyway. */ { ... }: null; + # Note that pre and post are the same here. I just had to name them somehow. + strictPre = /** Here's one way to do this */ a@{ ... }: a; + strictPost = /** Here's another way to do this */ { ... }@a: a; + + # TODO + + # /** This returns a documented function. */ + # documentedArgs = + # /** x */ + # x: + # /** y */ + # y: + # /** x + y */ + # x + y; + + # /** Documented formals */ + # documentedFormals = + # /** x */ + # x: x; +} diff --git a/tests/functional/repl/doc-compact.expected b/tests/functional/repl/doc-compact.expected new file mode 100644 index 000000000..4b05b653c --- /dev/null +++ b/tests/functional/repl/doc-compact.expected @@ -0,0 +1,11 @@ +Nix +Type :? for help. +Added variables. + +Function compact + … defined at + /path/to/tests/functional/repl/doc-comments.nix:18:20 + + boom + + diff --git a/tests/functional/repl/doc-compact.in b/tests/functional/repl/doc-compact.in new file mode 100644 index 000000000..c87c4e7ab --- /dev/null +++ b/tests/functional/repl/doc-compact.in @@ -0,0 +1,2 @@ +:l doc-comments.nix +:doc compact diff --git a/tests/functional/repl/doc-floatedIn.expected b/tests/functional/repl/doc-floatedIn.expected new file mode 100644 index 000000000..30f135725 --- /dev/null +++ b/tests/functional/repl/doc-floatedIn.expected @@ -0,0 +1,11 @@ +Nix +Type :? for help. +Added variables. + +Function floatedIn + … defined at + /path/to/tests/functional/repl/doc-comments.nix:16:5 + + This also works. + + diff --git a/tests/functional/repl/doc-floatedIn.in b/tests/functional/repl/doc-floatedIn.in new file mode 100644 index 000000000..97c12408e --- /dev/null +++ b/tests/functional/repl/doc-floatedIn.in @@ -0,0 +1,2 @@ +:l doc-comments.nix +:doc floatedIn diff --git a/tests/functional/repl/doc-lambda-flavors.expected b/tests/functional/repl/doc-lambda-flavors.expected new file mode 100644 index 000000000..4ac52f39e --- /dev/null +++ b/tests/functional/repl/doc-lambda-flavors.expected @@ -0,0 +1,29 @@ +Nix +Type :? for help. +Added variables. + +Function nonStrict + … defined at + /path/to/tests/functional/repl/doc-comments.nix:36:70 + + My syntax is not strict, but I'm strict anyway. + +Function strict + … defined at + /path/to/tests/functional/repl/doc-comments.nix:37:63 + + I don't have to be strict, but I am anyway. + +Function strictPre + … defined at + /path/to/tests/functional/repl/doc-comments.nix:39:48 + + Here's one way to do this + +Function strictPost + … defined at + /path/to/tests/functional/repl/doc-comments.nix:40:53 + + Here's another way to do this + + diff --git a/tests/functional/repl/doc-lambda-flavors.in b/tests/functional/repl/doc-lambda-flavors.in new file mode 100644 index 000000000..760c99636 --- /dev/null +++ b/tests/functional/repl/doc-lambda-flavors.in @@ -0,0 +1,5 @@ +:l doc-comments.nix +:doc nonStrict +:doc strict +:doc strictPre +:doc strictPost diff --git a/tests/functional/repl/doc-measurement.expected b/tests/functional/repl/doc-measurement.expected new file mode 100644 index 000000000..8598aaedb --- /dev/null +++ b/tests/functional/repl/doc-measurement.expected @@ -0,0 +1,11 @@ +Nix +Type :? for help. +Added variables. + +Function measurement + … defined at + /path/to/tests/functional/repl/doc-comments.nix:13:17 + + 👈 precisely this wide 👉 + + diff --git a/tests/functional/repl/doc-measurement.in b/tests/functional/repl/doc-measurement.in new file mode 100644 index 000000000..fecd5f9d2 --- /dev/null +++ b/tests/functional/repl/doc-measurement.in @@ -0,0 +1,2 @@ +:l doc-comments.nix +:doc measurement diff --git a/tests/functional/repl/doc-multiply.expected b/tests/functional/repl/doc-multiply.expected new file mode 100644 index 000000000..db82af91f --- /dev/null +++ b/tests/functional/repl/doc-multiply.expected @@ -0,0 +1,15 @@ +Nix +Type :? for help. +Added variables. + +Function multiply + … defined at + /path/to/tests/functional/repl/doc-comments.nix:10:14 + + Perform arithmetic multiplication. It's kind of like + repeated addition, very neat. + + | multiply 2 3 + | => 6 + + diff --git a/tests/functional/repl/doc-multiply.in b/tests/functional/repl/doc-multiply.in new file mode 100644 index 000000000..bffc6696f --- /dev/null +++ b/tests/functional/repl/doc-multiply.in @@ -0,0 +1,2 @@ +:l doc-comments.nix +:doc multiply diff --git a/tests/functional/repl/doc-unambiguous.expected b/tests/functional/repl/doc-unambiguous.expected new file mode 100644 index 000000000..0f89a6f64 --- /dev/null +++ b/tests/functional/repl/doc-unambiguous.expected @@ -0,0 +1,11 @@ +Nix +Type :? for help. +Added variables. + +Function unambiguous + … defined at + /path/to/tests/functional/repl/doc-comments.nix:23:5 + + Very close + + diff --git a/tests/functional/repl/doc-unambiguous.in b/tests/functional/repl/doc-unambiguous.in new file mode 100644 index 000000000..8282a5cb9 --- /dev/null +++ b/tests/functional/repl/doc-unambiguous.in @@ -0,0 +1,2 @@ +:l doc-comments.nix +:doc unambiguous From e68234c4f991071c0c7ff05852975a74d6a21771 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 9 Jul 2024 15:39:24 +0200 Subject: [PATCH 04/19] libexpr: Rearrange lexer files so that yylex_init_extra can be found --- src/libexpr/lexer-helpers.cc | 31 ++++++++++++++++++++++++++ src/libexpr/lexer-helpers.hh | 9 ++++++++ src/libexpr/lexer.l | 43 ++++++++---------------------------- src/libexpr/meson.build | 2 ++ 4 files changed, 51 insertions(+), 34 deletions(-) create mode 100644 src/libexpr/lexer-helpers.cc create mode 100644 src/libexpr/lexer-helpers.hh diff --git a/src/libexpr/lexer-helpers.cc b/src/libexpr/lexer-helpers.cc new file mode 100644 index 000000000..87c514c71 --- /dev/null +++ b/src/libexpr/lexer-helpers.cc @@ -0,0 +1,31 @@ +#include "lexer-tab.hh" +#include "lexer-helpers.hh" +#include "parser-tab.hh" + +void nix::lexer::internal::initLoc(YYLTYPE * loc) +{ + loc->first_line = loc->last_line = 0; + loc->first_column = loc->last_column = 0; +} + +void nix::lexer::internal::adjustLoc(yyscan_t yyscanner, YYLTYPE * loc, const char * s, size_t len) +{ + loc->stash(); + + LexerState & lexerState = *yyget_extra(yyscanner); + + if (lexerState.docCommentDistance == 1) { + // Preceding token was a doc comment. + ParserLocation doc; + doc.first_column = lexerState.lastDocCommentLoc.first_column; + ParserLocation docEnd; + docEnd.first_column = lexerState.lastDocCommentLoc.last_column; + DocComment docComment{lexerState.at(doc), lexerState.at(docEnd)}; + PosIdx locPos = lexerState.at(*loc); + lexerState.positionToDocComment.emplace(locPos, docComment); + } + lexerState.docCommentDistance++; + + loc->first_column = loc->last_column; + loc->last_column += len; +} diff --git a/src/libexpr/lexer-helpers.hh b/src/libexpr/lexer-helpers.hh new file mode 100644 index 000000000..caba6e18f --- /dev/null +++ b/src/libexpr/lexer-helpers.hh @@ -0,0 +1,9 @@ +#pragma once + +namespace nix::lexer::internal { + +void initLoc(YYLTYPE * loc); + +void adjustLoc(yyscan_t yyscanner, YYLTYPE * loc, const char * s, size_t len); + +} // namespace nix::lexer diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index 459f1d6f3..7d4e6963f 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -14,6 +14,10 @@ %x INPATH_SLASH %x PATH_START +%top { +#include "parser-tab.hh" // YYSTYPE +#include "parser-state.hh" +} %{ #ifdef __clang__ @@ -22,48 +26,19 @@ #include "nixexpr.hh" #include "parser-tab.hh" +#include "lexer-helpers.hh" -// !!! FIXME !!! -#define YY_EXTRA_TYPE ::nix::LexerState * -int yylex_init_extra ( YY_EXTRA_TYPE user_defined, yyscan_t* scanner); -YY_EXTRA_TYPE yyget_extra ( yyscan_t yyscanner ); -#undef YY_EXTRA_TYPE +namespace nix { + struct LexerState; +} using namespace nix; +using namespace nix::lexer::internal; namespace nix { #define CUR_POS state->at(*yylloc) -static void initLoc(YYLTYPE * loc) -{ - loc->first_line = loc->last_line = 0; - loc->first_column = loc->last_column = 0; -} - -static void adjustLoc(yyscan_t yyscanner, YYLTYPE * loc, const char * s, size_t len) -{ - loc->stash(); - - LexerState & lexerState = *yyget_extra(yyscanner); - - if (lexerState.docCommentDistance == 1) { - // Preceding token was a doc comment. - ParserLocation doc; - doc.first_column = lexerState.lastDocCommentLoc.first_column; - ParserLocation docEnd; - docEnd.first_column = lexerState.lastDocCommentLoc.last_column; - DocComment docComment{lexerState.at(doc), lexerState.at(docEnd)}; - PosIdx locPos = lexerState.at(*loc); - lexerState.positionToDocComment.emplace(locPos, docComment); - } - lexerState.docCommentDistance++; - - loc->first_column = loc->last_column; - loc->last_column += len; -} - - // we make use of the fact that the parser receives a private copy of the input // string and can munge around in it. static StringToken unescapeStr(SymbolTable & symbols, char * s, size_t length) diff --git a/src/libexpr/meson.build b/src/libexpr/meson.build index e65cf545f..fa90e7b41 100644 --- a/src/libexpr/meson.build +++ b/src/libexpr/meson.build @@ -139,6 +139,7 @@ sources = files( 'function-trace.cc', 'get-drvs.cc', 'json-to-value.cc', + 'lexer-helpers.cc', 'nixexpr.cc', 'paths.cc', 'primops.cc', @@ -165,6 +166,7 @@ headers = [config_h] + files( 'gc-small-vector.hh', 'get-drvs.hh', 'json-to-value.hh', + # internal: 'lexer-helpers.hh', 'nixexpr.hh', 'parser-state.hh', 'pos-idx.hh', From 491b9cf4154370df3da04977696dda3f053725f1 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 9 Jul 2024 19:26:22 +0200 Subject: [PATCH 05/19] Refactor: extract DocComment::getInnerText(PosTable) --- src/libexpr/eval.cc | 15 +-------------- src/libexpr/nixexpr.cc | 18 ++++++++++++++++++ src/libexpr/nixexpr.hh | 2 ++ 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index bd7bac0f1..f8282c400 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -572,20 +572,7 @@ std::optional EvalState::getDoc(Value & v) } if (exprLambda->docComment) { - auto begin = positions[exprLambda->docComment.begin]; - auto end = positions[exprLambda->docComment.end]; - auto docCommentStr = begin.getSnippetUpTo(end); - - // Strip "/**" and "*/" - constexpr size_t prefixLen = 3; - constexpr size_t suffixLen = 2; - docStr = docCommentStr.substr(prefixLen, docCommentStr.size() - prefixLen - suffixLen); - if (docStr.empty()) - return {}; - // Turn the now missing "/**" into indentation - docStr = " " + docStr; - // Strip indentation (for the whole, potentially multi-line string) - docStr = stripIndentation(docStr); + docStr = exprLambda->docComment.getInnerText(positions); } if (name.empty()) { diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 6e705c314..5479dd79e 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -641,4 +641,22 @@ size_t SymbolTable::totalSize() const return n; } +std::string DocComment::getInnerText(const PosTable & positions) const { + auto beginPos = positions[begin]; + auto endPos = positions[end]; + auto docCommentStr = beginPos.getSnippetUpTo(endPos); + + // Strip "/**" and "*/" + constexpr size_t prefixLen = 3; + constexpr size_t suffixLen = 2; + std::string docStr = docCommentStr.substr(prefixLen, docCommentStr.size() - prefixLen - suffixLen); + if (docStr.empty()) + return {}; + // Turn the now missing "/**" into indentation + docStr = " " + docStr; + // Strip indentation (for the whole, potentially multi-line string) + docStr = stripIndentation(docStr); + return docStr; +} + } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 913f46ff9..803b5ed8f 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -60,6 +60,8 @@ struct DocComment { */ operator bool() const { return static_cast(begin); } + std::string getInnerText(const PosTable & positions) const; + }; /** From d4f576b0b281265b58bb497108f8d063e2a0f06a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 9 Jul 2024 19:25:45 +0200 Subject: [PATCH 06/19] nix repl: Render docs for attributes --- .../rl-next/repl-doc-renders-doc-comments.md | 2 +- src/libcmd/repl.cc | 41 +++++++ src/libexpr/eval.cc | 42 +++++++- src/libexpr/eval.hh | 10 ++ src/libexpr/nixexpr.hh | 11 ++ src/libexpr/parser-state.hh | 2 +- src/libexpr/parser.y | 7 ++ src/libutil/position.hh | 8 ++ tests/functional/repl/doc-constant.expected | 102 ++++++++++++++++++ tests/functional/repl/doc-constant.in | 15 +++ 10 files changed, 237 insertions(+), 3 deletions(-) create mode 100644 tests/functional/repl/doc-constant.expected create mode 100644 tests/functional/repl/doc-constant.in diff --git a/doc/manual/rl-next/repl-doc-renders-doc-comments.md b/doc/manual/rl-next/repl-doc-renders-doc-comments.md index c9213a88c..05023697c 100644 --- a/doc/manual/rl-next/repl-doc-renders-doc-comments.md +++ b/doc/manual/rl-next/repl-doc-renders-doc-comments.md @@ -45,7 +45,7 @@ Examples ``` Known limitations: -- It currently only works for functions. We plan to extend this to attributes, which may contain arbitrary values. +- It does not render documentation for "formals", such as `{ /** the value to return */ x, ... }: x`. - Some extensions to markdown are not yet supported, as you can see in the example above. We'd like to acknowledge Yingchi Long for proposing a proof of concept for this functionality in [#9054](https://github.com/NixOS/nix/pull/9054), as well as @sternenseemann and Johannes Kirschbauer for their contributions, proposals, and their work on [RFC 145]. diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 37a34e3de..fb8106c46 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -27,6 +27,7 @@ #include "local-fs-store.hh" #include "print.hh" #include "ref.hh" +#include "value.hh" #if HAVE_BOEHMGC #define GC_INCLUDE_NEW @@ -616,6 +617,33 @@ ProcessLineResult NixRepl::processLine(std::string line) else if (command == ":doc") { Value v; + + auto expr = parseString(arg); + std::string fallbackName; + PosIdx fallbackPos; + DocComment fallbackDoc; + if (auto select = dynamic_cast(expr)) { + Value vAttrs; + auto name = select->evalExceptFinalSelect(*state, *env, vAttrs); + fallbackName = state->symbols[name]; + + state->forceAttrs(vAttrs, noPos, "while evaluating an attribute set to look for documentation"); + auto attrs = vAttrs.attrs(); + assert(attrs); + auto attr = attrs->get(name); + if (!attr) { + // Trigger the normal error + evalString(arg, v); + } + if (attr->pos) { + fallbackPos = attr->pos; + fallbackDoc = state->getDocCommentForPos(fallbackPos); + } + + } else { + evalString(arg, v); + } + evalString(arg, v); if (auto doc = state->getDoc(v)) { std::string markdown; @@ -633,6 +661,19 @@ ProcessLineResult NixRepl::processLine(std::string line) markdown += stripIndentation(doc->doc); logger->cout(trim(renderMarkdownToTerminal(markdown))); + } else if (fallbackPos) { + std::stringstream ss; + ss << "Attribute `" << fallbackName << "`\n\n"; + ss << " … defined at " << state->positions[fallbackPos] << "\n\n"; + if (fallbackDoc) { + ss << fallbackDoc.getInnerText(state->positions); + } else { + ss << "No documentation found.\n\n"; + } + + auto markdown = ss.str(); + logger->cout(trim(renderMarkdownToTerminal(markdown))); + } else throw Error("value does not have documentation"); } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index f8282c400..81c64ba71 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1415,6 +1415,22 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) v = *vAttrs; } +Symbol ExprSelect::evalExceptFinalSelect(EvalState & state, Env & env, Value & attrs) +{ + Value vTmp; + Symbol name = getName(attrPath[attrPath.size() - 1], state, env); + + if (attrPath.size() == 1) { + e->eval(state, env, vTmp); + } else { + ExprSelect init(*this); + init.attrPath.pop_back(); + init.eval(state, env, vTmp); + } + attrs = vTmp; + return name; +} + void ExprOpHasAttr::eval(EvalState & state, Env & env, Value & v) { @@ -2876,13 +2892,37 @@ Expr * EvalState::parse( const SourcePath & basePath, std::shared_ptr & staticEnv) { - auto result = parseExprFromBuf(text, length, origin, basePath, symbols, settings, positions, rootFS, exprSymbols); + DocCommentMap tmpDocComments; // Only used when not origin is not a SourcePath + DocCommentMap *docComments = &tmpDocComments; + + if (auto sourcePath = std::get_if(&origin)) { + auto [it, _] = positionToDocComment.try_emplace(*sourcePath); + docComments = &it->second; + } + + auto result = parseExprFromBuf(text, length, origin, basePath, symbols, settings, positions, *docComments, rootFS, exprSymbols); result->bindVars(*this, staticEnv); return result; } +DocComment EvalState::getDocCommentForPos(PosIdx pos) +{ + auto pos2 = positions[pos]; + auto path = pos2.getSourcePath(); + if (!path) + return {}; + + auto table = positionToDocComment.find(*path); + if (table == positionToDocComment.end()) + return {}; + + auto it = table->second.find(pos); + if (it == table->second.end()) + return {}; + return it->second; +} std::string ExternalValueBase::coerceToString(EvalState & state, const PosIdx & pos, NixStringContext & context, bool copyMore, bool copyToStore) const { diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 7dbf61c5d..3918fb092 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -130,6 +130,8 @@ struct Constant typedef std::map ValMap; #endif +typedef std::map DocCommentMap; + struct Env { Env * up; @@ -329,6 +331,12 @@ private: #endif FileEvalCache fileEvalCache; + /** + * Associate source positions of certain AST nodes with their preceding doc comment, if they have one. + * Grouped by file. + */ + std::map positionToDocComment; + LookupPath lookupPath; std::map> lookupPathResolved; @@ -771,6 +779,8 @@ public: std::string_view pathArg, PosIdx pos); + DocComment getDocCommentForPos(PosIdx pos); + private: /** diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 803b5ed8f..4c4c8af19 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -202,6 +202,17 @@ struct ExprSelect : Expr ExprSelect(const PosIdx & pos, Expr * e, AttrPath attrPath, Expr * def) : pos(pos), e(e), def(def), attrPath(std::move(attrPath)) { }; ExprSelect(const PosIdx & pos, Expr * e, Symbol name) : pos(pos), e(e), def(0) { attrPath.push_back(AttrName(name)); }; PosIdx getPos() const override { return pos; } + + /** + * Evaluate the `a.b.c` part of `a.b.c.d`. This exists mostly for the purpose of :doc in the repl. + * + * @param[out] v The attribute set that should contain the last attribute name (if it exists). + * @return The last attribute name in `attrPath` + * + * @note This does *not* evaluate the final attribute, and does not fail if that's the only attribute that does not exist. + */ + Symbol evalExceptFinalSelect(EvalState & state, Env & env, Value & attrs); + COMMON_METHODS }; diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 8dc910468..1df64e73d 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -64,7 +64,7 @@ struct LexerState /** * @brief Maps some positions to a DocComment, where the comment is relevant to the location. */ - std::map positionToDocComment; + std::map & positionToDocComment; PosTable & positions; PosTable::Origin origin; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 3c1bf95c8..a25c6dd87 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -33,6 +33,8 @@ namespace nix { +typedef std::map DocCommentMap; + Expr * parseExprFromBuf( char * text, size_t length, @@ -41,6 +43,7 @@ Expr * parseExprFromBuf( SymbolTable & symbols, const EvalSettings & settings, PosTable & positions, + DocCommentMap & docComments, const ref rootFS, const Expr::AstSymbols & astSymbols); @@ -335,10 +338,12 @@ binds $$ = $1; auto pos = state->at(@2); + auto exprPos = state->at(@4); { auto it = state->lexerState.positionToDocComment.find(pos); if (it != state->lexerState.positionToDocComment.end()) { $4->setDocComment(it->second); + state->lexerState.positionToDocComment.emplace(exprPos, it->second); } } @@ -463,11 +468,13 @@ Expr * parseExprFromBuf( SymbolTable & symbols, const EvalSettings & settings, PosTable & positions, + DocCommentMap & docComments, const ref rootFS, const Expr::AstSymbols & astSymbols) { yyscan_t scanner; LexerState lexerState { + .positionToDocComment = docComments, .positions = positions, .origin = positions.addOrigin(origin, length), }; diff --git a/src/libutil/position.hh b/src/libutil/position.hh index 729f2a523..aba263fdf 100644 --- a/src/libutil/position.hh +++ b/src/libutil/position.hh @@ -7,6 +7,7 @@ #include #include +#include #include "source-path.hh" @@ -65,6 +66,13 @@ struct Pos std::string getSnippetUpTo(const Pos & end) const; + /** + * Get the SourcePath, if the source was loaded from a file. + */ + std::optional getSourcePath() const { + return *std::get_if(&origin); + } + struct LinesIterator { using difference_type = size_t; using value_type = std::string_view; diff --git a/tests/functional/repl/doc-constant.expected b/tests/functional/repl/doc-constant.expected new file mode 100644 index 000000000..9aca06178 --- /dev/null +++ b/tests/functional/repl/doc-constant.expected @@ -0,0 +1,102 @@ +Nix +Type :? for help. +Added variables. + +error: value does not have documentation + +Attribute version + + … defined at + /path/to/tests/functional/repl/doc-comments.nix:29:3 + + Immovably fixed. + +Attribute empty + + … defined at + /path/to/tests/functional/repl/doc-comments.nix:32:3 + + Unchangeably constant. + +error: + … while evaluating the attribute 'attr.undocument' + at /path/to/tests/functional/repl/doc-comments.nix:32:3: + 31| /** Unchangeably constant. */ + 32| lib.attr.empty = { }; + | ^ + 33| + + error: attribute 'undocument' missing + at «string»:1:1: + 1| lib.attr.undocument + | ^ + Did you mean undocumented? + +Attribute constant + + … defined at + /path/to/tests/functional/repl/doc-comments.nix:26:3 + + Firmly rigid. + +Attribute version + + … defined at + /path/to/tests/functional/repl/doc-comments.nix:29:3 + + Immovably fixed. + +Attribute empty + + … defined at + /path/to/tests/functional/repl/doc-comments.nix:32:3 + + Unchangeably constant. + +Attribute undocumented + + … defined at + /path/to/tests/functional/repl/doc-comments.nix:34:3 + + No documentation found. + +error: undefined variable 'missing' + at «string»:1:1: + 1| missing + | ^ + +error: undefined variable 'constanz' + at «string»:1:1: + 1| constanz + | ^ + +error: undefined variable 'missing' + at «string»:1:1: + 1| missing.attr + | ^ + +error: attribute 'missing' missing + at «string»:1:1: + 1| lib.missing + | ^ + +error: attribute 'missing' missing + at «string»:1:1: + 1| lib.missing.attr + | ^ + +error: + … while evaluating the attribute 'attr.undocumental' + at /path/to/tests/functional/repl/doc-comments.nix:32:3: + 31| /** Unchangeably constant. */ + 32| lib.attr.empty = { }; + | ^ + 33| + + error: attribute 'undocumental' missing + at «string»:1:1: + 1| lib.attr.undocumental + | ^ + Did you mean undocumented? + + diff --git a/tests/functional/repl/doc-constant.in b/tests/functional/repl/doc-constant.in new file mode 100644 index 000000000..9c0dde5e1 --- /dev/null +++ b/tests/functional/repl/doc-constant.in @@ -0,0 +1,15 @@ +:l doc-comments.nix +:doc constant +:doc lib.version +:doc lib.attr.empty +:doc lib.attr.undocument +:doc (import ./doc-comments.nix).constant +:doc (import ./doc-comments.nix).lib.version +:doc (import ./doc-comments.nix).lib.attr.empty +:doc (import ./doc-comments.nix).lib.attr.undocumented +:doc missing +:doc constanz +:doc missing.attr +:doc lib.missing +:doc lib.missing.attr +:doc lib.attr.undocumental From cef11b23e89b71f81bc8c4b4b47c3ac87a1a8315 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 9 Jul 2024 19:27:36 +0200 Subject: [PATCH 07/19] Add missing .sh in _NIX_TEST_ACCEPT=1 message --- src/libexpr/eval.cc | 2 +- tests/functional/characterisation/framework.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 81c64ba71..c309e7e98 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -579,7 +579,7 @@ std::optional EvalState::getDoc(Value & v) s << "Function "; } else { - s << "Function **" << name << "**"; + s << "Function `" << name << "`"; if (pos) s << "\\\n … " ; else diff --git a/tests/functional/characterisation/framework.sh b/tests/functional/characterisation/framework.sh index 913fdd967..5ca125ab5 100644 --- a/tests/functional/characterisation/framework.sh +++ b/tests/functional/characterisation/framework.sh @@ -63,7 +63,7 @@ function characterisationTestExit() { echo '' echo 'You can rerun this test with:' echo '' - echo " _NIX_TEST_ACCEPT=1 make tests/functional/${TEST_NAME}.test" + echo " _NIX_TEST_ACCEPT=1 make tests/functional/${TEST_NAME}.sh.test" echo '' echo 'to regenerate the files containing the expected output,' echo 'and then view the git diff to decide whether a change is' From f9243eca75b0847a3984721564cf098baa185698 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 10 Jul 2024 00:48:23 +0200 Subject: [PATCH 08/19] tests/functional/repl.sh: Work around GHA failure --- tests/functional/repl.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/functional/repl.sh b/tests/functional/repl.sh index d356fe096..c1f265df3 100755 --- a/tests/functional/repl.sh +++ b/tests/functional/repl.sh @@ -253,11 +253,21 @@ badExitCode=0 nixVersion="$(nix eval --impure --raw --expr 'builtins.nixVersion' --extra-experimental-features nix-command)" runRepl () { + + # That is right, we are also filtering out the testdir _without underscores_. + # This is crazy, but without it, GHA will fail to run the tests, showing paths + # _with_ underscores in the set -x log, but _without_ underscores in the + # supposed nix repl output. I have looked in a number of places, but I cannot + # find a mechanism that could cause this to happen. + local testDirNoUnderscores + testDirNoUnderscores="${testDir//_/}" + # TODO: pass arguments to nix repl; see lang.sh nix repl 2>&1 \ | stripColors \ | sed \ -e "s@$testDir@/path/to/tests/functional@g" \ + -e "s@$testDirNoUnderscores@/path/to/tests/functional@g" \ -e "s@$nixVersion@@g" \ -e "s@Added [0-9]* variables@Added variables@g" \ | grep -vF $'warning: you don\'t have Internet access; disabling some network-dependent features' \ From 77e9f9ee82db9c77ef67f3916df746383ff451c3 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 11 Jul 2024 12:58:20 +0200 Subject: [PATCH 09/19] libexpr: Get rid of unused line tracking fields --- src/libexpr/lexer-helpers.cc | 1 - src/libexpr/lexer.l | 1 - src/libexpr/parser-state.hh | 6 +++--- src/libexpr/parser.y | 16 +++++++++++++++- 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libexpr/lexer-helpers.cc b/src/libexpr/lexer-helpers.cc index 87c514c71..f7d1017c3 100644 --- a/src/libexpr/lexer-helpers.cc +++ b/src/libexpr/lexer-helpers.cc @@ -4,7 +4,6 @@ void nix::lexer::internal::initLoc(YYLTYPE * loc) { - loc->first_line = loc->last_line = 0; loc->first_column = loc->last_column = 0; } diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index 7d4e6963f..b1b87b96a 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -287,7 +287,6 @@ or { return OR_KW; } \/\*\*[^/*]([^*]|\*+[^*/])*\*+\/ /* doc comments */ { LexerState & lexerState = *yyget_extra(yyscanner); lexerState.docCommentDistance = 0; - lexerState.lastDocCommentLoc.first_line = yylloc->first_line; lexerState.lastDocCommentLoc.first_column = yylloc->first_column; lexerState.lastDocCommentLoc.last_column = yylloc->last_column; } diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 1df64e73d..3e801c13a 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -22,8 +22,8 @@ struct StringToken struct ParserLocation { - int first_line, first_column; - int last_line, last_column; + int first_column; + int last_column; // backup to recover from yyless(0) int stashed_first_column, stashed_last_column; @@ -39,7 +39,7 @@ struct ParserLocation } /** Latest doc comment position, or 0. */ - int doc_comment_first_line, doc_comment_first_column, doc_comment_last_column; + int doc_comment_first_column, doc_comment_last_column; }; struct LexerState diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index a25c6dd87..76190c0fa 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -31,6 +31,21 @@ #define YY_DECL int yylex \ (YYSTYPE * yylval_param, YYLTYPE * yylloc_param, yyscan_t yyscanner, nix::ParserState * state) +// For efficiency, we only track offsets; not line,column coordinates +# define YYLLOC_DEFAULT(Current, Rhs, N) \ + do \ + if (N) \ + { \ + (Current).first_column = YYRHSLOC (Rhs, 1).first_column; \ + (Current).last_column = YYRHSLOC (Rhs, N).last_column; \ + } \ + else \ + { \ + (Current).first_column = (Current).last_column = \ + YYRHSLOC (Rhs, 0).last_column; \ + } \ + while (0) + namespace nix { typedef std::map DocCommentMap; @@ -69,7 +84,6 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char * { if (std::string_view(error).starts_with("syntax error, unexpected end of file")) { loc->first_column = loc->last_column; - loc->first_line = loc->last_line; } throw ParseError({ .msg = HintFmt(error), From 71cb8bf509cf0e180e19230350318e2b453e6dbe Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 11 Jul 2024 13:06:39 +0200 Subject: [PATCH 10/19] libexpr: Rename "column" fields to offset ... because that's what they are. --- src/libexpr/lexer-helpers.cc | 10 +++++----- src/libexpr/lexer.l | 4 ++-- src/libexpr/parser-state.hh | 18 +++++++++--------- src/libexpr/parser.y | 10 +++++----- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/libexpr/lexer-helpers.cc b/src/libexpr/lexer-helpers.cc index f7d1017c3..d9eeb73e2 100644 --- a/src/libexpr/lexer-helpers.cc +++ b/src/libexpr/lexer-helpers.cc @@ -4,7 +4,7 @@ void nix::lexer::internal::initLoc(YYLTYPE * loc) { - loc->first_column = loc->last_column = 0; + loc->beginOffset = loc->endOffset = 0; } void nix::lexer::internal::adjustLoc(yyscan_t yyscanner, YYLTYPE * loc, const char * s, size_t len) @@ -16,15 +16,15 @@ void nix::lexer::internal::adjustLoc(yyscan_t yyscanner, YYLTYPE * loc, const ch if (lexerState.docCommentDistance == 1) { // Preceding token was a doc comment. ParserLocation doc; - doc.first_column = lexerState.lastDocCommentLoc.first_column; + doc.beginOffset = lexerState.lastDocCommentLoc.beginOffset; ParserLocation docEnd; - docEnd.first_column = lexerState.lastDocCommentLoc.last_column; + docEnd.beginOffset = lexerState.lastDocCommentLoc.endOffset; DocComment docComment{lexerState.at(doc), lexerState.at(docEnd)}; PosIdx locPos = lexerState.at(*loc); lexerState.positionToDocComment.emplace(locPos, docComment); } lexerState.docCommentDistance++; - loc->first_column = loc->last_column; - loc->last_column += len; + loc->beginOffset = loc->endOffset; + loc->endOffset += len; } diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index b1b87b96a..58401be8e 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -287,8 +287,8 @@ or { return OR_KW; } \/\*\*[^/*]([^*]|\*+[^*/])*\*+\/ /* doc comments */ { LexerState & lexerState = *yyget_extra(yyscanner); lexerState.docCommentDistance = 0; - lexerState.lastDocCommentLoc.first_column = yylloc->first_column; - lexerState.lastDocCommentLoc.last_column = yylloc->last_column; + lexerState.lastDocCommentLoc.beginOffset = yylloc->beginOffset; + lexerState.lastDocCommentLoc.endOffset = yylloc->endOffset; } diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 3e801c13a..983a17a2e 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -22,20 +22,20 @@ struct StringToken struct ParserLocation { - int first_column; - int last_column; + int beginOffset; + int endOffset; // backup to recover from yyless(0) - int stashed_first_column, stashed_last_column; + int stashedBeginOffset, stashedEndOffset; void stash() { - stashed_first_column = first_column; - stashed_last_column = last_column; + stashedBeginOffset = beginOffset; + stashedEndOffset = endOffset; } void unstash() { - first_column = stashed_first_column; - last_column = stashed_last_column; + beginOffset = stashedBeginOffset; + endOffset = stashedEndOffset; } /** Latest doc comment position, or 0. */ @@ -308,12 +308,12 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos, inline PosIdx LexerState::at(const ParserLocation & loc) { - return positions.add(origin, loc.first_column); + return positions.add(origin, loc.beginOffset); } inline PosIdx ParserState::at(const ParserLocation & loc) { - return positions.add(origin, loc.first_column); + return positions.add(origin, loc.beginOffset); } } diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 76190c0fa..452d265bc 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -36,13 +36,13 @@ do \ if (N) \ { \ - (Current).first_column = YYRHSLOC (Rhs, 1).first_column; \ - (Current).last_column = YYRHSLOC (Rhs, N).last_column; \ + (Current).beginOffset = YYRHSLOC (Rhs, 1).beginOffset; \ + (Current).endOffset = YYRHSLOC (Rhs, N).endOffset; \ } \ else \ { \ - (Current).first_column = (Current).last_column = \ - YYRHSLOC (Rhs, 0).last_column; \ + (Current).beginOffset = (Current).endOffset = \ + YYRHSLOC (Rhs, 0).endOffset; \ } \ while (0) @@ -83,7 +83,7 @@ using namespace nix; void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char * error) { if (std::string_view(error).starts_with("syntax error, unexpected end of file")) { - loc->first_column = loc->last_column; + loc->beginOffset = loc->endOffset; } throw ParseError({ .msg = HintFmt(error), From 8a855296f555fdb5a334acb5f482f3c781e87657 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 14:27:09 +0200 Subject: [PATCH 11/19] tests/function/repl: Characterise the missing doc comment behavior --- src/libexpr/nixexpr.cc | 2 ++ .../repl/doc-comment-curried-args.expected | 24 ++++++++++++++++ .../repl/doc-comment-curried-args.in | 7 +++++ .../repl/doc-comment-formals.expected | 13 +++++++++ tests/functional/repl/doc-comment-formals.in | 3 ++ tests/functional/repl/doc-comments.nix | 27 ++++++++++-------- tests/functional/repl/doc-constant.expected | 28 +++++++++---------- .../repl/doc-lambda-flavors.expected | 8 +++--- .../functional/repl/doc-unambiguous.expected | 2 +- 9 files changed, 83 insertions(+), 31 deletions(-) create mode 100644 tests/functional/repl/doc-comment-curried-args.expected create mode 100644 tests/functional/repl/doc-comment-curried-args.in create mode 100644 tests/functional/repl/doc-comment-formals.expected create mode 100644 tests/functional/repl/doc-comment-formals.in diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 5479dd79e..d54f0b684 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -584,6 +584,8 @@ std::string ExprLambda::showNamePos(const EvalState & state) const } void ExprLambda::setDocComment(DocComment docComment) { + // RFC 145 specifies that the innermost doc comment wins. + // See https://github.com/NixOS/rfcs/blob/master/rfcs/0145-doc-strings.md#ambiguous-placement if (!this->docComment) { this->docComment = docComment; diff --git a/tests/functional/repl/doc-comment-curried-args.expected b/tests/functional/repl/doc-comment-curried-args.expected new file mode 100644 index 000000000..c10c171e1 --- /dev/null +++ b/tests/functional/repl/doc-comment-curried-args.expected @@ -0,0 +1,24 @@ +Nix +Type :? for help. +Added variables. + +Function curriedArgs + … defined at + /path/to/tests/functional/repl/doc-comments.nix:48:5 + + A documented function. + + +"Note that users may not expect this to behave as it currently does" + +Function curriedArgs + … defined at + /path/to/tests/functional/repl/doc-comments.nix:50:5 + + The function returned by applying once + +"This won't produce documentation, because we can't actually document arbitrary values" + +error: value does not have documentation + + diff --git a/tests/functional/repl/doc-comment-curried-args.in b/tests/functional/repl/doc-comment-curried-args.in new file mode 100644 index 000000000..8dbbfc370 --- /dev/null +++ b/tests/functional/repl/doc-comment-curried-args.in @@ -0,0 +1,7 @@ +:l doc-comments.nix +:doc curriedArgs +x = curriedArgs 1 +"Note that users may not expect this to behave as it currently does" +:doc x +"This won't produce documentation, because we can't actually document arbitrary values" +:doc x 2 diff --git a/tests/functional/repl/doc-comment-formals.expected b/tests/functional/repl/doc-comment-formals.expected new file mode 100644 index 000000000..704c0050b --- /dev/null +++ b/tests/functional/repl/doc-comment-formals.expected @@ -0,0 +1,13 @@ +Nix +Type :? for help. +Added variables. + +"Note that this is not yet complete" + +Function documentedFormals + … defined at + /path/to/tests/functional/repl/doc-comments.nix:57:5 + + Finds x + + diff --git a/tests/functional/repl/doc-comment-formals.in b/tests/functional/repl/doc-comment-formals.in new file mode 100644 index 000000000..e32fb8ab1 --- /dev/null +++ b/tests/functional/repl/doc-comment-formals.in @@ -0,0 +1,3 @@ +:l doc-comments.nix +"Note that this is not yet complete" +:doc documentedFormals diff --git a/tests/functional/repl/doc-comments.nix b/tests/functional/repl/doc-comments.nix index a98f1c688..e91ee0b51 100644 --- a/tests/functional/repl/doc-comments.nix +++ b/tests/functional/repl/doc-comments.nix @@ -17,6 +17,7 @@ compact=/**boom*/x: x; + # https://github.com/NixOS/rfcs/blob/master/rfcs/0145-doc-strings.md#ambiguous-placement /** Ignore!!! */ unambiguous = /** Very close */ @@ -41,17 +42,19 @@ # TODO - # /** This returns a documented function. */ - # documentedArgs = - # /** x */ - # x: - # /** y */ - # y: - # /** x + y */ - # x + y; + /** You won't see this. */ + curriedArgs = + /** A documented function. */ + x: + /** The function returned by applying once */ + y: + /** A function body performing summation of two items */ + x + y; - # /** Documented formals */ - # documentedFormals = - # /** x */ - # x: x; + /** Documented formals (but you won't see this comment) */ + documentedFormals = + /** Finds x */ + { /** The x attribute */ + x + }: x; } diff --git a/tests/functional/repl/doc-constant.expected b/tests/functional/repl/doc-constant.expected index 9aca06178..c66558333 100644 --- a/tests/functional/repl/doc-constant.expected +++ b/tests/functional/repl/doc-constant.expected @@ -7,24 +7,24 @@ error: value does not have documentation Attribute version … defined at - /path/to/tests/functional/repl/doc-comments.nix:29:3 + /path/to/tests/functional/repl/doc-comments.nix:30:3 Immovably fixed. Attribute empty … defined at - /path/to/tests/functional/repl/doc-comments.nix:32:3 + /path/to/tests/functional/repl/doc-comments.nix:33:3 Unchangeably constant. error: … while evaluating the attribute 'attr.undocument' - at /path/to/tests/functional/repl/doc-comments.nix:32:3: - 31| /** Unchangeably constant. */ - 32| lib.attr.empty = { }; + at /path/to/tests/functional/repl/doc-comments.nix:33:3: + 32| /** Unchangeably constant. */ + 33| lib.attr.empty = { }; | ^ - 33| + 34| error: attribute 'undocument' missing at «string»:1:1: @@ -35,28 +35,28 @@ error: Attribute constant … defined at - /path/to/tests/functional/repl/doc-comments.nix:26:3 + /path/to/tests/functional/repl/doc-comments.nix:27:3 Firmly rigid. Attribute version … defined at - /path/to/tests/functional/repl/doc-comments.nix:29:3 + /path/to/tests/functional/repl/doc-comments.nix:30:3 Immovably fixed. Attribute empty … defined at - /path/to/tests/functional/repl/doc-comments.nix:32:3 + /path/to/tests/functional/repl/doc-comments.nix:33:3 Unchangeably constant. Attribute undocumented … defined at - /path/to/tests/functional/repl/doc-comments.nix:34:3 + /path/to/tests/functional/repl/doc-comments.nix:35:3 No documentation found. @@ -87,11 +87,11 @@ error: attribute 'missing' missing error: … while evaluating the attribute 'attr.undocumental' - at /path/to/tests/functional/repl/doc-comments.nix:32:3: - 31| /** Unchangeably constant. */ - 32| lib.attr.empty = { }; + at /path/to/tests/functional/repl/doc-comments.nix:33:3: + 32| /** Unchangeably constant. */ + 33| lib.attr.empty = { }; | ^ - 33| + 34| error: attribute 'undocumental' missing at «string»:1:1: diff --git a/tests/functional/repl/doc-lambda-flavors.expected b/tests/functional/repl/doc-lambda-flavors.expected index 4ac52f39e..43f483ce9 100644 --- a/tests/functional/repl/doc-lambda-flavors.expected +++ b/tests/functional/repl/doc-lambda-flavors.expected @@ -4,25 +4,25 @@ Added variables. Function nonStrict … defined at - /path/to/tests/functional/repl/doc-comments.nix:36:70 + /path/to/tests/functional/repl/doc-comments.nix:37:70 My syntax is not strict, but I'm strict anyway. Function strict … defined at - /path/to/tests/functional/repl/doc-comments.nix:37:63 + /path/to/tests/functional/repl/doc-comments.nix:38:63 I don't have to be strict, but I am anyway. Function strictPre … defined at - /path/to/tests/functional/repl/doc-comments.nix:39:48 + /path/to/tests/functional/repl/doc-comments.nix:40:48 Here's one way to do this Function strictPost … defined at - /path/to/tests/functional/repl/doc-comments.nix:40:53 + /path/to/tests/functional/repl/doc-comments.nix:41:53 Here's another way to do this diff --git a/tests/functional/repl/doc-unambiguous.expected b/tests/functional/repl/doc-unambiguous.expected index 0f89a6f64..825aa1ee1 100644 --- a/tests/functional/repl/doc-unambiguous.expected +++ b/tests/functional/repl/doc-unambiguous.expected @@ -4,7 +4,7 @@ Added variables. Function unambiguous … defined at - /path/to/tests/functional/repl/doc-comments.nix:23:5 + /path/to/tests/functional/repl/doc-comments.nix:24:5 Very close From 6bbd493d49fa58aa128a2419db5b1139d5c7a944 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 15:08:41 +0200 Subject: [PATCH 12/19] libcmd/repl-interacter: INT_MAX -> numeric_limits --- src/libcmd/repl-interacter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcmd/repl-interacter.cc b/src/libcmd/repl-interacter.cc index 420cce1db..b285c8a9a 100644 --- a/src/libcmd/repl-interacter.cc +++ b/src/libcmd/repl-interacter.cc @@ -73,7 +73,7 @@ static int listPossibleCallback(char * s, char *** avp) { auto possible = curRepl->completePrefix(s); - if (possible.size() > (INT_MAX / sizeof(char *))) + if (possible.size() > (std::numeric_limits::max() / sizeof(char *))) throw Error("too many completions"); int ac = 0; From 131b6ccc716d1807f4ea91bc3fc1239ff7775648 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 18:04:33 +0200 Subject: [PATCH 13/19] nixexpr.hh: Avoid the warning and pragmas --- src/libexpr/nixexpr.hh | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 4c4c8af19..1bcc962c5 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -36,16 +36,11 @@ struct Value; */ struct DocComment { -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wcomment" // "nested comment start" is intentional - /** - * Start of the comment, including `/**`. + * Start of the comment, including the opening, ie `/` and `**`. */ PosIdx begin; -#pragma GCC diagnostic pop - /** * Position right after the final asterisk and `/` that terminate the comment. */ From 21817473e8035e2231c083c46184724057a30d7f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 19:04:37 +0200 Subject: [PATCH 14/19] Doc comments: use std::unordered_map Co-authored-by: Eelco Dolstra --- src/libexpr/eval.hh | 4 ++-- src/libexpr/parser-state.hh | 2 +- src/libexpr/parser.y | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 3918fb092..d9a3e80bc 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -130,7 +130,7 @@ struct Constant typedef std::map ValMap; #endif -typedef std::map DocCommentMap; +typedef std::unordered_map DocCommentMap; struct Env { @@ -335,7 +335,7 @@ private: * Associate source positions of certain AST nodes with their preceding doc comment, if they have one. * Grouped by file. */ - std::map positionToDocComment; + std::unordered_map positionToDocComment; LookupPath lookupPath; diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 983a17a2e..5a7bcb717 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -64,7 +64,7 @@ struct LexerState /** * @brief Maps some positions to a DocComment, where the comment is relevant to the location. */ - std::map & positionToDocComment; + std::unordered_map & positionToDocComment; PosTable & positions; PosTable::Origin origin; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 452d265bc..8ea176b24 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -48,7 +48,7 @@ namespace nix { -typedef std::map DocCommentMap; +typedef std::unordered_map DocCommentMap; Expr * parseExprFromBuf( char * text, From ac89df815d90eec38935f6c238df8811bd907cf9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 19:23:23 +0200 Subject: [PATCH 15/19] libcmd/repl.cc: Explain evalString call and defend --- src/libcmd/repl.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index fb8106c46..b5d0816dd 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -632,8 +632,13 @@ ProcessLineResult NixRepl::processLine(std::string line) assert(attrs); auto attr = attrs->get(name); if (!attr) { - // Trigger the normal error + // When missing, trigger the normal exception + // e.g. :doc builtins.foo + // behaves like + // nix-repl> builtins.foo + // error: attribute 'foo' missing evalString(arg, v); + assert(false); } if (attr->pos) { fallbackPos = attr->pos; From 6a125e65d0f2dac90cdc69f16be0a4bd888c7a2f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 19:33:56 +0200 Subject: [PATCH 16/19] Revert "Doc comments: use std::unordered_map" hash isn't implemented yet, and I can't cherry-pick a bug-free commit yet. This reverts commit 95529f31e3bbda99111c5ce98a33484dc6e7a462. --- src/libexpr/eval.hh | 4 ++-- src/libexpr/parser-state.hh | 2 +- src/libexpr/parser.y | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index d9a3e80bc..3918fb092 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -130,7 +130,7 @@ struct Constant typedef std::map ValMap; #endif -typedef std::unordered_map DocCommentMap; +typedef std::map DocCommentMap; struct Env { @@ -335,7 +335,7 @@ private: * Associate source positions of certain AST nodes with their preceding doc comment, if they have one. * Grouped by file. */ - std::unordered_map positionToDocComment; + std::map positionToDocComment; LookupPath lookupPath; diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 5a7bcb717..983a17a2e 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -64,7 +64,7 @@ struct LexerState /** * @brief Maps some positions to a DocComment, where the comment is relevant to the location. */ - std::unordered_map & positionToDocComment; + std::map & positionToDocComment; PosTable & positions; PosTable::Origin origin; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 8ea176b24..452d265bc 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -48,7 +48,7 @@ namespace nix { -typedef std::unordered_map DocCommentMap; +typedef std::map DocCommentMap; Expr * parseExprFromBuf( char * text, From ce31a0457f1e2e32d751200bb0a964d4a3e8f253 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 19:55:01 +0200 Subject: [PATCH 17/19] Use HintFmt for doc comments --- src/libcmd/repl.cc | 6 +++--- src/libexpr/eval.cc | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index b5d0816dd..a555fcfcc 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -668,12 +668,12 @@ ProcessLineResult NixRepl::processLine(std::string line) logger->cout(trim(renderMarkdownToTerminal(markdown))); } else if (fallbackPos) { std::stringstream ss; - ss << "Attribute `" << fallbackName << "`\n\n"; - ss << " … defined at " << state->positions[fallbackPos] << "\n\n"; + ss << HintFmt("Attribute '%1%'", fallbackName) << "\n\n"; + ss << HintFmt(" … defined at %1%", state->positions[fallbackPos]) << "\n\n"; if (fallbackDoc) { ss << fallbackDoc.getInnerText(state->positions); } else { - ss << "No documentation found.\n\n"; + ss << HintFmt("No documentation found.") << "\n\n"; } auto markdown = ss.str(); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index c309e7e98..dd3677e39 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -576,17 +576,17 @@ std::optional EvalState::getDoc(Value & v) } if (name.empty()) { - s << "Function "; + s << HintFmt("Function "); } else { - s << "Function `" << name << "`"; + s << HintFmt("Function '%s'", name); if (pos) s << "\\\n … " ; else s << "\\\n"; } if (pos) { - s << "defined at " << pos; + s << HintFmt("defined at %1%", pos); } if (!docStr.empty()) { s << "\n\n"; From 03d33703ef60ec40d8c376e4d935e991fc176294 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 19:55:05 +0200 Subject: [PATCH 18/19] Revert "Use HintFmt for doc comments" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unfortunately these don't render correctly, because they go into the markdown renderer, instead of the terminal. ``` nix-repl> :doc lib.version Attribute '[35;1mversion[0m' … defined at [35;1m/home/user/h/nixpkgs/lib/default.nix:73:40[0m ``` We could switch that to go direct to the terminal, but then we should do the same for the primops, to get a consistent look. Reverting for now. This reverts commit 3413e0338cbee1c7734d5cb614b5325e51815cde. --- src/libcmd/repl.cc | 6 +++--- src/libexpr/eval.cc | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index a555fcfcc..b5d0816dd 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -668,12 +668,12 @@ ProcessLineResult NixRepl::processLine(std::string line) logger->cout(trim(renderMarkdownToTerminal(markdown))); } else if (fallbackPos) { std::stringstream ss; - ss << HintFmt("Attribute '%1%'", fallbackName) << "\n\n"; - ss << HintFmt(" … defined at %1%", state->positions[fallbackPos]) << "\n\n"; + ss << "Attribute `" << fallbackName << "`\n\n"; + ss << " … defined at " << state->positions[fallbackPos] << "\n\n"; if (fallbackDoc) { ss << fallbackDoc.getInnerText(state->positions); } else { - ss << HintFmt("No documentation found.") << "\n\n"; + ss << "No documentation found.\n\n"; } auto markdown = ss.str(); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index dd3677e39..c309e7e98 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -576,17 +576,17 @@ std::optional EvalState::getDoc(Value & v) } if (name.empty()) { - s << HintFmt("Function "); + s << "Function "; } else { - s << HintFmt("Function '%s'", name); + s << "Function `" << name << "`"; if (pos) s << "\\\n … " ; else s << "\\\n"; } if (pos) { - s << HintFmt("defined at %1%", pos); + s << "defined at " << pos; } if (!docStr.empty()) { s << "\n\n"; From 61a4d3d45c91cb19a114796846e5af014f59a6b6 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 15 Jul 2024 20:08:41 +0200 Subject: [PATCH 19/19] getSnippetUpTo: Return optional This makes it possible to certain discern failures from empty snippets, which I think is an ok review comment. Maybe it should do so for swapped column indexes too, but I'm not sure. I don't think it matters in the grand scheme. We don't even have a real use case for `nullopt` now anyway. Since we don't have a use case, I'm not applying this logic to higher level functions yet. --- src/libexpr/nixexpr.cc | 2 +- src/libutil/position.cc | 6 +++--- src/libutil/position.hh | 2 +- tests/unit/libutil/position.cc | 8 +++++--- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index d54f0b684..93c8bdef6 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -646,7 +646,7 @@ size_t SymbolTable::totalSize() const std::string DocComment::getInnerText(const PosTable & positions) const { auto beginPos = positions[begin]; auto endPos = positions[end]; - auto docCommentStr = beginPos.getSnippetUpTo(endPos); + auto docCommentStr = beginPos.getSnippetUpTo(endPos).value_or(""); // Strip "/**" and "*/" constexpr size_t prefixLen = 3; diff --git a/src/libutil/position.cc b/src/libutil/position.cc index 508816d85..3289dbe8b 100644 --- a/src/libutil/position.cc +++ b/src/libutil/position.cc @@ -110,11 +110,11 @@ void Pos::LinesIterator::bump(bool atFirst) input.remove_prefix(eol); } -std::string Pos::getSnippetUpTo(const Pos & end) const { +std::optional Pos::getSnippetUpTo(const Pos & end) const { assert(this->origin == end.origin); if (end.line < this->line) - return ""; + return std::nullopt; if (auto source = getSource()) { @@ -152,7 +152,7 @@ std::string Pos::getSnippetUpTo(const Pos & end) const { } return result; } - return ""; + return std::nullopt; } diff --git a/src/libutil/position.hh b/src/libutil/position.hh index aba263fdf..25217069c 100644 --- a/src/libutil/position.hh +++ b/src/libutil/position.hh @@ -64,7 +64,7 @@ struct Pos bool operator==(const Pos & rhs) const = default; auto operator<=>(const Pos & rhs) const = default; - std::string getSnippetUpTo(const Pos & end) const; + std::optional getSnippetUpTo(const Pos & end) const; /** * Get the SourcePath, if the source was loaded from a file. diff --git a/tests/unit/libutil/position.cc b/tests/unit/libutil/position.cc index d38d2c538..484ecc247 100644 --- a/tests/unit/libutil/position.cc +++ b/tests/unit/libutil/position.cc @@ -25,7 +25,7 @@ TEST(Position, getSnippetUpTo_1) ASSERT_EQ(start.getSnippetUpTo(start), ""); ASSERT_EQ(start.getSnippetUpTo(end), "x"); ASSERT_EQ(end.getSnippetUpTo(end), ""); - ASSERT_EQ(end.getSnippetUpTo(start), ""); + ASSERT_EQ(end.getSnippetUpTo(start), std::nullopt); } { // NOTE: line and column are actually 1-based indexes @@ -37,7 +37,7 @@ TEST(Position, getSnippetUpTo_1) ASSERT_EQ(start.getSnippetUpTo(end), ""); ASSERT_EQ(end.getSnippetUpTo(end), ""); - ASSERT_EQ(end.getSnippetUpTo(start), ""); + ASSERT_EQ(end.getSnippetUpTo(start), std::nullopt); } { Pos start(1, 1, o); @@ -53,7 +53,7 @@ TEST(Position, getSnippetUpTo_1) ASSERT_EQ(start.getSnippetUpTo(start), ""); ASSERT_EQ(start.getSnippetUpTo(end), "x"); ASSERT_EQ(end.getSnippetUpTo(end), ""); - ASSERT_EQ(end.getSnippetUpTo(start), ""); + ASSERT_EQ(end.getSnippetUpTo(start), std::nullopt); } } TEST(Position, getSnippetUpTo_2) @@ -65,6 +65,8 @@ TEST(Position, getSnippetUpTo_2) ASSERT_EQ(start.getSnippetUpTo(start), ""); ASSERT_EQ(start.getSnippetUpTo(end), "a"); ASSERT_EQ(end.getSnippetUpTo(end), ""); + + // nullopt? I feel like changing the column handling would just make it more fragile ASSERT_EQ(end.getSnippetUpTo(start), ""); } {