lib/generators: uninline recursion depth tracking by K900 · Pull Request #168327 · NixOS/nixpkgs

K900 · 2022-04-11T21:02:28Z

As cute as it is, it breaks in "interesting" ways when combined with nixpkgs' magic __functor attribute and friends.

This has notably caused pretty-printing pkgs.rust to fail, making at least two users (me included) extremely confused.

There's probably a better approach here involving more nasty self-recursion and a wrapper, but let's cross that bridge when we come to it.

To reproduce: pkgs.lib.options.showDefs [ { file = "foo"; value = pkgs.rust.packages.stable.buildRustPackages; } ].

Description of changes

Removed the withRecursive wrapper, moved all the step-counting into toPretty, lowered the default depth to a more reasonable amount.

Ping @Profpatsch and @Ma27 as originators of the function.

Things done

K900 · 2022-04-12T06:45:21Z

Fixed the tests.

ckiee · 2022-04-12T06:50:16Z

Perhaps relevant for #167388?

alyssais · 2022-04-12T06:50:26Z

lib/strings.nix

I think this might be more useful with the arguments round the other way? I could imagine myself currying repeat 3 more than repeat "str".

Makes sense. Done.

Ma27 · 2022-04-12T09:34:54Z

The problem is quite simple, if { __functor = ... } is the last value to be evaluated, __functor will be replaced by <unevaluated> thus breaking isFunction.

Currently preparing a patch to fix this.

K900 · 2022-04-12T09:40:13Z

The same can also happen with __pretty and any other "magic" type that gets added later. I'm not sure trying to special case these is worth it.

Ma27 · 2022-04-12T10:28:37Z

Well, this issue only applies to "magic" attributes that are used within toPretty, right? I.e. __functionArgs, __functor, __pretty and __toString.

K900 · 2022-04-12T10:29:43Z

Yes, but more of those could be added at the same time.

Closes NixOS#168327 The issue reported there can be demonstrated with the following expression: → nix-instantiate --eval -E "with import ./. {}; pkgs.lib.options.showDefs [ { file = \"foo\"; value = pkgs.rust.packages.stable.buildRustPackages; } ]" error: attempt to call something which is not a function but a string at /home/ma27/Projects/nixpkgs/lib/trivial.nix:442:35: 441| isFunction = f: builtins.isFunction f || 442| (f ? __functor && isFunction (f.__functor f)); | ^ 443| Basically, if a `__functor` is in an attribute-set at depth-limit, `__functor` will be set to `"<unevaluated>"`. This however breaks `lib.isFunction` which checks for a `__functor` by invoking `__functor` with `f` itself. The same issue - "magic" attributes being shadowed by `withRecursion` - also applies to others such as `__pretty`/`__functionArgs`/`__toString`. Since these attributes have a low-risk of causing a stack overflow (because these are flat attr-sets or even functions), ignoring them in `withRecursion` seems like a valid solution.

Ma27 · 2022-04-12T11:43:57Z

First of all, people should be careful with that, there's no convention that prevents people from using __ on their own and thus we'd potentially break existing Nix code.

Second, if the new magic attribute affects pretty logic, you'd have to touch lib.generators.toPretty already, so I don't see that much of an issue here.

However don't get me wrong, I understand your point! I'll file a PR with my change anyways as an alternative, but it'd be understandable from my PoV if your's gets merged in favor of mine :)

Another option is btw to special-case each __ attribute, but I think that would cause too much confusion since e.g. stdenv also uses these attributes on derivations.

K900 · 2022-04-12T11:46:02Z

Am I correct in assuming that you want to keep withRecursive separate for reusability? I think we can get the best of both worlds here if we do some combinator crimes. I'll try and write up a prototype later tonight.

Ma27 · 2022-04-13T10:20:36Z

I think we can get the best of both worlds here if we do some combinator crimes

Can you elaborate what you exactly mean with "combinator crimes", please?

Am I correct in assuming that you want to keep withRecursive separate for reusability

tl;dr yes, recursion limits and pretty-printing are actually two different things and thus were separated, see also the discussion in #131205 (and the linked tickets).

K900 · 2022-04-13T10:25:33Z

We can make withRecursionLimit a "decorator" of sorts that wraps the serializer function and passes the wrapped copy to itself to allow for recursion. It will make writing serializers slightly more awkward, but the depth tracking will be actually correct.

K900 · 2022-04-13T10:57:04Z

@Ma27 try the latest commit

