treewide: abort -> throw where it makes sense #45637

infinisil · 2018-08-26T00:40:12Z

Motivation for this change

I don't really care about this getting merged, but I'd like to find out whether there's any reason for using aborts. Please let me know if you have any valid use case, because I couldn't find any. I also asked many people but nobody had one.

I have NOT tested this in any way, let me know if this breaks anything, which might just be a reason for having aborts then.

Note that I'm looking for actual use cases, not theoretical ones.

Ping @Profpatsch

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
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 nox --run "nox-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)
Fits CONTRIBUTING.md.

Ericson2314 · 2018-08-26T05:25:51Z

Yeah I 've never seen any consistency in one or the other. I assume people use one cause they didn't know of the other.

Mic92 · 2018-08-26T11:06:46Z

default.nix

@@ -2,7 +2,7 @@ let requiredVersion = import ./lib/minver.nix; in

 if ! builtins ? nixVersion || builtins.compareVersions requiredVersion builtins.nixVersion == 1 then

-  abort ''
+  throw ''


Is this maybe a special case? Some commands like nix-env -qa will silently ignore throw as it issues an assertion error while abort will result in an evaluation error.

Yes, this should stay abort.

Profpatsch · 2018-08-26T18:50:54Z

As @Mic92 said, abort can’t be skipped, while throw is skipped by nix-env. Most changes from lib signal programmer errors, so should stay abort. It doesn’t make sense for packages to use abort however.

Mic92 · 2018-08-26T21:23:06Z

@Profpatsch may be you point to the examples in lib where abort should be used. Because those are not clear to me.

Profpatsch

I commented on the first few changes, it looks to me like nearly all of these are programmer errors that should abort the evaluation whenever they happen (not skippable, like with throw).
So I’m against this change.

Profpatsch · 2018-08-27T08:59:51Z

default.nix

@@ -2,7 +2,7 @@ let requiredVersion = import ./lib/minver.nix; in

 if ! builtins ? nixVersion || builtins.compareVersions requiredVersion builtins.nixVersion == 1 then

-  abort ''
+  throw ''


Yes, this should stay abort.

Profpatsch · 2018-08-27T09:00:00Z

lib/attrsets.nix

