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: support __functor #11304

Merged
merged 3 commits into from
Aug 26, 2024
Merged

:doc: support __functor #11304

merged 3 commits into from
Aug 26, 2024

Conversation

roberth
Copy link
Member

@roberth roberth commented Aug 15, 2024

Motivation

Render more docs!

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.

@roberth roberth requested a review from edolstra as a code owner August 15, 2024 11:05
@roberth roberth added the repl The Read Eval Print Loop, "nix repl" command and debugger label Aug 15, 2024
@github-actions github-actions bot added with-tests Issues related to testing. PRs with tests have some priority and removed repl The Read Eval Print Loop, "nix repl" command and debugger labels Aug 15, 2024
Comment on lines +51 to +55
# The RFC might be ambiguous here. The doc comment from makeVeryOverridable
# is "inner" in terms of values, but not inner in terms of expressions.
# Returning the following attribute comment might be allowed.
# TODO: I suppose we could look whether the attribute value expression
# contains a doc, and if not, return the attribute comment anyway?
Copy link
Member Author

Choose a reason for hiding this comment

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

@hsjobeki What do you think?
Currently we follow the disambiguation rule at the value level.
Was that the intent, or should we only apply it at the expression level, so that in cases like this we can resolve to a more specific comment.
Or perhaps more simply, when the :doc query is specifically for an attribute, perhaps we should just prefer the attribute-based lookup?
I'm also considering the option that we should just show everything and treat the rule as a preference or priority.

nix-repl> :doc lib.helper3
Function `__functor`\
  … defined at /path/to/tests/functional/repl/doc-functor.nix:45:23


This is a function that can be overridden.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps more simply, when the :doc query is specifically for an attribute, perhaps we should just prefer the attribute-based lookup?

Yes. But functors are polymorph. I would show (maybe concatenate) both, or add an option for the user to decide what he is interested in.

As a user i would expect :doc lib.helper3 to yield.

:doc lib.helper3
->  Compute x^3

The property that the function is wrapped with makeVeryOverridable is not very interesting.
But i dont know how to solve this issue yet.

Copy link
Contributor

@hsjobeki hsjobeki Aug 15, 2024

Choose a reason for hiding this comment

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

For example this one edge case that i couldn't solve on a value level, because lamdbas are opaque.

If you try to unwrap this at the value level:
pkgs.fetchFromGitHub
-> (position)
https://github.com/nixos/nixpkgs/blob/master/lib/customisation.nix#L134:C17

Maybe we need some annotation for higher order functions like this. (like @wrapper, so we know its just wrapping some inner function that we are interested in)
Or we figure out some way for additional tracking on the expression level.

@hsjobeki
Copy link
Contributor

Great PR! This is exactly what noogle does with functors. I can confirm it works for (most) functors in nixpkgs.
But has some limitations as discussed.

@hsjobeki
Copy link
Contributor

Something i didnt really dig into yet IS the c-api. I could Imagine that someone could build a documentation Generator for nix Code as Independent Project

@roberth
Copy link
Member Author

roberth commented Aug 17, 2024

None of this is exposed in the C API yet. We might need a tier system or something for its interface stability when we add very new things like this, because I don't feel confident that we know what the interface should look like, particularly because the availability of features and their behavior is in part driven by evaluator implementation details. This may or may not be a problem, but it feels premature to make promises about it.

@roberth roberth enabled auto-merge August 26, 2024 14:15
@roberth roberth merged commit 88998fa into NixOS:master Aug 26, 2024
10 checks passed
@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-08-26-nix-team-meeting-minutes-172/51300/1

@hsjobeki
Copy link
Contributor

hsjobeki commented Sep 14, 2024

@roberth I have a partial solution for the lib.makeOverridable problem.

If the wrapped function uses a pattern ({ rev, ... }@args)

Then we can introspect them to find the pattern position. The lambda is just the next lambda node up in the ast.

pkgs.fetchFromGitHub.__functionArgs.sparseCheckout

The tracked attribute position:

 "/nix/store/sm63abmh01jvz9i31xsakc58ys1dmvcp-source/pkgs/build-support/fetchgithub/default.nix#L7:C3",

Alternative solution could be to expose __fOrig in makeOverrideable but we still need to walk in the AST then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants