PoC for RFC145: dynamic documentation for lambdas

[sternenseemann]

Ref. NixOS/rfcs#145.

The first commit is a faithful rebase of #1652. The later commits change the approach to conform with our more modern ideas and also add everything we'd need to hypothetically generate nixpkgs' documentation using a pure Nix expression.

Currently unfinished:

• Rebased nix repl: Provide documentation from comment when evaluating to lambda #1652
• Require /** … */ comments
• Move doc display into :doc command
• Introduced builtins.unsafeGetLambdaDoc
• Introduce builtins.unsafeGetAttrDoc (some work already done)
• Write lib tree walker.

Context

• Closes Inline docstring support #228

[hsjobeki]

I checked out your branch, added two builtins: (Thanks @roberth; @asymmetric for your help, Thx @sternenseemann)

• unsafeGetAttrDoc
• unsafeGetLambdaDoc

-> hsjobeki/nix/lambda-docstring

Almost all test cases are working, and we'll bootstrap a user guide with a bunch of examples here this week.

To support the ( last?) edge case we need: #8968.

For record, this edge case beeing:

A partially applied lambda, where the docstring is behind a pattern argument ({ attr ? <expr>, ...}@arg)
This is arbitrarily complex with the current solution, to solve this we need access to the parent node in the AST.

[ghost]

Five years from now we'll look back on builtins.unsafeGetLambdaDoc the way we look back on Nix function equality today.

... yeah but it seemed like such a useful trick at the time!

[ghost]

This needs to be behind an --experimental flag to prevent it from being used in nixpkgs.

[hsjobeki]

I do know it is a bad implementation. We should definetly not merge it. It was just for playing around with position tracking.

@sternenseemann can you close this PR to prevent people from thinking we should merge it.

[ghost]

I do know it is a bad implementation. We should definetly not merge it. It was just for playing around with position tracking.

Oh I don't think so!

I just want to make sure we don't make promises that can't be kept in the future.

What do you think about the change below? It basically requires that you must use nix-instantiate with --eval and without --read-write-mode in order to introspect on documentation comments. This means you can't produce derivations, but you can do anything else, including dumping out JSON which other tools then consume.

Would this conflict with any of your envisioned use cases?

If so I'd like to talk about those use cases to understand them better.

My two main concerns are:

1. Small concern: philosophical. Comments are supposed to be... comments. Like, changing them doesn't affect the execution of the program. This is a minor concern though.

2. Bigger concern: making sure that we can "compile-out" doc comment support for use on really big build clusters, like Hydra.

If we disallow writing to the store in doc-comment-introspection-mode, then we don't have to worry about the performance impact of doc-comments at all. For high-performance implementations like tvix we can have a #cfg[doc-comments] (basically Rust's version of #ifdef) and compile the interpreter twice: one version that goes really fast and omits any extra fields needed for tracking doc-comments, and another version that can handle doc-comments that are as rich as you like. By not allowing derivations to be created in doc-comments mode we guarantee that nixpkgs can always be built using the high-performance interpreter.

Then you won't have to worry about performance at all when designing this feature. It should give you a lot more freedom.

[ghost]

Suggested change

	state.forceFunction(*args[0], pos, "while evaluating the first argument to builtins.unsafeGetLambdaDoc");
	if (!settings.readOnlyMode) {
	throw Error("attempted to introspect on documentation comments in derivation-producing mode");
	}
	state.forceFunction(*args[0], pos, "while evaluating the first argument to builtins.unsafeGetLambdaDoc");

[hsjobeki]

Yes, I think it should be behind some secure flags. Currently, we might need to rethink the idea with the new builtins.

The new idea would be to have that feature as a nix command (e.g. nix doc --file <lib.nix> --expr "lib.add" )
I am not sure about the implications of that. Potential doc building would involve a step to generate e.g. a HashMap<File: Vec<position>> (using rust notation here)
Where each file has a list of expressions where i want to retrieve doc-comments from. (e.g. lib/lists.nix : [ lib.lists.map, lib.lists.concat ...]
Then i can invoke the new command nix doc.

compile the interpreter twice:
If possible i'd like to have the functionality in a new command (possibly) if we can handle the memory layout to be performant without the command.

[ghost]

Then i can invoke the new command nix doc.

I think that's a great idea. Also the :doc repl command sounds like a great idea too.

After thinking a lot about this, the only place I see trouble would be if we added builtins.unsafeGetDocComment without the settings.readOnlyMode restriction. So, please just keep that in mind if you ever decide to switch back to builtins.unsafeGetDocComment. There's nothing wrong with the builtin; we just need to make sure that it can't affect Hydra builds; that's all.

[roberth]

Hi @sternenseemann,
Thank you for your work on this.
We have merged an alternate, similar implementation some time ago.
I think we'd like to avoid adding primops that retrieve documentation, because we have alternate, in-derivation solutions for this kind of thing now, and doing it at evaluation time risks ossifying their behavior while we aren't quite sure what the best behavior is e.g. when multiple potential comments exist.
Maybe they could be added at some point, but for now it seems like too great a liability to me.
Closing for now.