Examples in NixOS options manual do not escape attribute names

[brendon-boldt]

Problem

Attribute sets in the manual's examples do not properly escape keys when are, for example, paths. For the "Example" for an option, we see { /home/user = "foo"; } which is invalid Nix code instead of { "/home/user" = "foo"; } which is valid Nix code. This happens when the Nix options get builtins.toJSON run on them during the manual generation. In the wild, look at /mnt/btr_pool in the example of this option. Maybe the general opinion on this is, "obviously attrset keys can't be paths", but it tripped me up for a few minutes as a newcomer, and I think it could be a nice quality of life improvement.

Checklist

• checked latest Nixpkgs manual (source) and latest NixOS manual (source)
• checked open documentation issues for possible duplicates
• checked open documentation pull requests for possible solutions

Proposal

Changing toJSON in Nix seems inappropriate, so maybe I could open a PR with a Nix function which escapes the problem attributes, but that might have performance issues. I am open to suggestions (or just marking it as "won't fix").

[ncfavier]

Good catch! Definitely not a WONTFIX. This also affects https://search.nixos.org. The places to look at are

nixpkgs/nixos/lib/make-options-doc/options-to-docbook.xsl

Line 205 in 671d37a

<xsl:value-of select="@name" />

and https://github.com/NixOS/nixos-search/blob/e7ceafb0d4fa3c570bbf06918db75760fba7f384/flake-info/src/data/prettyprint.rs#L68. The logic should adhere to that of lib.strings.escapeNixIdentifier.

cc @pennae @fricklerhandwerk

[pennae]

The logic should adhere to that of lib.strings.escapeNixIdentifier.

agreed. should probably be done in substSpecial to solve the problem for all renderers at once. (options.json contains the examples as json objects, having the keys formatted properly there already seems preferrable over other solutions)

[ncfavier]

to solve the problem for all renderers at once

+1000. If I understand correctly, you're suggesting that non-_typed defaults and examples be rendered as literalExpressions by the time they reach options.json? This seems perfect as there is no transition needed for out-of-tree renderers (since they already should support literalExpressions), and they can progressively drop support for non-_type values if we make this a guarantee.

[pennae]

If I understand correctly, you're suggesting that non-_typed defaults and examples be rendered as literalExpressions by the time they reach options.json?

that would be one way to do it. might be nicer to not fix non-literal examples that much though and only escape the attribute names where necessary, essentially justifying the current assumption that attr names of examples are safe to emit as-is. not making everything literal immediately would need less processing in nix and give greater flexibility for renderers. (in fact we may want to at some point push down literalExpression nodes to the places they are needed instead of providing examples that use them as opaque strings)

[ncfavier]

I am not really in favour of introducing a new ad-hoc format ("JSON with possibly Nix-escaped keys") for something publicly exported like options.json. For a similar reason, I am not in favour of pushing literalExpression down into regular values (creating another ad-hoc format). I think "regular value or top-level _type" is ad-hoc enough. I am also not sure that flexibility in renderers is a goal worth pursuing; maybe some uniformity would be nice, along with avoiding duplicated work. Minimising Nix processing is a valid concern though.

cc @roberth

[roberth]

I am not really in favour of introducing a new ad-hoc format ("JSON with possibly Nix-escaped keys") for something publicly exported like options.json.

The examples are, in general, Nix expressions, a data type that does not have a structured representation in JSON.¹
The fact that some, or even many, examples can be represented faithfully using toJSON semantics is irrelevant.²

Hence, the only representation exported by the module system should have its defaults represented by Nix strings containing expressions, which are subsequently escaped because that's how JSON represents its strings. Any consumers of this JSON will treat the JSON escapes according to spec, so that those consumers work with strings containing Nix expressions, as intended.

Attribute sets in the manual's examples do not properly escape keys when are, for example, paths.

Having established how we'll represent our data types, the rest is "boring".
A string representation of the Nix expressions can be acquired with lib.generators.toPretty {}; for example:

nix-repl> lib.generators.toPretty {} { "a.b" = true; "/home/user" = "foo"; }
"{\n  \"/home/user\" = \"foo\";\n  \"a.b\" = true;\n}"

¹: We could describe the AST structure in JSON, but that's besides the point.
²: unless we have a use case for specifically these examples and not the others, which I'm fairly confident we don't have. Even then, those use cases should be served by a separate attr, because the majority of use needs the natural string representation of expressions anyway.

[ncfavier]

Sounds about right. And since we also allow defaults/examples (I don't see any reason to treat defaults differently from examples in this matter) to be documentation strings instead of Nix expressions, we need _type to tell them apart, so we are back to my suggestion above.

[pennae]

And since we also allow defaults/examples (I don't see any reason to treat defaults differently from examples in this matter) to be documentation strings instead of Nix expressions

defaults have semantics, examples do not. there's defaultText for defaults that are not fully printable or have non-constant subexpressions.

The examples are, in general, Nix expressions, a data type that does not have a structured representation in JSON.

if the expression involves lambdas or non-constant subexpressions (like accesses to config or any kind of call). for those cases literalExpression is currently the only choice anyway, the others can be written out as json just fine (if you're not picky about ind-strings being reformatted). converting everything into a docstring on export seems very unnecessary considering that we have close to 1000 list-or-attrs examples in nixpkgs and only about 50 that use literalExpression.

[roberth]

converting everything into a docstrong on export seems very unnecessary

Likewise, having multiple longer code paths and a more complicated interface is unnecessary.
The "remainder" of those two statements is that performance of toPretty may be worse. That's a valid reason to come up with something more complicated than necessary. The goal of my comment was to provide a simple solution. If that's not good enough, we can settle for a fast and well-tested solution.