Doc comments #11072

roberth · 2024-07-09T15:15:13Z

Motivation

This is a basic implementation of rendering RFC 145-style /** documentation */ comments in the repl.

See release note for an example and limitations.

Context

Closes Feature: add :doc command to nix repl #3904
Closes Add :doc for lambdas to repl with nix-doc #10771 (but without nix-doc dependency)
Closes nix repl: Provide documentation from comment when evaluating to lambda #1652
Closes libexpr: support :doc for nixpkgs lambdas #9054

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

tomberek

Works as expected.

Lexer changes need additional review.
Parser changes look good.

functional + unit tests pass (x86_64-linux)

roberth · 2024-07-11T11:09:53Z

Lexer changes need additional review.

I've just added two commits that fix ParserLocation tech debt - should help with the review.

inclyc

Also I kindly ask why other PRs basically get no response?

inclyc · 2024-07-15T05:50:57Z

src/libexpr/eval.hh

+     * Associate source positions of certain AST nodes with their preceding doc comment, if they have one.
+     * Grouped by file.
+     */
+    std::map<SourcePath, DocCommentMap> positionToDocComment;


Would you like to inspect memory impact of this?

I think this is hard to assess until doc comments are used more widely in real code.
Performance is definitely worth considering, but I think for now it's more effective to use tooling to figure out where most memory is spent, because unlikely to be here.

inclyc · 2024-07-15T06:02:06Z

src/libutil/position.hh

