lib/attrsets: make toDerivation x always work when isStorePath x

[sternenseemann]

Motivation for this change

isStorePath may be true for anything which isCoercibleToString, however
there seems to be different coercion behavior within nix builtins: a
list of derivations can be coerced to a string with toString, but
builtins.storePath will throw:

nix-repl> builtins.storePath [ pkgs.hello ]
error: cannot coerce a list to a string, at (string):1:1

nix-repl> toString [ pkgs.hello ]
"/nix/store/0pisd259nldh8yfjvw663mspm60cn5v8-hello-2.10"

This means that lib.strings.isStorePath [ pkgs.hello ] == true, but
lib.toDerivation [ pkgs.hello ] will throw. This causes very hard to
debug evaluation errors when mistakenly putting a list in a place where
types.package was expected since it expects to be able to call
toDerivation on anything for that isStorePath returns true. The
evaluation errors for this issue are terrible as the only identifiable
location points to toDerivation and no offending values are printed
since types.package doesn't actually treat this situation as a type
error.

This is however fixed easily: Just call toString before passing the
value to builtins.storePath, so that builtin receives the same value
isStorePath does its checks on.

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.

[domenkozar]

A better fix would be to error out when a list is passed. This way you might slip in a list of numbers and get really confusing error message.

[sternenseemann]

This is already done by isStorePath:

> lib.strings.isStorePath [ 1 2 3 4 5 ]
false

The issue I'm fixing is only the edge case where you would pass a list like [ hello ] or [ "/nix/store/" "n58zy0srl2zj8s4ydf8wz7dsvkq2ad6w-hello-2.10" ] which appear to be a valid store path after being coerced to a string. In this case the type check for lib.types.package would succeed (because it uses isStorePath), but the actual toDerivation in the merge operation of the type would fail.

I'm not sure that it's necessarily a good thing you can pass lists here, but the impact is somewhat limited since the resulting coerced string needs to be of a certain form to type check. If we wanted to rewrite the type check however, we'd likely want to get rid of the string coercion, but this is a more sensitive thing to do than my fix, as you'd need to be careful not to break existing configurations.

For reference, this is the definition of the package type:

nixpkgs/lib/types.nix

Lines 330 to 337 in d01c15e

	# derivation is a reserved keyword.
	package = mkOptionType {
	name = "package";
	check = x: isDerivation x || isStorePath x;
	merge = loc: defs:
	let res = mergeOneOption loc defs;
	in if isDerivation res then res else toDerivation res;
	};

[infinisil]

I think we should go the other direction: Make isStorePath fail when it was passed a list. While a single-element list kind of makes sense, in general using lists for this doesn't make sense. E.g.

nix-repl> lib.isStorePath ["/nix/store/" "foo" "bar"]
true

nix-repl> toString ["/nix/store/" "foo" "bar"]        
"/nix/store/ foo bar"

[sternenseemann]

Done! Opted for a very simple extra check.

[infinisil]

Is this still needed though? I wouldn't think so

[sternenseemann]

You're right, dropped that commit. It does everything we want (__toString, derivations, strings, paths).