CONTRIBUTING: establish builtins prefix convention for prelude

[trueNAHO]

Establish the builtins prefix convention for prelude attributes to
progress [1] ("Unwritten style guides should be written"), following the
treewide removal of the builtins prefix for prelude attributes [2] [3]
[4] [5] [6].

[1]: https://github.com/NixOS/nixpkgs/issues/387072
[2]: https://github.com/NixOS/nixpkgs/pull/444432
[3]: https://github.com/NixOS/nixpkgs/pull/447401
[4]: https://github.com/NixOS/nixpkgs/pull/447402
[5]: https://github.com/NixOS/nixpkgs/pull/447403
[6]: https://github.com/NixOS/nixpkgs/pull/447404

Unlike removing builtins prefixes, adding them back in a follow-up PR should be fully automatable, including updates to code comments and documentation.

As pointed out in #447402 (comment), it seems inconsistent that builtins.fromTOML is in the prelude while builtins.toJSON is not. I suppose this stems from historical reasons to avoid breaking language changes.

Beyond establishing builtins prefix conventions, this proposal should decide whether to add more attributes to the prelude or discourage existing ones. For example, should builtins.fromJSON be added, or should fromTOML be discouraged in favor of builtins.fromTOML? Since expanding the prelude increases language complexity, this requires careful consideration.

Although this proposal is a non-functional change, it has far-reaching consequences for the Nix style guide. I am pinging several people to raise awareness:

• @NixOS/nix-team
• @NixOS/nixpkgs-vet
• statix: @mightyiam, @oppiliappan

Considering the far-reaching consequences of this proposal, should this be elevated to an RFC, or is a PR discussion fine?

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[trueNAHO]

The NixOS/nix#14015 change is not yet visible on this link. Is this the correct one?

[philiptaron]

It'll be part of the next release of Nix.

[trueNAHO]

This is written as a placeholder to make it easier to later decide what should be added to each category.

For convenience, here is the rendered table:

Use builtins	Omit builtins
builtins.abort builtins.baseNameOf builtins.break builtins.derivation builtins.derivationStrict builtins.dirOf builtins.false builtins.fetchGit builtins.fetchMercurial builtins.fetchTarball builtins.fetchTree builtins.fromTOML builtins.import builtins.isNull builtins.map builtins.null builtins.placeholder builtins.removeAttrs builtins.scopedImport builtins.throw builtins.toString builtins.true	abort baseNameOf break derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true

[philiptaron]

This isn't the correct "Use builtins" list. Here's what I get:

Save this as t.nix:

builtins.attrNames (removeAttrs builtins [ "abort" "baseNameOf" "break" "derivation" "derivationStrict" "dirOf" "false" "fetchGit" "fetchMercurial" "fetchTarball" "fetchTree" "fromTOML" "import" "isNull" "map" "null" "placeholder" "removeAttrs" "scopedImport" "throw" "toString" "true" ])

Run it with nix-instantiate --strict --json --eval t.nix | jq -r '.[] | "1. " + .':

1. add
2. addDrvOutputDependencies
3. addErrorContext
4. all
5. any
6. appendContext
7. attrNames
8. attrValues
9. bitAnd
10. bitOr
11. bitXor
12. builtins
13. catAttrs
14. ceil
15. compareVersions
16. concatLists
17. concatMap
18. concatStringsSep
19. convertHash
20. currentSystem
21. currentTime
22. deepSeq
23. div
24. elem
25. elemAt
26. fetchurl
27. filter
28. filterSource
29. findFile
30. flakeRefToString
31. floor
32. foldl'
33. fromJSON
34. functionArgs
35. genList
36. genericClosure
37. getAttr
38. getContext
39. getEnv
40. getFlake
41. groupBy
42. hasAttr
43. hasContext
44. hashFile
45. hashString
46. head
47. intersectAttrs
48. isAttrs
49. isBool
50. isFloat
51. isFunction
52. isInt
53. isList
54. isPath
55. isString
56. langVersion
57. length
58. lessThan
59. listToAttrs
60. mapAttrs
61. match
62. mul
63. nixPath
64. nixVersion
65. parseDrvName
66. parseFlakeRef
67. partition
68. path
69. pathExists
70. readDir
71. readFile
72. readFileType
73. replaceStrings
74. seq
75. sort
76. split
77. splitVersion
78. storeDir
79. storePath
80. stringLength
81. sub
82. substring
83. tail
84. toFile
85. toJSON
86. toPath
87. toXML
88. trace
89. traceVerbose
90. tryEval
91. typeOf
92. unsafeDiscardOutputDependency
93. unsafeDiscardStringContext
94. unsafeGetAttrPos
95. warn
96. zipAttrsWith