+    /**
+     * Get the SourcePath, if the source was loaded from a file.
+     */
+    inline std::optional<SourcePath> getSourcePath() const {


likely you don't need inline because methods defined in structs are implicitly inlined.

inclyc · 2024-07-15T06:03:23Z

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 {


Don't know what's the rationale behind this?

Maybe just parse the doc-comment range with yacc?

This lets us avoid storing copies of the comments in memory.

AFAIK the Pos contains "source" information, I think it's a bit odd to have a range betwern (origin1, position1) and (origin2, position2), what will happen if origin1 != origin2? (Even if we have assert statement to ensure this will not happen, but the code is "semantically" weired.

if indeed we don't want to copy strings how about std::string_views with a dedicated string storage?

It is the exact range which contains the comment. I suppose we could store only the start position, but then we'd have to re-parse the comment to find the end. PosIdx is 4 bytes, so we basically get to store the end for free when used in an 8 byte aligned situation on 64 bit machines.

You're right that there's a redundancy because of the extra origin, but I don't think removing that redundancy would make a substantial difference, and it might make this method harder to use.

how about std::string_views with a dedicated string storage?

That could also work, but I don't think we're already storing all file contents in memory and I'm not sure we should.

That could also work, but I don't think we're already storing all file contents in memory

Currently we not. getSource works with "reading" the file path again (as per my understanding of this codebase).

In my previous career in clang there are SourceManager to store all file contents in memory, that will not consume much (~10MB for a whole nixpkgs). And allocate PosIdx-like handler to reference locations in a source. Accessing files from a filesystem is somehow not very good (just my personal preferences, because I feel there maybe lazytrees to allowing "in-memory" filesystem for nix in the future and current solution may have conflicts about that).

and I'm not sure we should.

I think we should start with a carefully revised version for this feature, otherwise we'll ending up having a lot of workarounds about it (e.g. how about lazytrees?).

Also current implementation does not handle non-ASCII characters right?

We've already merged SourcePath from lazy-trees (in 2.19 iirc?) so that's not a risk anymore.

that will not consume much (~10MB for a whole nixpkgs).

I feel like it'd be more, but you may well be right and that's a very manageable amount of overhead if it's worth the benefits. I'm sure there's more benefits to such an architecture than changing DocComment to the equivalent of string_view, but I don't quite see what they would be, and I think that deserves its own PR. That PR can show its quality by removing this code.

inclyc · 2024-07-15T06:04:25Z

src/libexpr/parser.y

+    do                                                                  \
+      if (N)                                                            \
+        {                                                               \
+          (Current).beginOffset = YYRHSLOC (Rhs, 1).beginOffset;      \


these \, are not aligned

inclyc · 2024-07-15T06:07:01Z

src/libexpr/nixexpr.cc

@@ -581,6 +581,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) {


why setDocComment method cannot "reset" the comment if it already exists?

Removing this would cause the outermost comment to take precedence over the closest one. This behavior conforms with this part of the RFC.

roberth · 2024-07-15T10:35:41Z

Also I kindly ask why other PRs basically get no response?

We try. This PR is the result of Eelco and I pairing on the problem to get a better understanding, and then some individual work by me to make it polished.

Earlier we did not respond much, because the RFC was still in progress, and around the time when it was, we weren't able to because other issues including the governance situation took up our time.

I understand this could feel frustrating to you as someone who's already put time and energy into a solution. Unfortunately doing a good review also takes time, and in this case specifically we're changing the parser, which has rarely been changed during the existence of the Nix team, so we are comparatively inexperienced with it, except for Eelco.

I did acknowledge your work in the release note, but I would like to add that it is helpful to compare solutions, and I think you did good work, and I'd like to thank you for that.

inclyc · 2024-07-15T13:07:48Z

Also I kindly ask why other PRs basically get no response?

We try. This PR is the result of Eelco and I pairing on the problem to get a better understanding, and then some individual work by me to make it polished.

Earlier we did not respond much, because the RFC was still in progress, and around the time when it was, we weren't able to because other issues including the governance situation took up our time.

I understand this could feel frustrating to you as someone who's already put time and energy into a solution. Unfortunately doing a good review also takes time, and in this case specifically we're changing the parser, which has rarely been changed during the existence of the Nix team, so we are comparatively inexperienced with it, except for Eelco.

I did acknowledge your work in the release note, but I would like to add that it is helpful to compare solutions, and I think you did good work, and I'd like to thank you for that.

Thanks! Much appreciate for pushing this work ❤️

doc/manual/rl-next/repl-doc-renders-doc-comments.md

src/libexpr/nixexpr.hh

edolstra · 2024-07-15T12:48:43Z

src/libexpr/nixexpr.hh

+     */
+    operator bool() const { return static_cast<bool>(begin); }
+
+    std::string getInnerText(const PosTable & positions) const;


Add a doc, especially to say whether this reads the comment from disk.

src/libexpr/eval.cc

edolstra · 2024-07-15T12:54:57Z

src/libexpr/eval.hh

+     * Associate source positions of certain AST nodes with their preceding doc comment, if they have one.
+     * Grouped by file.
+     */
+    std::map<SourcePath, DocCommentMap> positionToDocComment;


Suggested change

std::map<SourcePath, DocCommentMap> positionToDocComment;

std::unordered_map<SourcePath, DocCommentMap> positionToDocComment;

(See #11092.)

edolstra · 2024-07-15T13:10:21Z

src/libexpr/parser.y

 namespace nix {

+typedef std::map<PosIdx, DocComment> DocCommentMap;


Suggested change

typedef std::map<PosIdx, DocComment> DocCommentMap;

typedef std::unordered_map<PosIdx, DocComment> DocCommentMap;

Almost! I've prepared this change, but had to revert for now 6a125e6

edolstra · 2024-07-15T13:16:06Z

src/libutil/position.cc

+        }
+        return result;
+    }
+    return "";


Maybe return an std::optional to denote failure?

edolstra · 2024-07-15T13:16:40Z

src/libutil/position.hh

+    /**
+     * Get the SourcePath, if the source was loaded from a file.
+     */
+    inline std::optional<SourcePath> getSourcePath() const {


Suggested change

inline std::optional<SourcePath> getSourcePath() const {

std::optional<SourcePath> getSourcePath() const

{

src/libcmd/repl.cc

edolstra · 2024-07-15T13:26:19Z

src/libcmd/repl.cc

@@ -631,6 +659,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";


Suggested change

ss << "Attribute `" << fallbackName << "`\n\n";

ss << fmt("Attribute `%s`\n\n", fallbackName);

etc.

I've tried this, but unfortunately it doesn't work, as these 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.

Could be done in a follow-up and I've left a reverted commit to cherry pick.

... because that's what they are.

Co-authored-by: Eelco Dolstra <[email protected]>

hash<SourcePath> isn't implemented yet, and I can't cherry-pick a bug-free commit yet. This reverts commit 95529f31e3bbda99111c5ce98a33484dc6e7a462.

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.

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.

mightyiam · 2024-07-16T01:02:31Z

No way

nixos-discourse · 2024-07-17T21:10:33Z

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

https://discourse.nixos.org/t/2024-07-15-nix-team-meeting-minutes-161/49228/1

edolstra · 2024-07-23T14:05:34Z

@roberth This broke the buildReadlineNoMarkdown.x86_64-linux test.

inclyc · 2024-08-19T14:38:12Z

src/libexpr/nixexpr.hh

+     * therefore baking optionality into it is also useful, to avoiding the memory
+     * overhead of `std::optional`.
+     */
+    operator bool() const { return static_cast<bool>(begin); }


missing explicit?

roberth requested review from fricklerhandwerk and edolstra as code owners July 9, 2024 15:15

github-actions bot added documentation contributor-experience Developer experience for Nix contributors with-tests Issues related to testing. PRs with tests have some priority labels Jul 9, 2024

roberth force-pushed the doc-comments branch from 3800736 to 1b52aa4 Compare July 9, 2024 17:33

github-actions bot added the repl The Read Eval Print Loop, "nix repl" command and debugger label Jul 9, 2024

roberth marked this pull request as draft July 10, 2024 00:15

roberth force-pushed the doc-comments branch from 3b9b4f8 to 6f00c43 Compare July 10, 2024 13:02

roberth marked this pull request as ready for review July 10, 2024 13:02

tomberek approved these changes Jul 11, 2024

View reviewed changes

roberth mentioned this pull request Jul 11, 2024

SymbolStr: Remove std::string conversion #11082

Merged

roberth force-pushed the doc-comments branch 3 times, most recently from 2177f47 to 8838d8a Compare July 14, 2024 10:28

roberth mentioned this pull request Jul 14, 2024

Pretty print idempotently #11100

Merged

inclyc reviewed Jul 15, 2024

View reviewed changes

roberth force-pushed the doc-comments branch from 8838d8a to e8b8a50 Compare July 15, 2024 12:29

edolstra approved these changes Jul 15, 2024

View reviewed changes

roberth added 2 commits July 15, 2024 18:41

doc/testing: Typo

63f520f

libutil: Add Pos::getSnippetUpTo(Pos)

e5af7cb

roberth force-pushed the doc-comments branch from e8b8a50 to 5f503ed Compare July 15, 2024 16:44

roberth added 4 commits July 15, 2024 19:56

Track doc comments and render them in :doc

7fae378

libexpr: Rearrange lexer files so that yylex_init_extra can be found

e68234c

Refactor: extract DocComment::getInnerText(PosTable)

491b9cf

nix repl: Render docs for attributes

d4f576b

roberth and others added 12 commits July 15, 2024 19:56

Add missing .sh in _NIX_TEST_ACCEPT=1 message

cef11b2

tests/functional/repl.sh: Work around GHA failure

f9243ec

libexpr: Get rid of unused line tracking fields

77e9f9e

libexpr: Rename "column" fields to offset

71cb8bf

... because that's what they are.

tests/function/repl: Characterise the missing doc comment behavior

8a85529

libcmd/repl-interacter: INT_MAX -> numeric_limits

6bbd493

nixexpr.hh: Avoid the warning and pragmas

131b6cc

Doc comments: use std::unordered_map

2181747

Co-authored-by: Eelco Dolstra <[email protected]>

libcmd/repl.cc: Explain evalString call and defend

ac89df8

Revert "Doc comments: use std::unordered_map"

6a125e6

hash<SourcePath> isn't implemented yet, and I can't cherry-pick a bug-free commit yet. This reverts commit 95529f31e3bbda99111c5ce98a33484dc6e7a462.

Use HintFmt for doc comments

ce31a04

roberth force-pushed the doc-comments branch from 5f503ed to 03d3370 Compare July 15, 2024 17:56

roberth enabled auto-merge July 15, 2024 18:13

roberth merged commit c6b5503 into master Jul 15, 2024
19 checks passed

roberth deleted the doc-comments branch July 15, 2024 19:00

roberth mentioned this pull request Jul 16, 2024

Doc comments: use std::unordered_map #11113

Merged

roberth mentioned this pull request Jul 23, 2024

Fix repl test for buildReadlineNoMarkdown #11167

Merged

roberth mentioned this pull request Aug 15, 2024

:doc: support __functor #11304

Merged

inclyc reviewed Aug 19, 2024

View reviewed changes

roberth mentioned this pull request Aug 22, 2024

parser rewrite #9989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doc comments #11072

Doc comments #11072

roberth commented Jul 9, 2024

tomberek left a comment

roberth commented Jul 11, 2024

inclyc left a comment

inclyc Jul 15, 2024

roberth Jul 15, 2024

inclyc Jul 15, 2024

inclyc Jul 15, 2024

roberth Jul 15, 2024

inclyc Jul 15, 2024

roberth Jul 15, 2024

inclyc Jul 15, 2024

roberth Jul 15, 2024

inclyc Jul 15, 2024

inclyc Jul 15, 2024

roberth Jul 15, 2024

roberth commented Jul 15, 2024

inclyc commented Jul 15, 2024

edolstra Jul 15, 2024

edolstra Jul 15, 2024

edolstra Jul 15, 2024

roberth Jul 15, 2024

edolstra Jul 15, 2024

edolstra Jul 15, 2024

edolstra Jul 15, 2024

roberth Jul 15, 2024

mightyiam commented Jul 16, 2024

nixos-discourse commented Jul 17, 2024

edolstra commented Jul 23, 2024

inclyc Aug 19, 2024

	std::map<SourcePath, DocCommentMap> positionToDocComment;
	std::unordered_map<SourcePath, DocCommentMap> positionToDocComment;

		namespace nix {

		typedef std::map<PosIdx, DocComment> DocCommentMap;

	typedef std::map<PosIdx, DocComment> DocCommentMap;
	typedef std::unordered_map<PosIdx, DocComment> DocCommentMap;

	inline std::optional<SourcePath> getSourcePath() const {
	std::optional<SourcePath> getSourcePath() const
	{

	ss << "Attribute `" << fallbackName << "`\n\n";
	ss << fmt("Attribute `%s`\n\n", fallbackName);

Doc comments #11072

Doc comments #11072

Conversation

roberth commented Jul 9, 2024

Motivation

Context

Priorities and Process

tomberek left a comment

Choose a reason for hiding this comment

roberth commented Jul 11, 2024

inclyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth commented Jul 15, 2024

inclyc commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyiam commented Jul 16, 2024

nixos-discourse commented Jul 17, 2024

edolstra commented Jul 23, 2024

Choose a reason for hiding this comment