@@ -72,7 +72,7 @@ rec {
  */
  getAttrFromPath = attrPath: set:
    let errorMsg = "cannot find attribute `" + concatStringsSep "." attrPath + "'";
-    in attrByPath attrPath (abort errorMsg) set;
+    in attrByPath attrPath (throw errorMsg) set;


Profpatsch · 2018-08-27T09:00:30Z

lib/generators.nix

@@ -31,7 +31,7 @@ rec {
   * suitable for bash scripts but not much else.
   */
  mkValueStringDefault = {}: v: with builtins;
-    let err = t: v: abort
+    let err = t: v: throw


abort, is a programmer error.

Profpatsch · 2018-08-27T09:00:46Z

lib/generators.nix

@@ -171,7 +171,7 @@ rec {
                       fna);
      in if fna == {}    then "<λ>"
                         else "<λ:{${showFnas}}>"
-    else abort "generators.toPretty: should never happen (v = ${v})";
+    else throw "generators.toPretty: should never happen (v = ${v})";


abort, is a failed pattern match and thus a programmer error.

Profpatsch · 2018-08-27T09:00:56Z

lib/generators.nix

@@ -184,7 +184,7 @@ rec {
      if isList x   then list ind x else
      if isAttrs x  then attrs ind x else
      if isFloat x  then float ind x else
-      abort "generators.toPlist: should never happen (v = ${v})";
+      throw "generators.toPlist: should never happen (v = ${v})";


Profpatsch · 2018-08-27T09:01:42Z

lib/modules.nix

@@ -671,7 +671,7 @@ rec {
    let
      fromOpt = getAttrFromPath from options;
      toOf = attrByPath to
-        (abort "Renaming error: option `${showOption to}' does not exist.");
+        (throw "Renaming error: option `${showOption to}' does not exist.");


Also a programmer error, abort.

Profpatsch · 2018-08-27T09:01:49Z

lib/options.nix

@@ -60,14 +60,14 @@ rec {
    else throw "Cannot merge definitions of `${showOption loc}' given in ${showFiles (getFiles defs)}.";

  mergeOneOption = loc: defs:
-    if defs == [] then abort "This case should never happen."
+    if defs == [] then throw "This case should never happen."


Profpatsch · 2018-08-27T09:01:55Z

lib/options.nix

    else if length defs != 1 then
      throw "The unique option `${showOption loc}' is defined multiple times, in ${showFiles (getFiles defs)}."
    else (head defs).value;

  /* "Merge" option definitions by checking that they all have the same value. */
  mergeEqualOption = loc: defs:
-    if defs == [] then abort "This case should never happen."
+    if defs == [] then throw "This case should never happen."


Profpatsch · 2018-08-27T09:02:14Z

nixos/lib/testing.nix

@@ -91,7 +91,7 @@ in rec {

      testDriverName = with builtins;
        if testNameLen > maxTestNameLen then
-          abort ("The name of the test '${name}' must not be longer than ${toString maxTestNameLen} " +
+          throw ("The name of the test '${name}' must not be longer than ${toString maxTestNameLen} " +


Programmer error, abort.

FRidh · 2018-08-27T11:39:39Z

Added "needs: documentation" label because clearly that's the issue here.

infinisil · 2018-08-27T13:21:58Z

Ah yes, thanks @Profpatsch! Makes sense for those to be aborts as you explained. I undid those ones.

infinisil · 2018-09-18T06:28:12Z

Can this be merged like this? This now only changes the aborts that should've been throws from the beginning.

Mic92 · 2018-09-18T09:05:26Z

I am not sure why @edolstra was against the pull request and if those issues are still present.

infinisil · 2019-04-28T19:20:04Z

Can't be bothered with this

infinisil requested review from edolstra, Ericson2314, nbp and Profpatsch as code owners August 26, 2018 00:40

Mic92 reviewed Aug 26, 2018

View reviewed changes

Profpatsch requested changes Aug 27, 2018

View reviewed changes

FRidh added the 9.needs: documentation label Aug 27, 2018

treewide nixos/pkgs: abort -> throw

49105db

infinisil force-pushed the remove-aborts branch from 465aa0e to 49105db Compare August 27, 2018 13:21

infinisil mentioned this pull request Sep 8, 2018

[RFC 0033] [WIP] Deprecation NixOS/rfcs#33

Closed

infinisil changed the title ~~treewide: abort -> throw (aka aborts are useless, convince me otherwise)~~ treewide: abort -> throw where it makes sense Sep 18, 2018

infinisil closed this Apr 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treewide: abort -> throw where it makes sense #45637

treewide: abort -> throw where it makes sense #45637

infinisil commented Aug 26, 2018 •

edited

Loading

Ericson2314 commented Aug 26, 2018

Mic92 Aug 26, 2018 •

edited

Loading

Profpatsch Aug 27, 2018

Profpatsch commented Aug 26, 2018

Mic92 commented Aug 26, 2018 •

edited

Loading

Profpatsch left a comment

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

Profpatsch Aug 27, 2018

FRidh commented Aug 27, 2018

infinisil commented Aug 27, 2018

infinisil commented Sep 18, 2018

Mic92 commented Sep 18, 2018

infinisil commented Apr 28, 2019

treewide: abort -> throw where it makes sense #45637

treewide: abort -> throw where it makes sense #45637

Conversation

infinisil commented Aug 26, 2018 • edited Loading

Motivation for this change

Things done

Ericson2314 commented Aug 26, 2018

Mic92 Aug 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Profpatsch commented Aug 26, 2018

Mic92 commented Aug 26, 2018 • edited Loading

Profpatsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh commented Aug 27, 2018

infinisil commented Aug 27, 2018

infinisil commented Sep 18, 2018

Mic92 commented Sep 18, 2018

infinisil commented Apr 28, 2019

infinisil commented Aug 26, 2018 •

edited

Loading

Mic92 Aug 26, 2018 •

edited

Loading

Mic92 commented Aug 26, 2018 •

edited

Loading