[philiptaron]

Maybe I misunderstood your table, though.

[trueNAHO]

This isn't the correct use builtins list. Here's what I get:

Save this as t.nix:

builtins.attrNames (removeAttrs builtins [ "abort" "baseNameOf" "break" "derivation" "derivationStrict" "dirOf" "false" "fetchGit" "fetchMercurial" "fetchTarball" "fetchTree" "fromTOML" "import" "isNull" "map" "null" "placeholder" "removeAttrs" "scopedImport" "throw" "toString" "true" ])

[...]

Maybe I misunderstood your table, though.

Yes, sorry for the confusing placeholder template and not providing any real explanations.

The idea was to declare which prelude attributes go into the "Use builtins" or "Omit builtins" category. For example, if true is preferred over builtins.true, then true would be added to the "Omit builtins" category and would be removed from "Use builtins":

Use builtins	Omit builtins
	true

For conciseness, the table should only focus on prelude attributes and ignore the other builtins that require the builtins prefix. With that in mind, feel free to propose better table formats.

Currently, both categories contain all prelude attributes to make it easier to remove them from each category later on.

See #449123 (comment) for a real table example.

[MattSturgeon]

Although this proposal is a non-functional change, it has far-reaching consequences for the Nix style guide. I am pinging several people to raise awareness:

Let's ping @NixOS/nix-formatting too; although focused on formatting nix code, some members may have an interest in style guides too.

Since you pinged statix, let's cc the nixf-diagnose maintainer too @inclyc

Considering the far-reaching consequences of this proposal, should this be elevated to an RFC, or is a PR discussion fine?

Most things can be decided in issues and PRs. That's even more true now we have a @NixOS/nixpkgs-core team who can make controversial decisions when necessary.

RFCs are only useful when we need to have complex and in depth discussion over a long drawn out period of time. Also, they are only effective when someone is able to proactively steer the RFC towards a conclusion.

---

Personally, I'd like to avoid introducing trivial style rules that need to be enforced manually by authors and reviews.* They usually result in nit-picky reviews that frustrate both the reviewer themselves and the PR author. Unless there's a good reason to add specific guidance, I strongly prefer having linters and formatters define and enforce the style guide where possible.

*Of course, with enough effort this could be enforced by a linter. I'm not opposed to discussing what the ideal style should be, assuming there is one; that way someone can go off and implement linting rules for that ideal style.

[trueNAHO]

Personally, I'd like to avoid introducing trivial style rules that need to be enforced manually by authors and reviews.* [...]

*Of course, with enough effort this could be enforced by a linter. I'm not opposed to discussing what the ideal style should be, assuming there is one; that way someone can go off and implement linting rules for that ideal style.

Agreed.

Implementing this in statix has already been proposed in oppiliappan/statix#140. IIUC, https://github.com/oppiliappan/statix/milestone/1 would allow enforcing this in CI with statix.

[inclyc]

let's cc the nixf-diagnose maintainer too @inclyc

nix-community/nixd#721

[philiptaron]

I'm in favor of this on purely "it's shorter" grounds. Now that it's documented as part of upstream Nix, it's not just esoteric knowledge. But it's also not a huge deal.

[wolfgangwalther]

We should only make the contribution guides even longer if we can't encode something as a linter rule or so (yet).

AFAICT, this has been implemented in nix-community/nixd#721. We already have nixf-diagnose in CI now, so this is just a question of updating nixd in nixpkgs, waiting for it to reach the channel, updating the pin, running it via nixf-diagnose.

From what I can tell, this also supports auto-fix.

Thus, I think we shouldn't do any of this PR.

[trueNAHO]

AFAICT, this has been implemented in nix-community/nixd#721. We already have nixf-diagnose in CI now, so this is just a question of updating nixd in nixpkgs, waiting for it to reach the channel, updating the pin, running it via nixf-diagnose.

[...]

Thus, I think we shouldn't do any of this PR.

If it merely enforces the treewide removal of the builtins prefix for all prelude attributes, then the main point of this proposal has not been addressed yet:

Beyond establishing builtins prefix conventions, this proposal should decide whether to add more attributes to the prelude or discourage existing ones. For example, should builtins.fromJSON be added, or should fromTOML be discouraged in favor of builtins.fromTOML? Since expanding the prelude increases language complexity, this requires careful consideration.

For example, following up on the #449123 (comment) discussion, one possible prelude convention could be:

Use builtins	Omit builtins
builtins.baseNameOf builtins.derivation builtins.derivationStrict builtins.dirOf builtins.fetchGit builtins.fetchMercurial builtins.fetchTarball builtins.fetchTree builtins.fromTOML builtins.isNull builtins.placeholder builtins.removeAttrs builtins.scopedImport	abort break false import map null throw toString true

For example, builtins.baseNameOf should always use the builtins prefix, while abort should never. That means, all prelude attributes in the "Use builtins" category should ideally be removed from the prelude, although this should practically not be done any time soon.

Furthermore, we should carefully consider extending the prelude:

As pointed out in #447402 (comment), it seems inconsistent that builtins.fromTOML is in the prelude while builtins.toJSON is not. I suppose this stems from historical reasons to avoid breaking language changes.

Enforcing the convention this proposal is trying to establish can be done at a later stage:

with enough effort this could be enforced by a linter. I'm not opposed to discussing what the ideal style should be, assuming there is one; that way someone can go off and implement linting rules for that ideal style.

[wolfgangwalther]

Beyond establishing builtins prefix conventions, this proposal should decide whether to add more attributes to the prelude or discourage existing ones.

This is certainly out of scope for Nixpkgs. This needs to be done in Nix - and also Lix. Before any of that could be relevant for Nixpkgs, that would take a few years, because we keep compatibility here.

The most consistent and least controversial thing we can do is to enforce removal of the builtins prefix for all builtins that are available globally in both Nix and Lix since 2.18. That's what the nixd rule does, IIUC. With that implementation in mind, it makes no sense to discuss or commit a written rule that is different to that and would be superseded in a matter of days or weeks.

We could also force the addition of the builtins prefix for all others, but I'm not sure whether there are any relevant cases at all.

[eclairevoyant]

all builtins that are available globally in both Nix and Lix since 2.18

I commented this on the nixf PR as well, but for coherence I'll mention it here:

Every globally-exposed primop (not prefixed with __) has been present since nix 2.3, except for

• break (added 3 years ago in --debugger flag nix#5416), which is present in all versions since nix 2.9 (i.e. since it was added to nix at all)
• fetchTree, from 5 years ago, present since nix 2.4 (Backport libfetchers from the flakes branch nix#3459)

[SuperSandro2000]

The most consistent and least controversial thing we can do is to enforce removal of the builtins prefix for all builtins that are available globally in both Nix and Lix since 2.18. That's what the nixd rule does, IIUC. With that implementation in mind, it makes no sense to discuss or commit a written rule that is different to that and would be superseded in a matter of days or weeks.

👍🏼

[MattSturgeon]

Beyond establishing builtins prefix conventions, this proposal should decide whether to add more attributes to the prelude or discourage existing ones.

This is certainly out of scope for Nixpkgs. This needs to be done in Nix - and also Lix. Before any of that could be relevant for Nixpkgs, that would take a few years, because we keep compatibility here.

I agree that discussing adding new things to the prelude is out of scope for a Nixpkgs issue, but whether we want to encourage or discourage specific uses of the prelude is definitely in scope for this issue. E.g. do we want to encourage using fromTOML without a builtins. prefix when that is inconsistent with builtins.fromJSON?

With nixpkgs' compatibility in mind, we should also note that any decisions to add to the prelude would mean the nixd (libnixf) implementation needs to change to support an explicit list, version pinning, or similar. Currently it extracts the list automatically from nix's C++.

Given the prelude hasn't changed in a long time (thanks @eclairevoyant for confirming), I assume the nix team(s) would now prefer to avoid adding new names to it. But that is an assumption.

If we wanted to discourage specific prelude usage (e.g. fromTOML), we'd also need libnixf to support an explicit list, or a list of specific exclusions (e.g. --exclude-prelude).

I guess --exclude-prelude could also handle the scenario where something new is added to the prelude, assuming we notice when bumping the ci-pinned nixpkgs.

With that implementation in mind, it makes no sense to discuss or commit a written rule that is different to that and would be superseded in a matter of days or weeks.

Agreed, I don't think we should merge verbose and prescriptive documentation here when we can enforce it automatically. At the most, I'd expect the docs to say something brief like:

:::{.note}
Some builtins can be access directly, without typing `builtins.`.
Nixpkgs enforces this style automatically in `treefmt`.
:::

Although this PR isn't just about its actual changes, it also servs as a discussion thread to talk about the broarder issue. Maybe an issue would've been better for that, but we're here now with relevant parties already pinged.