Profpatsch · 2022-04-16T12:07:57Z

I think @infinisil added that originally?

Profpatsch · 2022-04-16T12:11:10Z

lib/strings.nix

We shouldn’t introduce a new stdlib function in an unrelated commit.

Could always inline this, but it sounds like something that's both simple, slightly non-trivial and generally useful. Maybe I can split this off into a separate commit or even PR instead?

A separate PR sounds reasonable. But even just having a separate commit is better than bunching a new library function with some other changes.

Profpatsch · 2022-04-16T13:01:28Z

lib/generators.nix

This is a breaking API change of an exposed library function, we can’t do that.

You need to keep the API the same; e.g. making some of the options no-ops should also work.

That's not really viable here unfortunately - the old version took an object to be inspected, and the new one takes a function to be wrapped. I guess we can make a withRecursionLimit', or even a _withRecursionLimit'?

As I said, I don’t think we should break the API without good cause.

Or rather we shouldn’t break the API, period :)

Yes, what I'm saying is, keep the old function as it is, introduce the new one without exposing it.

lib/strings.nix

Profpatsch · 2022-04-20T15:54:42Z

lib/strings.nix

The example should probably contain a string that is longer than one character.

K900 · 2022-04-23T10:59:43Z

Updated. Split strings.repeat into its own commit (along with the tests), restored the old withRecursion function and added new withRecursionLimit (name subject to bikeshedding); this keeps everything API compatible, except the output of toPretty, which I sincerely hope no one actually relies on.

Avoids weird issues when recursing into magic attributes like __functor and __pretty by counting recursions only when actually necessary.

Ma27 · 2022-04-23T11:06:34Z

I think @infinisil added that originally?

@Profpatsch I added it originally it in #131205 and also filed a PR #168374 to fix it using withRecursion.

However, @K900 brought some valid concerns up regarding my solution, so I'd leave it up to you to decide :)

(sorry for my late replies, was sick recently and had a lot of other stuff to take care of after that)

K900 · 2022-04-23T11:10:32Z

Force-pushed again to fix some weird mismerge - my local copy of nixpkgs was behind the laptop I wrote the "generator crimes" PoC on.

K900 requested review from Profpatsch, edolstra, infinisil and nbp as code owners April 11, 2022 21:02

K900 mentioned this pull request Apr 11, 2022

[RFC] types.listOf: actually validate the contents #168295

Closed

13 tasks

alyssais reviewed Apr 12, 2022

View reviewed changes

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 Apr 12, 2022

Ma27 mentioned this pull request Apr 12, 2022

lib/generators: withRecursion: don't break attr-sets with special attrs #168374

Merged

13 tasks

Profpatsch suggested changes Apr 16, 2022

View reviewed changes

Profpatsch reviewed Apr 20, 2022

View reviewed changes

lib/strings.nix Outdated Show resolved Hide resolved

Profpatsch reviewed Apr 20, 2022

View reviewed changes

lib/strings.nix Outdated

Copy link

Member

Profpatsch Apr 20, 2022

Choose a reason for hiding this comment

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

The example should probably contain a string that is longer than one character.

lib/strings: add repeat helper

2a054da

K900 requested review from Profpatsch and alyssais April 23, 2022 10:59

lib/generators: rework recursion limit tracking

e4d7dc4

Avoids weird issues when recursing into magic attributes like __functor and __pretty by counting recursions only when actually necessary.

infinisil closed this in #168374 May 23, 2022

K900 mentioned this pull request Jun 23, 2022

[2018-2024] Nixpkgs committer nominations NixOS/nixpkgs-committers#60

Closed

Uh oh!

Conversation

K900 commented Apr 11, 2022

Description of changes

Things done

Uh oh!

K900 commented Apr 12, 2022

Uh oh!

ckiee commented Apr 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ma27 commented Apr 12, 2022

Uh oh!

K900 commented Apr 12, 2022

Uh oh!

Ma27 commented Apr 12, 2022

Uh oh!

K900 commented Apr 12, 2022

Uh oh!

Ma27 commented Apr 12, 2022

Uh oh!

K900 commented Apr 12, 2022

Uh oh!

Ma27 commented Apr 13, 2022

Uh oh!

K900 commented Apr 13, 2022

Uh oh!

K900 commented Apr 13, 2022

Uh oh!

Profpatsch commented Apr 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

K900 commented Apr 23, 2022

Uh oh!

Ma27 commented Apr 23, 2022

Uh oh!

K900 commented Apr 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants