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

Add 'poison' context #10212

Closed
wants to merge 2 commits into from
Closed

Add 'poison' context #10212

wants to merge 2 commits into from

Conversation

9999years
Copy link
Contributor

@9999years 9999years commented Mar 10, 2024

This can be added to strings to prevent them from being used in derivations. This lets us implement "unstable" functions without fear!

Motivated by #10206.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Mar 10, 2024
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I like the idea, and I've linked my description/proposal issue in the PR description.

Besides the review comments, I have two points of attention

  • builtins.unsafeDiscardStringContext: does it throw when it encounters the poison? I think it should (despite the unsafe qualifier)

  • This adds a new cross-cutting responsibility to the evaluator and its callers. What can we do to make the Value and string context API safer? Perhaps the raw string value could be encapsulated behind getters that make the intent of the caller clear.

    • No context allowed
    • Normal dependencies (store paths, outputs) allowed
    • Diagnostics and dependencies allowed

src/libexpr/value/context.hh Outdated Show resolved Hide resolved
src/libexpr/eval-error.cc Outdated Show resolved Hide resolved
@L-as
Copy link
Member

L-as commented Mar 10, 2024

You can still use them in derivations with some processing, albeit it's going to be much harder.
Why not just build the functionality directly into builtins.trace, if you're only going to pass it to builtins.trace?

@lf-
Copy link
Member

lf- commented Mar 10, 2024

* `builtins.unsafeDiscardStringContext`: does it throw when it encounters the poison? I think it should (despite the unsafe qualifier)

Personally I think you should be allowed to deliberately shoot yourself in the foot, lest we accidentally create a need for alternative string context discarding functions. The point of this to me is to stop mistakes, rather than limit possible expression.

@9999years
Copy link
Contributor Author

Why not just build the functionality directly into builtins.trace, if you're only going to pass it to builtins.trace?

Assuming "the functionality" is "pretty-printing values for debugging":

builtins.trace already prints values in debug-form when you pass it a single value, but often (especially in the case of warning messages) you want to pass it a string containing an interpolated value:

https://github.com/NixOS/nixpkgs/blob/4907e7a50c83b6214239d30a833f59493754eb48/nixos/modules/misc/documentation.nix#L54
https://github.com/NixOS/nixpkgs/blob/4907e7a50c83b6214239d30a833f59493754eb48/lib/meta.nix#L143
https://github.com/NixOS/nixpkgs/blob/4907e7a50c83b6214239d30a833f59493754eb48/pkgs/top-level/release-attrpaths-superset.nix#L160

In that case, you're stuck with the default toString function (which errors when passed many sorts of values). To work around this, nixpkgs has implemented a kind of crappy pretty-printer themselves:

https://github.com/NixOS/nixpkgs/blob/4907e7a50c83b6214239d30a833f59493754eb48/lib/generators.nix#L355-L368

builtins.toStringDebug will also be useful in assert messages/errors (builtins.throw, lib.assertMsg).

@L-as
Copy link
Member

L-as commented Mar 11, 2024

You could make string interpolation lazy, and then depending on how you use the resulting string, you could get the behavior you want. So builtins.trace "${x}" y could show a different string rather than builtins.derivation { src = "${x}"; }. In fact, for a string interpolation, you could carry around the expanded version with version-dependent debug information along with the original one, using the original one for all operations except the ones that print it to the user.

I'm not sure this complexity is worth it, but I don't think it would be very complex to implement.

@roberth
Copy link
Member

roberth commented Mar 11, 2024

Why not just build the functionality directly into builtins.trace, if you're only going to pass it to builtins.trace?

We could make it accept a "data-only" DSL. Maybe not go overboard with tons of functionality like #7795, but maybe implement what this issue calls MVP.

Not sure if we want to force such a representation on library authors, so maybe there's a case for both solutions to exist eventually.

This can be added to strings to prevent them from being used in
derivations. This lets us implement "unstable" functions without fear!
@9999years 9999years closed this Mar 12, 2024
@9999years 9999years deleted the poison-context branch March 12, 2024 23:27
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-03-11-nix-team-meeting-132/42960/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poison pill string context
5 participants