Skip to content

libexpr: support :doc for nixpkgs lambdas#9054

Closed
inclyc wants to merge 1 commit intoNixOS:masterfrom
inclyc:users/inclyc/doc
Closed

libexpr: support :doc for nixpkgs lambdas#9054
inclyc wants to merge 1 commit intoNixOS:masterfrom
inclyc:users/inclyc/doc

Conversation

@inclyc
Copy link
Member

@inclyc inclyc commented Sep 27, 2023 •
edited
Loading

Motivation

This is a proof-of-conecpt PR that implements NixOS/rfcs#145. (lambdas only, but we can easily add more types.)

Currently it can display functions like lib.isList, lib.id

 Welcome to Nix 2.19.0. Type :? for help.

nix-repl> lib = (import <nixpkgs> { }).lib              

nix-repl> :doc lib.id                      
/* The identity function For when you need a function that does “nothing”.

    |  Type: id :: a -> a

    */

nix-repl> lib.id
«lambda @ /nix/store/cz973qsjldbw4x7fx0rwcvhr9k645vz2-source/lib/trivial.nix:14:5»

nix-repl> :doc lib.const
/* The constant function

    |  Ignores the second argument. If called with only one argument,
    |  constructs a function that always returns a static value.
    | 
    |  Type: const :: a -> b -> a
    |  Example:
    |    let f = const 5; in f 10
    |    => 5

    */

nix-repl> :doc lib.concat
/* Concatenate two lists

    |  Type: concat :: [a] -> [a] -> [a]
    | 
    |  Example:
    |    concat [ 1 2 ] [ 3 4 ]
    |    => [ 1 2 3 4 ]

    */

nix-repl>

image
image

Context

Related-to: #5527 #1652
Fixes: #3904
Link: NixOS/rfcs#145

Priorities

Add 👍 to pull requests you find important.

@inclyc inclyc requested a review from edolstra as a code owner September 27, 2023 13:29
@inclyc inclyc marked this pull request as draft September 27, 2023 13:37
hsjobeki
hsjobeki reviewed Sep 27, 2023
View reviewed changes
hsjobeki
hsjobeki reviewed Sep 27, 2023
View reviewed changes
src/libexpr/lexer.l
});
}

\/\*([^*]|\*+[^*/])*\*+\/ {
Copy link
Contributor

@hsjobeki hsjobeki Sep 27, 2023

Choose a reason for hiding this comment

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

doc-comments per rfc145 start with /** */, but this is nice though for testing against nixpkgs.lib

hsjobeki
hsjobeki reviewed Sep 27, 2023
View reviewed changes
src/libexpr/parser.y
PosTable::Origin origin;
std::optional<ErrorInfo> error;
std::vector<Comment> comments;
std::map<nix::Expr *, Comment> attachedComments;
Copy link
Contributor

@hsjobeki hsjobeki Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if Comments is a vector, does this mean there can be multiple comments attached to one expr?
We could simplify here because doc-comments are multiline comments only und thus only this is a one-to-one relation

@inclyc inclyc force-pushed the users/inclyc/doc branch 3 times, most recently from 8055ecd to dcef056 Compare September 28, 2023 02:57
@inclyc inclyc marked this pull request as ready for review October 5, 2023 05:18
@inclyc
Copy link
Member Author

inclyc commented Oct 5, 2023

Note: this PR is not ready to merge but I want to let the nix team review this approach. It is attaching expressions in the semantic action to nix::Expr *, and there are some rules about whether or not we can attach the comment.

Currently we have many POC impls of NixOS/rfcs#145.

  • get the location by nix::ExprLambda * ->getPos() and unsafeGetAttrPos (position stored in AttrName), and parse the file again, extract the comment using regex match, or external parser (e.g. rnix).

    • (+) it requires less overhead
    • (-) theoretically we can not parse a grammar using a Deterministic Finite Automata (DFA), so it will be more tricky & error prone
    • (-) we do not want to introduce other external dependency, for native implementation

  • use annotations, as mentioned by @infinisil , so that we do not need to hack comments.

    • (+) easy to implement
    • (-) not user-friendly to read code with many builtins.withAnnotation
    • (-) hard to migrate from existing nixpkgs code

  • hack comments handling in the lexer, & parser this approach1.

    • (-) more overhead, but we can reduce this by introducing a switch into eval-settings.hh
    • (+) less complicated with consistency, because we handling comments in lexer & parser directly

We need to access the buffer if it is needed to check whitespaces, the RFC specified that there are only whitespaces allowed between comments & expression. What is the correct way to do such thing?

Footnotes

  1. FWIW, in C++ world this is the same mechanism for clang's -Wdocumentation, it is commonly used technique that treat comments are special tokens, and handling the location in semantic actions (this is hand-written in clang). The difference between this PR and clang is, we will not parse the comment again using special syntax for comments. ref: https://llvm.org/devmtg/2012-11/Gribenko_CommentParsing.pdf ↩

@hsjobeki hsjobeki mentioned this pull request Oct 12, 2023
Merged
@inclyc inclyc mentioned this pull request Oct 18, 2023
Open
@SomeoneSerge SomeoneSerge mentioned this pull request Mar 11, 2024
Merged
13 tasks
@roberth roberth mentioned this pull request Jun 10, 2024
Closed
@roberth roberth mentioned this pull request Jul 9, 2024
Merged
@inclyc inclyc deleted the users/inclyc/doc branch July 16, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

1 more reviewer

@hsjobeki hsjobeki hsjobeki left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature: add :doc command to nix repl

2 participants

@inclyc @hsjobeki