lib.strings: Prevent paths as inputs in some functions

[infinisil]

Description of changes

Some functions in lib.strings could lead to unexpected results when path data types are passed. This is confusing with lib.path now existing and being extended.

# Always returns false
nix-repl> lib.strings.hasPrefix ./lib ./lib
false

# No point, since absolute paths are always normalised, normalizePath is explicitly only useful for strings
nix-repl> lib.strings.normalizePath ./lib
"/nix/store/6d499zmgypxx73c7gdm3b36mkxy21iis-lib"

This work is sponsored by Antithesis ✨

[roberth]

Suggested change

	if isPath pref then throw ''
	lib.strings.hasPrefix: The first argument (${toString pref}) is a path, which is not supported''
	# Before 23.05, strings would be added to the store before comparing. This was too unexpected.
	if isPath pref then throw ''
	lib.strings.hasPrefix: The first argument (${toString pref}) is a path value, but only strings are supported.''

Leave a clue in case users encounter this in their code base, so they know something has changed here. It appears in the code block of the stack trace iirc. The exception message does not include the whole history, because I don't expect that to be productive. It could stick around for years being unnecessarily noisy.

[infinisil]

Sounds good, but "only strings are supported" isn't entirely correct, all stringLike values (including outPath, __toString) are supported, except paths. Maybe that's good enough though

[roberth]

Right. Probably best to keep it simple and avoid unnecessary dissonance?
This can only be cured with poetry. Cue ChatGPT:

Here is a revised limerick about type coercions in programming:

Type coercion can be quite slick,
To convert data types that don't mix.
But if you're not careful,
Your code can be awful,
And bugs will give your code a big kick.

[infinisil]

Haha nicely done 😆

I agree, let's keep it simple, I applied this suggestion to all the error messages (also the normalizePath one).

[fricklerhandwerk]

This is a breaking change and at the very least needs release notes. Almost certainly this will error out on someone in production. We may even want to phase this in with a warning or setting. Who else should we ask to make that decision? @roberth opinions?

[infinisil]

Hmm yeah I guess it's a breaking change, even if the code we break is certainly a bug (in case of hasPrefix), or unnecessary (in case of normalizePath).

I changed the errors to a warning instead and added some more explanatory text to them.

[fricklerhandwerk]

Have to take the space bar heaters into account... been there myself.

[infinisil]

I also noticed that removePrefix and removeSuffix suffered from the same problems, so I added the warning to those as well.

[infinisil]

I think this looks good and would like to merge it soon

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-47/27387/1