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

lib: Use Nix's static scope checking, fix error message, optimize #101139

Merged
merged 8 commits into from Oct 26, 2020

Conversation

roberth
Copy link
Member

@roberth roberth commented Oct 20, 2020

Motivation

Nix can perform static scope checking, but whenever code is inside
a with expression, the analysis breaks down, because it can't
know statically what's in the attribute set whose attributes were
brought into scope. In those cases, Nix has to assume that
everything works out.

Except it doesnt. Removing with from lib/ revealed an undefined
variable in an error message.

If that doesn't convince you that we're better off without with,
I can tell you that this PR results in a 3% evaluation performance
improvement because Nix can look up local variables by index.
This adds up with applications like the module system.

Furthermore, removing with makes the binding site of each
variable obvious, which helps with comprehension.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@roberth roberth force-pushed the lib-use-static-scope-checking branch from 044957f to b101812 Compare October 20, 2020 13:03
@roberth roberth marked this pull request as draft October 20, 2020 13:44
@roberth roberth marked this pull request as ready for review October 20, 2020 15:34
@roberth
Copy link
Member Author

roberth commented Oct 20, 2020

Ironically my effort to select the attributes from the right lib.* attrsets can't be checked by the scope checking.

Those inherits should be good now. If anyone is concerned if ofborg and me missed one, we could inherit from a single attrset per file like inherit (lib // lib.trivial // ...) isFloat ...; but that pattern smells a bit of tech debt. So I suggest using the current approach. I'll happily fix a wrong inherit if it comes up.

Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

Great success!

Ofborg is green without any rebuilds, and I checked the import lists with the static mode, so it looks like this is a no-op (apart from the new symbols exposed).

Comment on lines +17 to +35
inherit (lib)
isInt
attrNames
isList
isAttrs
substring
addErrorContext
attrValues
concatLists
concatStringsSep
const
elem
generators
head
id
isDerivation
isFunction
mapAttrs
trace;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn’t importing from lib instead of builtins introduce another indirection? Sounds to me like we can get another small speedup by being explicit about the builtins; otoh that means we need to be careful to not import stuff that’s not supported in old nix versions.
Maybe worth investigating whether there’s a small evaluation gain there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same thought, but libs top-level attribute set is only evaluated once, so the overhead of this indirection amounts to a single lookup. You really need the repeated evaluation of certain expressions to make this optimization effective.

I'd be in favor of adding more builtins to lib for consistency and convenience. I've only done it for ones I got wrong writing the explicit inherits, which, although admittedly a bit arbitrarily, keeps the scope of this PR in check.

Copy link
Member

Choose a reason for hiding this comment

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

Let’s do it in a followup then.

lib/modules.nix Outdated
Comment on lines 3 to 49
let
inherit (lib.attrsets)
mapAttrsRecursiveCond
;
inherit (lib.lists)
any all concatLists concatMap
count filter findFirst foldl foldl'
head imap1 length optional
reverseList sort
;
inherit (lib.options)
isOption
mkOption
showDefs
showFiles
showOption
unknownModule
;
inherit (lib.attrsets)
attrByPath
attrNames
catAttrs
getAttrFromPath
mapAttrs
mapAttrsToList
optionalAttrs
recursiveUpdate
setAttrByPath
toList
;
inherit (lib.types)
types
;
inherit (lib.trivial)
flip
id
isBool
isFunction
isString
min
warn
;
inherit (lib.strings)
optionalString
;
inherit (lib)
elem
isAttrs
;
Copy link
Member

Choose a reason for hiding this comment

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

Import list is correct (checked with the nix-mode in emacs)

@@ -616,7 +659,7 @@ rec {
fixupOptionType = loc: opt:
let
options = opt.options or
(throw "Option `${showOption loc'}' has type optionSet but has no option attribute, in ${showFiles opt.declarations}.");
(throw "Option `${showOption loc}' has type optionSet but has no option attribute, in ${showFiles opt.declarations}.");
Copy link
Member

Choose a reason for hiding this comment

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

Is this the place that was buggy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. Scope checking will now find undeclared local variables, even if not used in any tests.

lib/options.nix Outdated Show resolved Hide resolved
lib/options.nix Outdated
Comment on lines 4 to 11
let
inherit (lib)
isAttrs isBool isDerivation isFunction isInt isList isString
all collect concatMap concatLists elemAt filter foldl' head length mapAttrs optionals optional take
;
inherit (lib.attrsets) optionalAttrs;
inherit (lib.strings) concatMapStrings concatStringsSep;
inherit (lib.types) mkOptionType;
Copy link
Member

Choose a reason for hiding this comment

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

correct import list

Comment on lines +11 to +33
inherit (builtins)
compareVersions
elem
elemAt
filter
fromJSON
head
isInt
isList
isString
match
parseDrvName
readFile
replaceStrings
split
storeDir
stringLength
substring
tail
toJSON
typeOf
unsafeDiscardStringContext
;
Copy link
Member

Choose a reason for hiding this comment

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

import list correct

concatStringsSep
escapeNixString
isCoercibleToString
;
Copy link
Member

Choose a reason for hiding this comment

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

import lists correct

lib/types.nix Show resolved Hide resolved
lib/default.nix Show resolved Hide resolved
@@ -63,7 +63,7 @@ let
deepSeq elem elemAt filter genericClosure genList getAttr
hasAttr head isAttrs isBool isInt isList isString length
lessThan listToAttrs pathExists readFile replaceStrings seq
stringLength sub substring tail;
stringLength sub substring tail trace;
Copy link
Member

Choose a reason for hiding this comment

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

This has been bothering me for a while 👍

@Profpatsch
Copy link
Member

Some of my comments only make sense in the full diff view, Github is not a good tool for these reviews.

Nix can perform static scope checking, but whenever code is inside
a `with` expression, the analysis breaks down, because it can't
know statically what's in the attribute set whose attributes were
brought into scope. In those cases, Nix has to assume that
everything works out.

Except it doesnt. Removing `with` from lib/ revealed an undefined
variable in an error message.

If that doesn't convince you that we're better off without `with`,
I can tell you that this PR results in a 3% evaluation performance
improvement because Nix can look up local variables by index.
This adds up with applications like the module system.

Furthermore, removing `with` makes the binding site of each
variable obvious, which helps with comprehension.
Unlike the other three is* functions in lib.trivial, it was only
available as lib.trivial.isFloat
This puts it among the trace* family of functions derived from it.
@roberth roberth force-pushed the lib-use-static-scope-checking branch from cde5540 to fe4a58e Compare October 22, 2020 11:47
@roberth
Copy link
Member Author

roberth commented Oct 22, 2020

Rebased and recent lib additions tested.

@roberth
Copy link
Member Author

roberth commented Oct 22, 2020

I appreciate further improvements to lib but I'd like to merge this soon to avoid conflicts with other changes to lib.

@@ -270,7 +270,7 @@ rec {
name = "attrs";
description = "attribute set";
check = isAttrs;
merge = loc: foldl' (res: def: mergeAttrs res def.value) {};
merge = loc: foldl' (res: def: res // def.value) {};
Copy link
Member

Choose a reason for hiding this comment

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

mergeAttrs = x: y: x // y so this is correct

Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@roberth
Copy link
Member Author

roberth commented Oct 26, 2020

I've waited a bit because I wasn't going to be available in the past couple of days. No relevant changes were made to lib/ in the meantime, so I'll go ahead with the merge.

@roberth roberth merged commit 7102388 into NixOS:master Oct 26, 2020
@vbgl
Copy link
Contributor

vbgl commented Oct 26, 2020

Evaluation seems broken:

error: undefined variable 'boolToString' at …/nixpkgs/lib/types.nix:552:42

kini added a commit to kini/nixpkgs that referenced this pull request Oct 26, 2020
I think there was a silent (i.e. semantic) merge conflict between PR NixOS#101139 and
PR NixOS#100456.  This commit should fix the error, which manifests as follows:

  error: undefined variable 'boolToString' at /home/kkini/src/nixpkgs/lib/types.nix:552:42
@kini kini mentioned this pull request Oct 26, 2020
10 tasks
@xaverdh
Copy link
Contributor

xaverdh commented Oct 26, 2020

This is coming from cebf919#diff-64168148acd9f2147ef733b1498b8821c24f2e3f32354b0e147dd421d71274f3, which was merged in the meantime

@roberth
Copy link
Member Author

roberth commented Oct 26, 2020

Apologies for the breakage.
I did check the diff but I must have misread it.

@deviant
Copy link
Member

deviant commented Oct 30, 2020

FWIW, this breaks lib.commitIdFromGitRepo.

@Profpatsch
Copy link
Member

Was fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants