libnixf: check builtins. prefix from variable lookups

[inclyc]

No description provided.

[philiptaron]

Is this correct? The list of top-level names isn't everything in builtins, it's only this set:

1. abort
2. baseNameOf
3. break
4. derivation
5. derivationStrict
6. dirOf
7. false
8. fetchGit
9. fetchMercurial
10. fetchTarball
11. fetchTree
12. fromTOML
13. import
14. isNull
15. map
16. null
17. placeholder
18. removeAttrs
19. scopedImport
20. throw
21. toString
22. true

I don't see any gating in this PR to just those names.

[inclyc]

Is this correct? The list of top-level names isn't everything in builtins, it's only this set:

1. abort
2. baseNameOf
3. break
4. derivation
5. derivationStrict
6. dirOf
7. false
8. fetchGit
9. fetchMercurial
10. fetchTarball
11. fetchTree
12. fromTOML
13. import
14. isNull
15. map
16. null
17. placeholder
18. removeAttrs
19. scopedImport
20. throw
21. toString
22. true

I don't see any gating in this PR to just those names.

That list is generated by the NixOS/nix library. Specifically, those values are referred to as PrimOp in that implementation. PrimOps follow these visibility rules:

1. __xxx -> builtins.xxx + __xxx
2. xxx -> builtins.xxx + xxx

This information is obtained directly from the C++ library and is not encoded in the nixd code.

[inclyc]

builtins_480p.mov

@philiptaron See this video, is that expected?

[philiptaron]

This information is obtained directly from the C++ library and is not encoded in the nixd code.

Wonderful.

[MattSturgeon]

This information is obtained directly from the C++ library and is not encoded in the nixd code.

My only concern with this approach is when using this for nixpkgs specifically, is nixpkgs is supposed to eval on multiple implementations & versions of nix.

E.g. if a new version of nix adds or remove something from the prelude, nixpkgs wouldn't want to change its linting rules until that change was reflected in all "supported" nix versions (i.e. when we bump minFeatures to include the change).

I wonder if there is a way to configure this linting and use the C++ information merely as the default? E.g. we could have an --enforce-globals list:

--enforce-global true \
--enforce-global false \
--enforce-global map \
--enforce-global throw \
# ...

When used, only the explicitly listed "globals" would be treated as prelude values.

IDK if such an interface would need a way to both append to the default list or override it? For nixpkgs we'd want to override the list, but maybe others would want a different behaviour?

[philiptaron]

I would be shocked if Nix introduced a new name to the prelude; they've almost always been added to builtins.

[MattSturgeon]

I would be shocked if Nix introduced a new name to the prelude; they've almost always been added to builtins.

I don't know how comfortable I am relying on that assumption, when the result of something being added/moved/removed would result in linting errors across nixpkgs and fixing those lint errors would break eval on older still-supported versions (and other supported implementations).

I guess the questions are 1) when did the prelude last change? 2) are there any plans to move things into or out of the prelude? That would give us an idea of how unreasonably my discomfort is 😁

[trueNAHO]

I would be shocked if Nix introduced a new name to the prelude; they've almost always been added to builtins.

I don't know how comfortable I am relying on that assumption, when the result of something being added/moved/removed would result in linting errors across nixpkgs and fixing those lint errors would break eval on older still-supported versions (and other supported implementations).

I guess the questions are 1) when did the prelude last change? 2) are there any plans to move things into or out of the prelude? That would give us an idea of how unreasonably my discomfort is 😁

That is more or less what NixOS/nixpkgs#449123 (comment) is trying to figure out.

[wolfgangwalther]

To avoid problems with later nix versions adding new globals - how about we achieve consistency by enforcing builtins. prefixes to all of these?

This does not suffer from the same problem. It is also much better to feature detect for lib/minfeatures.nix in Nixpkgs.

Ultimately, we just want consistency and nothing to nit-pick about, right?

[inclyc]

To avoid problems with later nix versions adding new globals - how about we achieve consistency by enforcing builtins. prefixes to all of these?

This does not suffer from the same problem. It is also much better to feature detect for lib/minfeatures.nix in Nixpkgs.

Ultimately, we just want consistency and nothing to nit-pick about, right?

Too pedantic to write builtins.true ?

[wolfgangwalther]

In that case, I agree with @MattSturgeon that it would be best to freeze this list at those globals that were present in 2.18.

[eclairevoyant]

I guess the questions are 1) when did the prelude last change?

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

• break (added 3 years ago in --debugger flag NixOS/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 NixOS/nix#3459)

[MattSturgeon]

I understand if this is considered not worth it, currently. But in an ideal world, I'd like to see --exclude-prelude and --include-prelude.

--exclude-prelude would allow making specific exceptions to the rule. E.g. --exclude-prelude fromTOML would enforce builtins.fromTOML instead of fromTOML for consistency with builtins.fromJSON.

--include-prelude would allow overriding the rule with an explicit list. E.g. --include-prelude true --include-prelude false would act as if true and false are the only prelude items. When used, there would be no need to check nix's actual preludes.

While I think this would be useful beyond nixpkgs, it would also allow nixpkgs to handle backwards compatibility if anything does get added to the prelude in the future. Based on @eclairevoyant's comment this seems infrequent and unlikely, but still possible.

Personally, I think --exclude-prelude seems more useful generally. --include-prelude is kinda niche and, I assume, more effort to implement. It'd only be useful for projects that want to use a strict subset of the prelude.

[eclairevoyant]

Side note: "prelude" isn't a documented term anywhere in nix, I think it's a haskell thing.
(Though, not trying to get too in the weeds on naming, before deciding if it's even worth it obviously.)