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

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.
2024-11-25 08:23:09 +00:00 · 2020-10-20 13:47:24 +02:00 · 2020-10-20 13:47:24 +02:00 · afa6c51f27
commit afa6c51f27
parent 5aa2a98dfa
9 changed files with 238 additions and 82 deletions
--- a/lib/debug.nix
+++ b/lib/debug.nix
@ -14,9 +14,25 @@
 */
 { lib }:
 let
-  inherit (builtins) trace isAttrs isList isInt
-          head substring attrNames;
-  inherit (lib) id elem isFunction;
+  inherit (lib)
+    isInt
+    attrNames
+    isList
+    isAttrs
+    substring
+    addErrorContext
+    attrValues
+    concatLists
+    concatStringsSep
+    const
+    elem
+    generators
+    head
+    id
+    isDerivation
+    isFunction
+    mapAttrs
+    trace;
 in

 rec {
@ -94,7 +110,7 @@ rec {
       trace: { a = { b = {…}; }; }
       => null
   */
-  traceSeqN = depth: x: y: with lib;
+  traceSeqN = depth: x: y:
    let snip = v: if      isList  v then noQuotes "[…]" v
                  else if isAttrs v then noQuotes "{…}" v
                  else v;
@ -149,7 +165,7 @@ rec {
  */
  runTests =
    # Tests to run
-    tests: lib.concatLists (lib.attrValues (lib.mapAttrs (name: test:
+    tests: concatLists (attrValues (mapAttrs (name: test:
    let testsToRun = if tests ? tests then tests.tests else [];
    in if (substring 0 4 name == "test" ||  elem name testsToRun)
       && ((testsToRun == []) || elem name tests.tests)
@ -176,9 +192,9 @@ rec {
          + "and will be removed in the next release. "
          + "Please use more specific concatenation "
          + "for your uses (`lib.concat(Map)StringsSep`)." )
-    (lib.concatStringsSep "; " (map (x: "${x}=") (attrNames a)));
+    (concatStringsSep "; " (map (x: "${x}=") (attrNames a)));

-  showVal = with lib;
+  showVal =
    trace ( "Warning: `showVal` is deprecated "
          + "and will be removed in the next release, "
          + "please use `traceSeqN`" )
@ -226,7 +242,7 @@ rec {
    trace ( "Warning: `addErrorContextToAttrs` is deprecated "
          + "and will be removed in the next release. "
          + "Please use `builtins.addErrorContext` directly." )
-    (lib.mapAttrs (a: v: lib.addErrorContext "while evaluating ${a}" v) attrs);
+    (mapAttrs (a: v: addErrorContext "while evaluating ${a}" v) attrs);

  # example: (traceCallXml "myfun" id 3) will output something like
  # calling myfun arg 1: 3 result: 3
--- a/lib/default.nix
+++ b/lib/default.nix
@ -9,7 +9,7 @@ let

  lib = makeExtensible (self: let
    callLibs = file: import file { lib = self; };
-  in with self; {
+  in {

    # often used, or depending on very little
    trivial = callLibs ./trivial.nix;
@ -54,7 +54,7 @@ let
    filesystem = callLibs ./filesystem.nix;

    # back-compat aliases
-    platforms = systems.doubles;
+    platforms = self.systems.doubles;

    # linux kernel configuration
    kernel = callLibs ./kernel.nix;
@ -64,13 +64,13 @@ let
      hasAttr head isAttrs isBool isInt isList isString length
      lessThan listToAttrs pathExists readFile replaceStrings seq
      stringLength sub substring tail;
-    inherit (trivial) id const pipe concat or and bitAnd bitOr bitXor
+    inherit (self.trivial) id const pipe concat or and bitAnd bitOr bitXor
      bitNot boolToString mergeAttrs flip mapNullable inNixShell min max
      importJSON importTOML warn info showWarnings nixpkgsVersion version mod compare
      splitByAndCompare functionArgs setFunctionArgs isFunction toHexString toBaseDigits;
-    inherit (fixedPoints) fix fix' converge extends composeExtensions
+    inherit (self.fixedPoints) fix fix' converge extends composeExtensions
      makeExtensible makeExtensibleWithCustomName;
-    inherit (attrsets) attrByPath hasAttrByPath setAttrByPath
+    inherit (self.attrsets) attrByPath hasAttrByPath setAttrByPath
      getAttrFromPath attrVals attrValues getAttrs catAttrs filterAttrs
      filterAttrsRecursive foldAttrs collect nameValuePair mapAttrs
      mapAttrs' mapAttrsToList mapAttrsRecursive mapAttrsRecursiveCond
@ -79,13 +79,13 @@ let
      recursiveUpdate matchAttrs overrideExisting getOutput getBin
      getLib getDev getMan chooseDevOutputs zipWithNames zip
      recurseIntoAttrs dontRecurseIntoAttrs;
-    inherit (lists) singleton forEach foldr fold foldl foldl' imap0 imap1
+    inherit (self.lists) singleton forEach foldr fold foldl foldl' imap0 imap1
      concatMap flatten remove findSingle findFirst any all count
      optional optionals toList range partition zipListsWith zipLists
      reverseList listDfs toposort sort naturalSort compareLists take
      drop sublist last init crossLists unique intersectLists
      subtractLists mutuallyExclusive groupBy groupBy';
-    inherit (strings) concatStrings concatMapStrings concatImapStrings
+    inherit (self.strings) concatStrings concatMapStrings concatImapStrings
      intersperse concatStringsSep concatMapStringsSep
      concatImapStringsSep makeSearchPath makeSearchPathOutput
      makeLibraryPath makeBinPath optionalString
@ -97,19 +97,19 @@ let
      nameFromURL enableFeature enableFeatureAs withFeature
      withFeatureAs fixedWidthString fixedWidthNumber isStorePath
      toInt readPathsFromFile fileContents;
-    inherit (stringsWithDeps) textClosureList textClosureMap
+    inherit (self.stringsWithDeps) textClosureList textClosureMap
      noDepEntry fullDepEntry packEntry stringAfter;
-    inherit (customisation) overrideDerivation makeOverridable
+    inherit (self.customisation) overrideDerivation makeOverridable
      callPackageWith callPackagesWith extendDerivation hydraJob
      makeScope;
-    inherit (meta) addMetaAttrs dontDistribute setName updateName
+    inherit (self.meta) addMetaAttrs dontDistribute setName updateName
      appendToName mapDerivationAttrset setPrio lowPrio lowPrioSet hiPrio
      hiPrioSet;
-    inherit (sources) pathType pathIsDirectory cleanSourceFilter
+    inherit (self.sources) pathType pathIsDirectory cleanSourceFilter
      cleanSource sourceByRegex sourceFilesBySuffices
      commitIdFromGitRepo cleanSourceWith pathHasContext
      canCleanSource pathIsRegularFile pathIsGitRepo;
-    inherit (modules) evalModules unifyModuleSyntax
+    inherit (self.modules) evalModules unifyModuleSyntax
      applyIfFunction mergeModules
      mergeModules' mergeOptionDecls evalOptionValue mergeDefinitions
      pushDownProperties dischargeProperties filterOverrides
@ -119,21 +119,21 @@ let
      mkAliasAndWrapDefinitions fixMergeModules mkRemovedOptionModule
      mkRenamedOptionModule mkMergedOptionModule mkChangedOptionModule
      mkAliasOptionModule doRename;
-    inherit (options) isOption mkEnableOption mkSinkUndeclaredOptions
+    inherit (self.options) isOption mkEnableOption mkSinkUndeclaredOptions
      mergeDefaultOption mergeOneOption mergeEqualOption getValues
      getFiles optionAttrSetToDocList optionAttrSetToDocList'
      scrubOptionValue literalExample showOption showFiles
      unknownModule mkOption;
-    inherit (types) isType setType defaultTypeMerge defaultFunctor
+    inherit (self.types) isType setType defaultTypeMerge defaultFunctor
      isOptionType mkOptionType;
-    inherit (asserts)
+    inherit (self.asserts)
      assertMsg assertOneOf;
-    inherit (debug) addErrorContextToAttrs traceIf traceVal traceValFn
+    inherit (self.debug) addErrorContextToAttrs traceIf traceVal traceValFn
      traceXMLVal traceXMLValMarked traceSeq traceSeqN traceValSeq
      traceValSeqFn traceValSeqN traceValSeqNFn traceShowVal
      traceShowValMarked showVal traceCall traceCall2 traceCall3
      traceValIfNot runTests testAllTrue traceCallXml attrNamesToStr;
-    inherit (misc) maybeEnv defaultMergeArg defaultMerge foldArgs
+    inherit (self.misc) maybeEnv defaultMergeArg defaultMerge foldArgs
      maybeAttrNullable maybeAttr ifEnable checkFlag getValue
      checkReqs uniqList uniqListExt condConcat lazyGenericClosure
      innerModifySumArgs modifySumArgs innerClosePropagation
@ -143,7 +143,7 @@ let
      mergeAttrsByFuncDefaultsClean mergeAttrBy
      fakeHash fakeSha256 fakeSha512
      nixType imap;
-    inherit (versions)
+    inherit (self.versions)
      splitVersion;
  });
 in lib
--- a/lib/lists.nix
+++ b/lib/lists.nix
@ -1,9 +1,9 @@
 # General list operations.

 { lib }:
-with lib.trivial;
 let
  inherit (lib.strings) toInt;
+  inherit (lib.trivial) compare min;
 in
 rec {

--- a/lib/modules.nix
+++ b/lib/modules.nix
@ -1,12 +1,55 @@
 { lib }:

-with lib.lists;
-with lib.strings;
-with lib.trivial;
-with lib.attrsets;
-with lib.options;
-with lib.debug;
-with lib.types;
+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
+    ;
+in

 rec {

@ -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}.");
      f = tp:
        let optionSetIn = type: (tp.name == type) && (tp.functor.wrapped.name == "optionSet");
        in
--- a/lib/options.nix
+++ b/lib/options.nix
@ -1,11 +1,15 @@
 # Nixpkgs/NixOS option handling.
 { lib }:

-with lib.trivial;
-with lib.lists;
-with lib.attrsets;
-with lib.strings;
-
+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;
+in
 rec {

  /* Returns true when the given argument is an option
--- a/lib/sources.nix
+++ b/lib/sources.nix
@ -1,16 +1,33 @@
 # Functions for copying sources to the Nix store.
 { lib }:

+let
+  inherit (builtins)
+    hasContext
+    match
+    readDir
+    readFile
+    storeDir
+    tryEval
+    ;
+  inherit (lib)
+    filter
+    getAttr
+    isString
+    pathExists
+    split
+    ;
+in
 rec {

  # Returns the type of a path: regular (for file), symlink, or directory
-  pathType = p: with builtins; getAttr (baseNameOf p) (readDir (dirOf p));
+  pathType = p: getAttr (baseNameOf p) (readDir (dirOf p));

  # Returns true if the path exists and is a directory, false otherwise
-  pathIsDirectory = p: if builtins.pathExists p then (pathType p) == "directory" else false;
+  pathIsDirectory = p: if pathExists p then (pathType p) == "directory" else false;

  # Returns true if the path exists and is a regular file, false otherwise
-  pathIsRegularFile = p: if builtins.pathExists p then (pathType p) == "regular" else false;
+  pathIsRegularFile = p: if pathExists p then (pathType p) == "regular" else false;

  # Bring in a path as a source, filtering out all Subversion and CVS
  # directories, as well as backup files (*~).
@ -19,8 +36,8 @@ rec {
    (baseName == ".git" || type == "directory" && (baseName == ".svn" || baseName == "CVS" || baseName == ".hg")) ||
    # Filter out editor backup / swap files.
    lib.hasSuffix "~" baseName ||
-    builtins.match "^\\.sw[a-z]$" baseName != null ||
-    builtins.match "^\\..*\\.sw[a-z]$" baseName != null ||
+    match "^\\.sw[a-z]$" baseName != null ||
+    match "^\\..*\\.sw[a-z]$" baseName != null ||

    # Filter out generates files.
    lib.hasSuffix ".o" baseName ||
@ -89,7 +106,7 @@ rec {
    in lib.cleanSourceWith {
      filter = (path: type:
        let relPath = lib.removePrefix (toString origSrc + "/") (toString path);
-        in lib.any (re: builtins.match re relPath != null) regexes);
+        in lib.any (re: match re relPath != null) regexes);
      inherit src;
    };

@ -102,13 +119,12 @@ rec {
      in type == "directory" || lib.any (ext: lib.hasSuffix ext base) exts;
    in cleanSourceWith { inherit filter; src = path; };

-  pathIsGitRepo = path: (builtins.tryEval (commitIdFromGitRepo path)).success;
+  pathIsGitRepo = path: (tryEval (commitIdFromGitRepo path)).success;

  # Get the commit id of a git repo
  # Example: commitIdFromGitRepo <nixpkgs/.git>
  commitIdFromGitRepo =
    let readCommitFromFile = file: path:
-      with builtins;
        let fileName       = toString path + "/" + file;
            packedRefsName = toString path + "/packed-refs";
            absolutePath   = base: path:
@ -145,11 +161,11 @@ rec {
           # packed-refs file, so we have to grep through it:
           then
             let fileContent = readFile packedRefsName;
-                 matchRef = builtins.match "([a-z0-9]+) ${file}";
-                 isRef = s: builtins.isString s && (matchRef s) != null;
+                 matchRef = match "([a-z0-9]+) ${file}";
+                 isRef = s: isString s && (matchRef s) != null;
                 # there is a bug in libstdc++ leading to stackoverflow for long strings:
                 # https://github.com/NixOS/nix/issues/2147#issuecomment-659868795
-                 refs = builtins.filter isRef (builtins.split "\n" fileContent);
+                 refs = filter isRef (split "\n" fileContent);
             in if refs == []
                then throw ("Could not find " + file + " in " + packedRefsName)
                else lib.head (matchRef (lib.head refs))
@ -157,7 +173,7 @@ rec {
           else throw ("Not a .git directory: " + path);
    in readCommitFromFile "HEAD";

-  pathHasContext = builtins.hasContext or (lib.hasPrefix builtins.storeDir);
+  pathHasContext = builtins.hasContext or (lib.hasPrefix storeDir);

  canCleanSource = src: src ? _isLibCleanSourceWith || !(pathHasContext (toString src));
 }
--- a/lib/strings-with-deps.nix
+++ b/lib/strings-with-deps.nix
@ -41,10 +41,15 @@ Usage:
  [1] maybe this behaviour should be removed to keep things simple (?)
 */

-with lib.lists;
-with lib.attrsets;
-with lib.strings;
-
+let
+  inherit (lib)
+    concatStringsSep
+    head
+    isAttrs
+    listToAttrs
+    tail
+    ;
+in
 rec {

  /* !!! The interface of this function is kind of messed up, since
--- a/lib/strings.nix
+++ b/lib/strings.nix
@ -8,7 +8,29 @@ in

 rec {

-  inherit (builtins) stringLength substring head tail isString replaceStrings;
+  inherit (builtins)
+    compareVersions
+    elem
+    elemAt
+    filter
+    fromJSON
+    head
+    isInt
+    isList
+    isString
+    match
+    parseDrvName
+    readFile
+    replaceStrings
+    split
+    storeDir
+    stringLength
+    substring
+    tail
+    toJSON
+    typeOf
+    unsafeDiscardStringContext
+    ;

  /* Concatenate a list of strings.

@ -120,7 +142,7 @@ rec {
    subDir:
    # List of base paths
    paths:
-    concatStringsSep ":" (map (path: path + "/" + subDir) (builtins.filter (x: x != null) paths));
+    concatStringsSep ":" (map (path: path + "/" + subDir) (filter (x: x != null) paths));

  /* Construct a Unix-style search path by appending the given
     `subDir` to the specified `output` of each of the packages. If no
@ -313,7 +335,7 @@ rec {
       escapeNixString "hello\${}\n"
       => "\"hello\\\${}\\n\""
  */
-  escapeNixString = s: escape ["$"] (builtins.toJSON s);
+  escapeNixString = s: escape ["$"] (toJSON s);

  /* Turn a string into an exact regular expression

@ -337,7 +359,7 @@ rec {
  */
  escapeNixIdentifier = s:
    # Regex from https://github.com/NixOS/nix/blob/d048577909e383439c2549e849c5c2f2016c997e/src/libexpr/lexer.l#L91
-    if builtins.match "[a-zA-Z_][a-zA-Z0-9_'-]*" s != null
+    if match "[a-zA-Z_][a-zA-Z0-9_'-]*" s != null
    then s else escapeNixString s;

  # Obsolete - use replaceStrings instead.
@ -466,7 +488,7 @@ rec {
       versionOlder "1.1" "1.1"
       => false
  */
-  versionOlder = v1: v2: builtins.compareVersions v2 v1 == 1;
+  versionOlder = v1: v2: compareVersions v2 v1 == 1;

  /* Return true if string v1 denotes a version equal to or newer than v2.

@ -492,7 +514,7 @@ rec {
  */
  getName = x:
   let
-     parse = drv: (builtins.parseDrvName drv).name;
+     parse = drv: (parseDrvName drv).name;
   in if isString x
      then parse x
      else x.pname or (parse x.name);
@ -509,7 +531,7 @@ rec {
  */
  getVersion = x:
   let
-     parse = drv: (builtins.parseDrvName drv).version;
+     parse = drv: (parseDrvName drv).version;
   in if isString x
      then parse x
      else x.version or (parse x.name);
@ -527,7 +549,7 @@ rec {
    let
      components = splitString "/" url;
      filename = lib.last components;
-      name = builtins.head (splitString sep filename);
+      name = head (splitString sep filename);
    in assert name != filename; name;

  /* Create an --{enable,disable}-<feat> string that can be passed to
@ -617,14 +639,14 @@ rec {
  */
  floatToString = float: let
    result = toString float;
-    precise = float == builtins.fromJSON result;
+    precise = float == fromJSON result;
  in if precise then result
    else lib.warn "Imprecise conversion from float to string ${result}" result;

  /* Check whether a value can be coerced to a string */
  isCoercibleToString = x:
-    builtins.elem (builtins.typeOf x) [ "path" "string" "null" "int" "float" "bool" ] ||
-    (builtins.isList x && lib.all isCoercibleToString x) ||
+    elem (typeOf x) [ "path" "string" "null" "int" "float" "bool" ] ||
+    (isList x && lib.all isCoercibleToString x) ||
    x ? outPath ||
    x ? __toString;

@ -643,8 +665,8 @@ rec {
  isStorePath = x:
    if isCoercibleToString x then
      let str = toString x; in
-      builtins.substring 0 1 str == "/"
-      && dirOf str == builtins.storeDir
+      substring 0 1 str == "/"
+      && dirOf str == storeDir
    else
      false;

@ -662,8 +684,8 @@ rec {
  */
  # Obviously, it is a bit hacky to use fromJSON this way.
  toInt = str:
-    let may_be_int = builtins.fromJSON str; in
-    if builtins.isInt may_be_int
+    let may_be_int = fromJSON str; in
+    if isInt may_be_int
    then may_be_int
    else throw "Could not convert ${str} to int.";

@ -685,10 +707,10 @@ rec {
  readPathsFromFile = lib.warn "lib.readPathsFromFile is deprecated, use a list instead"
    (rootPath: file:
      let
-        lines = lib.splitString "\n" (builtins.readFile file);
+        lines = lib.splitString "\n" (readFile file);
        removeComments = lib.filter (line: line != "" && !(lib.hasPrefix "#" line));
        relativePaths = removeComments lines;
-        absolutePaths = builtins.map (path: rootPath + "/${path}") relativePaths;
+        absolutePaths = map (path: rootPath + "/${path}") relativePaths;
      in
        absolutePaths);

@ -702,7 +724,7 @@ rec {
       fileContents ./version
       => "1.0"
  */
-  fileContents = file: removeSuffix "\n" (builtins.readFile file);
+  fileContents = file: removeSuffix "\n" (readFile file);


  /* Creates a valid derivation name from a potentially invalid one.
@ -720,13 +742,13 @@ rec {
  sanitizeDerivationName = string: lib.pipe string [
    # Get rid of string context. This is safe under the assumption that the
    # resulting string is only used as a derivation name
-    builtins.unsafeDiscardStringContext
+    unsafeDiscardStringContext
    # Strip all leading "."
-    (x: builtins.elemAt (builtins.match "\\.*(.*)" x) 0)
+    (x: elemAt (match "\\.*(.*)" x) 0)
    # Split out all invalid characters
    # https://github.com/NixOS/nix/blob/2.3.2/src/libstore/store-api.cc#L85-L112
    # https://github.com/NixOS/nix/blob/2242be83c61788b9c0736a92bb0b5c7bbfc40803/nix-rust/src/store/path.rs#L100-L125
-    (builtins.split "[^[:alnum:]+._?=-]+")
+    (split "[^[:alnum:]+._?=-]+")
    # Replace invalid character ranges with a "-"
    (concatMapStrings (s: if lib.isList s then "-" else s))
    # Limit to 211 characters (minus 4 chars for ".drv")
--- a/lib/types.nix
+++ b/lib/types.nix
@ -1,12 +1,62 @@
 # Definitions related to run-time type checking.  Used in particular
 # to type-check NixOS configurations.
 { lib }:
-with lib.lists;
-with lib.attrsets;
-with lib.options;
-with lib.trivial;
-with lib.strings;
+
 let
+  inherit (lib)
+    elem
+    flip
+    functionArgs
+    isAttrs
+    isBool
+    isDerivation
+    isFloat
+    isFunction
+    isInt
+    isList
+    isString
+    isStorePath
+    setFunctionArgs
+    toDerivation
+    toList
+    ;
+  inherit (lib.lists)
+    all
+    concatLists
+    count
+    elemAt
+    filter
+    foldl'
+    head
+    imap1
+    last
+    length
+    tail
+    unique
+    ;
+  inherit (lib.attrsets)
+    attrNames
+    filterAttrs
+    hasAttr
+    mapAttrs
+    optionalAttrs
+    zipAttrsWith
+    ;
+  inherit (lib.options)
+    getFiles
+    getValues
+    mergeDefaultOption
+    mergeEqualOption
+    mergeOneOption
+    showFiles
+    showOption
+    ;
+  inherit (lib.strings)
+    concatMapStringsSep
+    concatStringsSep
+    escapeNixString
+    isCoercibleToString
+    ;

  inherit (lib.modules) mergeDefinitions;
  outer_types =