Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doc comments #11072

Merged
merged 19 commits into from
Jul 15, 2024
Merged

Doc comments #11072

merged 19 commits into from
Jul 15, 2024

Conversation

roberth
Copy link
Member

@roberth roberth commented Jul 9, 2024

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

Priorities and Process

Add 👍 to pull requests you find important.

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

@github-actions 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
@github-actions github-actions bot added the repl The Read Eval Print Loop, "nix repl" command and debugger label Jul 9, 2024
@roberth roberth marked this pull request as draft July 10, 2024 00:15
@roberth roberth marked this pull request as ready for review July 10, 2024 13:02
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Works as expected.

Lexer changes need additional review.
Parser changes look good.

functional + unit tests pass (x86_64-linux)

@roberth
Copy link
Member Author

roberth commented Jul 11, 2024

Lexer changes need additional review.

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

Copy link
Member

@inclyc inclyc left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Would you like to inspect memory impact of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

@@ -110,4 +110,50 @@ void Pos::LinesIterator::bump(bool atFirst)
input.remove_prefix(eol);
}

std::string Pos::getSnippetUpTo(const Pos & end) const {
Copy link
Member

Choose a reason for hiding this comment

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

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

Maybe just parse the doc-comment range with yacc?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

these \, are not aligned

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@roberth
Copy link
Member Author

roberth commented Jul 15, 2024

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
Copy link
Member

inclyc commented Jul 15, 2024

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 Outdated Show resolved Hide resolved
src/libexpr/nixexpr.hh Outdated Show resolved Hide resolved
*/
operator bool() const { return static_cast<bool>(begin); }

std::string getInnerText(const PosTable & positions) const;
Copy link
Member

Choose a reason for hiding this comment

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

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

src/libexpr/eval.cc Outdated Show resolved Hide resolved
* Associate source positions of certain AST nodes with their preceding doc comment, if they have one.
* Grouped by file.
*/
std::map<SourcePath, DocCommentMap> positionToDocComment;
Copy link
Member

Choose a reason for hiding this comment

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

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

(See #11092.)

namespace nix {

typedef std::map<PosIdx, DocComment> DocCommentMap;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
return result;
}
return "";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe return an std::optional to denote failure?

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

Choose a reason for hiding this comment

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

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

src/libcmd/repl.cc Show resolved Hide resolved
@@ -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";
Copy link
Member

Choose a reason for hiding this comment

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

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

etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

roberth and others added 12 commits July 15, 2024 19:56
... because that's what they are.
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
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.
@roberth roberth enabled auto-merge July 15, 2024 18:13
@roberth roberth merged commit c6b5503 into master Jul 15, 2024
19 checks passed
@roberth roberth deleted the doc-comments branch July 15, 2024 19:00
@mightyiam
Copy link
Member

No way

@nixos-discourse
Copy link

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
Copy link
Member

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

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

Choose a reason for hiding this comment

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

missing explicit?

@roberth roberth mentioned this pull request Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors documentation repl The Read Eval Print Loop, "nix repl" command and debugger with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add :doc for lambdas to repl with nix-doc Feature: add :doc command to nix repl
6 participants