lib.strings: Prevent paths as inputs in some functions by infinisil · Pull Request #221204 · NixOS/nixpkgs

infinisil · 2023-03-14T19:18:54Z

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 ✨

lib/strings.nix

roberth · 2023-03-14T22:10:02Z

lib/strings.nix

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.

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

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.

Haha nicely done 😆

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

lib/strings.nix

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?

lib/strings.nix

infinisil · 2023-03-14T23:23:28Z

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.

lib/strings.nix

fricklerhandwerk · 2023-03-15T00:25:11Z

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

lib.{hasPrefix,hasInfix,hasSuffix} would otherwise return an always-false result, which can be very unexpected: nix-repl> lib.strings.hasPrefix ./lib ./lib/meta.nix false

There's no need to call this function on path data types, and it's confusing with the new lib.path library functions

infinisil · 2023-03-15T18:51:05Z

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

…rguments See also parent commits

infinisil · 2023-03-21T17:35:50Z

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

nixos-discourse · 2023-04-18T13:10:22Z

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

infinisil requested a review from edolstra as a code owner March 14, 2023 19:18

infinisil requested review from fricklerhandwerk and roberth March 14, 2023 19:19

This was referenced Mar 14, 2023

Path library functions #210426

Closed

lib.path.hasPrefix, lib.path.removePrefix: init #210423

Closed

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 14, 2023

roberth reviewed Mar 14, 2023

View reviewed changes

infinisil force-pushed the deprecate-paths-to-strings branch 2 times, most recently from f2d6bd4 to d9cbdc5 Compare March 14, 2023 22:29

fricklerhandwerk reviewed Mar 14, 2023

View reviewed changes

lib/strings.nix Outdated Show resolved Hide resolved

infinisil force-pushed the deprecate-paths-to-strings branch from d9cbdc5 to 7101e4b Compare March 14, 2023 23:22

infinisil force-pushed the deprecate-paths-to-strings branch 3 times, most recently from addf8c4 to c518535 Compare March 14, 2023 23:42

fricklerhandwerk reviewed Mar 15, 2023

View reviewed changes

lib/strings.nix Outdated Show resolved Hide resolved

infinisil added 2 commits March 15, 2023 19:42

lib.strings: Deprecate path prefix/suffix/infix arguments

99bc90e

lib.{hasPrefix,hasInfix,hasSuffix} would otherwise return an always-false result, which can be very unexpected: nix-repl> lib.strings.hasPrefix ./lib ./lib/meta.nix false

lib.strings.normalizePath: Deprecate for path values

5e8b9de

There's no need to call this function on path data types, and it's confusing with the new lib.path library functions

infinisil force-pushed the deprecate-paths-to-strings branch from c518535 to 5e8b9de Compare March 15, 2023 18:42

lib.strings.remove{Prefix,Suffix}: Deprecate for path prefix/suffix a…

61012f6

…rguments See also parent commits

infinisil force-pushed the deprecate-paths-to-strings branch from 15a8230 to 61012f6 Compare March 15, 2023 18:51

infinisil requested a review from roberth March 21, 2023 17:35

fricklerhandwerk approved these changes Mar 21, 2023

View reviewed changes

infinisil mentioned this pull request Mar 21, 2023

[WIP] lib.sources.predicateFilter: init #221361

Closed

infinisil merged commit 4f35a58 into NixOS:master Apr 4, 2023

infinisil deleted the deprecate-paths-to-strings branch April 4, 2023 16:35

infinisil mentioned this pull request Nov 7, 2023

Don't expose all functions in the top-level lib, prefer sub-libraries #266103

Open

-    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.''

Uh oh!

Comments

Conversation

infinisil commented Mar 14, 2023

Description of changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roberth Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

roberth Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fricklerhandwerk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

infinisil commented Mar 14, 2023

Uh oh!

Uh oh!

fricklerhandwerk commented Mar 15, 2023

Uh oh!

infinisil commented Mar 15, 2023

Uh oh!

infinisil commented Mar 21, 2023

Uh oh!

nixos-discourse commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fricklerhandwerk left a comment •

edited

Loading