Skip to content

lib.types.stringLike: init#256353

Open
figsoda wants to merge 1 commit intoNixOS:masterfrom
figsoda:string-like
Open

lib.types.stringLike: init#256353
figsoda wants to merge 1 commit intoNixOS:masterfrom
figsoda:string-like

Conversation

@figsoda
Copy link
Copy Markdown
Member

@figsoda figsoda commented Sep 20, 2023

Description of changes

Motivation: microvm-nix/microvm.nix#138

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Sep 20, 2023
@figsoda figsoda requested a review from roberth September 20, 2023 20:13
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 20, 2023
@ofborg 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 Sep 20, 2023
Copy link
Copy Markdown
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I wonder, could types.str (and all types on top of it) use check = isStringLike and coerce using toString?

Also, are there more than just the linked use-case?

@roberth
Copy link
Copy Markdown
Member

roberth commented Sep 21, 2023

coerce using toString?

I'd expect a string interpolation instead. Path values are generally turned into store paths first.

EDIT: in the context of str. stringLike should probably preserve the definition value, unchanged.

@figsoda
Copy link
Copy Markdown
Member Author

figsoda commented Sep 21, 2023

I wonder, could types.str (and all types on top of it) use check = isStringLike and coerce using toString?

As @roberth said, it might be a bit unintuitive whether str coerces things using toString or interpolation, meanwhile stringLike preserving the original type seems fitting for the name

Also, are there more than just the linked use-case?

I can see str being used as a stricter version of stringLike when store paths are not ideal, e.g. names and descriptions might want to use str, but environment variables and possibly relative directory paths might want to use stringLike

Copy link
Copy Markdown
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I think we should have more descriptive types than this, "string-like" is too arbitrary, mainly just based on what Nix builtins consider strings. This doesn't really have a coherent meaning in e.g. a NixOS system.

For the issue you linked, it seems like a path type specifically for runtime paths would be better, e.g. types.runtimePath. You might also want to allow relative paths, types.relativePath or types.subpath (see also lib.path.subpath.normalise and lib.path.subpath.isValid). If you need to accept both, this could be either runtimePath relativePath. These are just some quick ideas, and other use cases might have other types that would be more fitting. It would be great to spend some time to think about this.

And in any case, there should be docs and tests for any new types.

@figsoda
Copy link
Copy Markdown
Member Author

figsoda commented Sep 21, 2023

force pushed to add docs and tests

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation labels Sep 21, 2023
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 21, 2023
@roberth
Copy link
Copy Markdown
Member

roberth commented Sep 22, 2023

-1 on "runtime" because it not defined in a purely Nix context, and even in a Nixpkgs context, a dev output would not be runtime (I guess), but I see no reason to distinguish it from other store paths. I don't see how that would be a useful distinction.

stringLike seems like a good alternative for types like oneOf [str pathInStore package]. Indeed that's quite broad, but that's often what we need in awkwardly generic places like environment variable values.

I don't believe having stringLike would be bad. If we have related alternatives, we can add them in follow-up PRs and document how they relate.

@infinisil
Copy link
Copy Markdown
Member

I guess more concretely, lib.strings.isStringLike would also accept path values, which when treated as a string get imported into the store, which is far from obvious from the type name. This is in comparison to e.g. types.pathInStore, which perhaps unexpectedly doesn't allow arbitrary path values.

Imo we have too many pathy types with unclear semantics already:

  • types.path: Accepts anything string-coercible, including path values, but doesn't determine whether path values get imported or not
  • types.pathInStore: Accepts anything string-coercible that when coerced is under the Nix store. Counter-intuitively doesn't accept arbitrary paths even though they'd turn into a store path if coerced to a string. Does accept Nix store path literals.
  • types.package: Accepts derivations and anything string-coercible that when coerced is a Nix store path. As with types.pathInStore, doesn't accept arbitrary paths, even though the merge function deceptively has logic to handle paths. The return value always looks like a derivation, though is actually just a fake derivation if a non-derivation was passed.
  • types.shellPackage: Almost like package, but only accepts derivations (and they need the shellPath attribute).
  • types.str: Only accepts string values

This is just a mess, and stringLike would just add to that.

Instead we should work towards a more clear set of types that have clearer semantics, including how path values get handled, what the intended use case is, ensuring the return value is always the same type, with the same semantics no matter the input type.

@roberth
Copy link
Copy Markdown
Member

roberth commented Sep 22, 2023

Doesn't seem messier than the fairly extensive problem domain we model with these types, but if you have concrete suggestions or plans to improve it, feel free.

@infinisil
Copy link
Copy Markdown
Member

Concretely I just have one idea:
All types should return the same result structure every time, no matter what the definitions are. So e.g. for types accepting string-like values and paths, they should always return e.g. strings and nothing else.

More generally I'd like a design document for why we need this many string/path-like types, comparing them and specifying their use cases. And if we can't find a good justification for them, rename/document the types more properly or introduce new types with better semantics.

@roberth
Copy link
Copy Markdown
Member

roberth commented Sep 22, 2023

I see that the motivating issue was better solved with str, so it seems that this PR is not urgent (and generally it's ok for types to be defined locally until properly reviewed etc).

Plan sgtm @infinisil. I'll add that the is* functions should also be considered in the design and/or explanation and/or improvements, as this type and name are meant to align with isStringLike.

As for the design part (ie making changes) we might want to gather requirements in the form of simple use cases first so we can more easily test designs we come up with.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 1, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 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. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

7 participants