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.

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

Should path values be deprecated? #7338

Open
infinisil opened this issue Nov 23, 2022 · 8 comments
Open

Should path values be deprecated? #7338

infinisil opened this issue Nov 23, 2022 · 8 comments
Labels
breaking Changes we can't make without breaking old expressions, changing hashes, etc question

Comments

@infinisil
Copy link
Member

infinisil commented Nov 23, 2022

The Nix language has path expressions which evaluate to path values. I believe it might be good to deprecate path values and eventually make path expressions return string values instead.

Reasons why path values aren't great:

  • Their store import semantics are implicit and non-obvious. This makes it really hard for especially new people to understand paths, this relates to Make string interpolation and builtins.toString behave the same #7327:
    • "${some/path}" imports the path into the store
    • "${toString some/path}" doesn't import it into the store
  • .. path components get normalised for no good reason (which can be wrong if symlinks are involved, which Nix doesn't check for):
    nix-repl> /does/not/exist                 
    /does/not/exist
    
    nix-repl> /does/not/exist + "/foo/../bar"
    /does/not/exist/bar
    
  • Are unintuitive to work with due to trailing / getting normalised, see also Overload / to mean path append; alternative to path + "/" + "foo" footgun #7301:
    nix-repl> /foo + "/" + "bar"
    /foobar
    
    nix-repl> /foo + ("/" + "bar")
    /foo/bar
    

Meanwhile, there's essentially no good use cases for path values:

  • builtins.path can be used to explicitly import a path into the store, which would make it much more obvious when it happens and it can't happen accidentally.
  • Builtins don't require path values, they work with strings too, and even more correctly due to the .. not getting normalised away

Ignoring deprecation for now, I think ideally path expressions should return string values instead, so like an implicit toString. This then requires people to be very explicit about what gets imported into the store, either using builtins.path or some library function built on top (like lib.cleanSource).

See also #7335 and NixOS/nixpkgs#200718, ping @roberth @fricklerhandwerk

@infinisil infinisil added the bug label Nov 23, 2022
@edolstra
Copy link
Member

While there are a few warts with path values, I don't see how that justifies removing them. How would you distinguish between the derivation attributes

builder = ./builder.sh;

vs

builder = "./builder.sh";

And if the idea is to keep the path syntax but not path values: that doesn't work for lazy trees, where ./builder.sh is a tuple that points to the source tree accessor and the path relative to the root of that accessor.

builtins.path can be used to explicitly import a path into the store, which would make it much more obvious when it happens and it can't happen accidentally.

We're not going to prefix builtins.path to every file reference in Nixpkgs. Nix is a DSL for writing Nix packages concisely, and this change would make Nix packages much more annoying to write.

@edolstra edolstra removed the bug label Nov 25, 2022
@roberth
Copy link
Member

roberth commented Nov 27, 2022

I agree with @edolstra's answer and it is a sufficient answer to the question as posed, so I won't go into all that was said, but I think a few underlying concerns could still be addressed.

Reasons why path values aren't great:

I couldn't easily find the section of the manual that explains path interpolation / antiquotation.

Actionable:

  • .. path components get normalised for no good reason (which can be wrong if symlinks are involved, which Nix doesn't check for):
    nix-repl> /does/not/exist                 
    /does/not/exist
    
    nix-repl> /does/not/exist + "/foo/../bar"
    /does/not/exist/bar
    

Operating on the logical model keeps the path logic simple. Both the logical and physical model are valid, and either choice would lead to confusion. I do not believe that a switch is warranted for this reason alone, but also this would be even more breaking, because currently path values can also be used for manipulating paths that aren't relative to the evaluating system's filesystem root. By assuming that they are, we're introducing a perhaps rare, but very surprising impurity in such code.

Actionable:

  • Document that path value operations work on paths, and not on the file system.

Actionable:

  • Builtins don't require path values, they work with strings too, and even more correctly due to the .. not getting normalised away

Builtins do not work with relative paths, because they don't have an ambient base directory to work from. Path values bridge the gap between the relative path syntax and the builtins. To keep supporting this is in line with Eelco's answer.

Finally, making such a breaking change breaks its ability to evaluate older expressions, which would seriously impact Nix's applicability in a number of valuable areas.

@fricklerhandwerk
Copy link
Contributor

The documentation for + is incorrect https://nixos.org/manual/nix/stable/language/operators.html

@roberth do you mean that it's missing the path + string? case?

@roberth
Copy link
Member

roberth commented Nov 29, 2022

@fricklerhandwerk Yes. Edited.

@fricklerhandwerk
Copy link
Contributor

@roberth This #7278 adds a complete section on string interpolation, which also mentions copying paths to the store (which has been there for a while actually).

I hope to find time to eventually rework that by reusable including text sections in multiple locations, because right now that information only lives in one place but should actually be available both in the data type references as well as the string interpolation section. But that would be too much of a change at once.

@roberth
Copy link
Member

roberth commented Dec 13, 2022

That's a good improvement, but it's still only from the string perspective.

We'll also need a section that focuses on path values. ${path} is behavior that's relevant to both types, but there's plenty more to say about paths that's not about string interpolation.

@fricklerhandwerk
Copy link
Contributor

@roberth, I also opened #7498 to account for operator behavior.

Before I proceed to opening an entire section just on paths, the glossary, data types section, and possibly architecture chapter need some more work. Then we can just refer to existing explanations of terms or mechanisms, and focus on what makes paths special in Nix.

@tejing1
Copy link

tejing1 commented Jan 23, 2023

Don't remove path-types, but do make the concepts consistent. In particular, I would recommend that the current function of toString on path-types be relegated to a name clearly not intended for normal use, like builtins.unsafeConvertPathToString, and toString should behave identically to the way string interpolation already does.

We almost have a consistent rule in the nix language that path-types are resolved at eval time and strings containing paths are resolved at run time (or sometimes build time). toString breaks this, and I consider that a bug, especially since the non-functionality of "${5}" and some other such things tends to push people to start using toString as a rule, whether they need it or not, without being aware of this semantic quirk.

If the language was consistent in this, and the rule was clearly stated in documentation and introductions to the language, I think newbies wouldn't have too much trouble with the concept, despite its uniqueness. It's something that's obviously very valuable for the nix language's domain of use.

@roberth roberth added the breaking Changes we can't make without breaking old expressions, changing hashes, etc label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Changes we can't make without breaking old expressions, changing hashes, etc question
Projects
None yet
Development

No branches or pull requests

5 participants