Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib.path.hasPrefix: init #237610

Merged
merged 2 commits into from
Jun 21, 2023
Merged

lib.path.hasPrefix: init #237610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
866f75e
lib.path.append: Add a law
infinisil Apr 5, 2023
592213a
lib.path.hasPrefix: init
infinisil Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions lib/path/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ let
isPath
split
match
typeOf
;

inherit (lib.lists)
Expand All @@ -18,6 +19,7 @@ let
all
concatMap
foldl'
take
;

inherit (lib.strings)
Expand Down Expand Up @@ -100,6 +102,22 @@ let
# An empty string is not a valid relative path, so we need to return a `.` when we have no components
(if components == [] then "." else concatStringsSep "/" components);

# Type: Path -> { root :: Path, components :: [ String ] }
#
# Deconstruct a path value type into:
# - root: The filesystem root of the path, generally `/`
# - components: All the path's components
#
# This is similar to `splitString "/" (toString path)` but safer
# because it can distinguish different filesystem roots
deconstructPath =
let
recurse = components: base:
# If the parent of a path is the path itself, then it's a filesystem root
if base == dirOf base then { root = base; inherit components; }
else recurse ([ (baseNameOf base) ] ++ components) (dirOf base);
in recurse [];

in /* No rec! Add dependencies on this file at the top. */ {

/* Append a subpath string to a path.
Expand All @@ -108,6 +126,12 @@ in /* No rec! Add dependencies on this file at the top. */ {
More specifically, it checks that the first argument is a [path value type](https://nixos.org/manual/nix/stable/language/values.html#type-path"),
and that the second argument is a valid subpath string (see `lib.path.subpath.isValid`).

Laws:

- Not influenced by subpath normalisation

append p s == append p (subpath.normalise s)

Type:
append :: Path -> String -> Path

Expand Down Expand Up @@ -149,6 +173,51 @@ in /* No rec! Add dependencies on this file at the top. */ {
${subpathInvalidReason subpath}'';
path + ("/" + subpath);

/*
Whether the first path is a component-wise prefix of the second path.

Laws:

- `hasPrefix p q` is only true if `q == append p s` for some subpath `s`.

- `hasPrefix` is a [non-strict partial order](https://en.wikipedia.org/wiki/Partially_ordered_set#Non-strict_partial_order) over the set of all path values

Type:
hasPrefix :: Path -> Path -> Bool

Example:
hasPrefix /foo /foo/bar
=> true
hasPrefix /foo /foo
=> true
hasPrefix /foo/bar /foo
=> false
hasPrefix /. /foo
=> true
*/
hasPrefix =
path1:
assert assertMsg
(isPath path1)
"lib.path.hasPrefix: First argument is of type ${typeOf path1}, but a path was expected";
let
path1Deconstructed = deconstructPath path1;
in
path2:
assert assertMsg
(isPath path2)
"lib.path.hasPrefix: Second argument is of type ${typeOf path2}, but a path was expected";
let
path2Deconstructed = deconstructPath path2;
in
assert assertMsg
(path1Deconstructed.root == path2Deconstructed.root) ''
lib.path.hasPrefix: Filesystem roots must be the same for both paths, but paths with different roots were given:
first argument: "${toString path1}" with root "${toString path1Deconstructed.root}"
second argument: "${toString path2}" with root "${toString path2Deconstructed.root}"'';
Comment on lines +213 to +217
Copy link
Member Author

@infinisil infinisil Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not cause the function to fail, it should instead just return false (Edit: I don't think so anymore, see #237610 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all a bit messed up, because the reason this check is here is because we decided to limit the number of assumptions the path library makes to a minimum, see the design document. Notably we don't assume that there's only one filesystem root, meaning that theoretically paths could have no common ancestor. The reason for this is to ensure compatibility with how NixOS/nix#6530 implements lazy paths, introducing a way to have multiple filesystem roots.

But it's kind of ridiculous, Nixpkgs shouldn't have to ensure forwards compatibility with a potential future Nix version. We can't even test this code here right now without going through hoops!

On the other hand, having this check here ensures that we won't have any buggy behavior in case the above PR gets merged like that, and it doesn't cause any problems if the PR isn't merged either, so it's harmless and just a safety measure.

So because of that I'd say: Let's continue without the assumption of singular filesystem roots and have this check here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding throwing vs not throwing. I think it should definitely throw for now because:

  • We can always remove throws afterwards, it's backwards compatible
  • The notion of different filesystem roots is very unexplored, we aren't sure what the semantics should be like yet
  • I can imagine however that in most cases, this would indicate an user error, and it would be better to notify the user instead of "failing" silently
  • Code that needs to ensure it doesn't throw can still check whether the two paths have the same filesystem root beforehand

So in conclusion, no change is necessary here imo.

And I did test this code path at least locally:

❯ nix run github:edolstra/nix/lazy-trees -- repl -f lib
Welcome to Nix 2.17.0pre20230602_3c4d678. Type :? for help.

Loading installable ''...
Added 411 variables.
nix-repl> p = (builtins.fetchTree { type = "git"; url = ./.; }).outPath

nix-repl> path.hasPrefix p ./.                                         
error: lib.path.hasPrefix: Filesystem roots must be the same for both paths, but paths with different roots were given:
    first argument: "/home/tweagysil/antithesis/nixpkgs/" with root "/home/tweagysil/antithesis/nixpkgs/"
    second argument: "/home/tweagysil/antithesis/nixpkgs" with root "/"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We can always remove throws afterwards, it's backwards compatible

We'd break code that relies on this function for input validation. Just because it works for valid inputs doesn't mean that changing it won't lead to hard to troubleshoot problems for others. Basically your third point, but with a time / change aspect added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, though I'd argue that backwards compatibility conventionally only means "If it worked before it should work in the future", a one-way implication, so something not working now doesn't mean it will continue not working in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's kind of ridiculous, Nixpkgs shouldn't have to ensure forwards compatibility with a potential future Nix version. We can't even test this code here right now without going through hoops!

I kind of agree, but like you, I care too much about the future of lib.

The hoops are packaging that branch and using it in lib/release.nix. However, that would be irresponsible because of how broken it is. While I am confident that this particular behavior is good, users might adapt their programming to behaviors that are bad. Paths are already confusing to users, so we should discourage all adaptation to path related behaviors that will not be released.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users won't run into this error anyways right now though, the PR isn't merged yet, or might not even get merged with this behavior ever.

And if it were to get merged, I still think throwing an error here is better than returning false for the other reasons at least. Whether we want to change this behavior in the future and whether we should consider that backwards incompatible should be left for then, I don't think it makes sense to discuss this much here.

Do you agree that the code is fine as is?

take (length path1Deconstructed.components) path2Deconstructed.components == path1Deconstructed.components;


/* Whether a value is a valid subpath string.

- The value is a string
Expand Down
19 changes: 18 additions & 1 deletion lib/path/tests/unit.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
{ libpath }:
let
lib = import libpath;
inherit (lib.path) append subpath;
inherit (lib.path) hasPrefix append subpath;

cases = lib.runTests {
# Test examples from the lib.path.append documentation
Expand Down Expand Up @@ -40,6 +40,23 @@ let
expected = false;
};

testHasPrefixExample1 = {
expr = hasPrefix /foo /foo/bar;
expected = true;
};
testHasPrefixExample2 = {
expr = hasPrefix /foo /foo;
expected = true;
};
testHasPrefixExample3 = {
expr = hasPrefix /foo/bar /foo;
expected = false;
};
testHasPrefixExample4 = {
expr = hasPrefix /. /foo;
expected = true;
};

# Test examples from the lib.path.subpath.isValid documentation
testSubpathIsValidExample1 = {
expr = subpath.isValid null;
Expand Down
3 changes: 2 additions & 1 deletion lib/strings.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,8 @@ rec {
lib.strings.hasPrefix: The first argument (${toString pref}) is a path value, but only strings are supported.
There is almost certainly a bug in the calling code, since this function always returns `false` in such a case.
This function also copies the path to the Nix store, which may not be what you want.
This behavior is deprecated and will throw an error in the future.''
This behavior is deprecated and will throw an error in the future.
You might want to use `lib.path.hasPrefix` instead, which correctly supports paths.''
(substring 0 (stringLength pref) str == pref);

/* Determine whether a string has given suffix.
Expand Down