Rewrite meta-check.nix type checking logic. #37252

Profpatsch · 2018-03-17T15:20:10Z

Description of the main commit:

stdenv/generic/check-meta: rewrite type checking code  …
The previous code used some ad-hoc attribute enumeration to check for missing
fields and did not really type check the attributes, because it used `t.check`
from the module system types, which only does shallow checks for nested types,
e.g. just `builtins.isList` for (listOf strings).

This new version uses `types-simple` to recursively check every attribute,
outputting accurate information for type errors in nested fields, e.g.:

Package ‘hello-2.10’ in … has an invalid meta attrset:
maintainers.1.email should be: string
but is: null

When we set
meta.maintainers [ eelco foobar ];
in `hello/default.nix` and foobar is defined as
{ name = "foobar"; email = null; };
in `maintainers-list.nix`.

The path accurately pins down where the type check failed:
In the `maintainers` field, the second element of the list (counting from 0)
and the field email in that element.

Further work: If an unsupported meta field is used, the type error will print
the complete type of the meta product, which is quite big. The type error should
show a diff of the problematic fields in that case.
This is possible but not yet integrated into the type checking logic.

~~Please go trough the commits one-by-one, since they all add some pretty different changes and are built on each other as single units.~~
Adding the type library might be a bit controversial, but I hope I have provided enough incentive in the docstrings and the commit messages to persuade you that it is a good addition.

Meta can be checked with this invocation from the ofborg readme.

Profpatsch · 2018-03-17T17:00:12Z

Okay, with the latest commit I recommend looking at lib/types-simple.nix in the overview, since it should be a lot easier to understand with these comments.

aszlig

Great work on the type system, although I'm not quite sure whether this is a good idea to use for checking meta, because we already have to fight against eval time.

Apart from that, it could also make sense to replace the type checking for the NixOS module system to use this instead (as it avoids the need to use tryEval for checking) on everything but submodules, maybe @nbp could chime in for that as well.

Other than that, I think adding another type system needs to have more eyes on this, so I'd suggest adding this as a separate pull request.

aszlig · 2018-03-18T06:28:22Z

lib/types-simple.nix

+  # and it returns a more or less pretty string of errors.
+  prettyPrintErrors =
+    let
+      join = lib.foldl lib.concat [];


Why not concatLists (which is a primop)?

Good catch, will change.

aszlig · 2018-03-18T06:32:04Z

pkgs/stdenv/generic/check-meta.nix

-  checkMeta = meta: if shouldCheckMeta then lib.remove null (lib.mapAttrsToList checkMetaAttr meta) else [];
+  checkMeta = meta:
+    if shouldCheckMeta
+    then lib.types-simple.checkType metaType meta


Did you actually measure the evaluation time overhead this has?

Nope, how would I measure it? Maybe cc @grahamc.

Right now the checks are not enabled by default, of course that would be a final goal.

aszlig · 2018-03-18T06:44:47Z

lib/types-simple.nix

+
+  ## -- HELPERS --
+
+  unimplemented = abort "unimplemented";


This is not used anywhere, maybe remove?

aszlig · 2018-03-18T06:55:46Z

lib/types-simple.nix

+
+  ## -- TYPES --
+
+  # the type with no inhabitants (kind of useless …)


So why have it in the first place?

Ah, it’s used in the tests for example: (list void) is a type with exactly one inhabitant, []. One wouldn’t believe it, but void is a pretty handy type to have (the description is a bit tongue-in-cheek).

aszlig · 2018-03-18T07:01:01Z

lib/types-simple.nix

+  };
+
+  # the type with exactly one inhabitant
+  unit = mkScalar {


Why do we need this type?

Similar to void this doesn’t seem useful at first, but it’s even more elementar than bool in a sense; e.g. the void you find in languages like C(++) is really a unit.

A great use-case is for sum types:

let mode = sum { off = unit; level = int; }; in checkType mode { off = {}; }

which corresponds to the Haskell type:

data Mode = Off | Level Int

aszlig · 2018-03-18T07:03:58Z

lib/types-simple.nix

+  # list (attrs unit):
+  #   [ { a = {}; } { b = {}; } ]
+  #   []
+  list = t: mkRecursive {


Maybe call this listOf so we use the same naming as with NixOS modules.

I deliberately changed the names to be internally consistent; the module system has string and str, because string did strange concatenation. listOf and attrsOf have the same history with attrs and list, which are not able to recursively check their contents.

Compare the correspondence table below.

aszlig · 2018-03-18T07:04:18Z

lib/types-simple.nix

+  # attrs (attrs string):
+  #  { foo.bar = "hello"; baz.quux = "x"; }
+  #  { x = { y = "wow"; }; }
+  attrs = t: mkRecursive {


Same here: attrsOf

aszlig · 2018-03-18T07:07:28Z

lib/types-simple.nix

+          optfs = builtins.attrNames opt;
+          vfs   = builtins.attrNames v;
+      in builtins.isAttrs v &&
+      (if opt == {}


I'd use intersectAttrs (especially because it's a primop) here instead of converting to list and use subtractLists.

To check for then reqfs == vfs in the line below, I’d have to use builtins.intersectAttrs v req == req, which has to do a deep equality check for req (or v if I turn the arguments around).

aszlig · 2018-03-18T07:11:19Z

lib/types-simple.nix

+  # or even recursiveUpdate if you want to be naughty.
+  # Hint: It’s probably better to use `productOpt` instead,
+  #       but more power to the people.
+  defaults = t:


Hm, I don't think this has any more benefit to having to specify the defaults directly, especially if it comes to code readability. You need to look up the default here instead of just following the code you're already looking at.

I implemented that as a crutch for optional fields originally, but productOpt kind of replaced it pretty quickly. It might make sense to remove it again, yes.

Profpatsch · 2018-03-18T11:11:07Z

Here’s a (non-exhaustive) correspondence table.

module types	types-simple
`string`	n/a
`str`	`string`
`attrs`	`attrs any`
`list`	`list any`
`attrsOf`	`attrs`
`listOf`	`list`
`either l r`	`union [ l r ]`
`loaOf t`	`union [ (list t) (attrs t) ]`
`nullOr`	n/a
`submodule`	~ `product`/`productOpt`
`enum`	can be trivially built with `restrict`
`ints.between`	can be trivially built with `restrict`
n/a	`sum`
`addCheck` (no description update)	`restrict`

Some of the module types come from the fact that they also contain merging logic and submodule logic, which complicates the base design and lead to the deprecation of some types.

A simple type system to check plain nix values. Please see the module comments for further explanations. Will be used to correct the buggy meta-field checks and help in sanity-checking other parts of nixpkgs.

productOpt: a product type with the possibility for optional fields defaults: generate (semi-arbitrary) default values for a given type In preparation for the rewrite of `check-meta.nix`.

The previous code used some ad-hoc attribute enumeration to check for missing fields and did not really type check the attributes, because it used `t.check` from the module system types, which only does shallow checks for nested types, e.g. just `builtins.isList` for (listOf strings). This new version uses `types-simple` to recursively check every attribute, outputting accurate information for type errors in nested fields, e.g.: Package ‘hello-2.10’ in … has an invalid meta attrset: maintainers.1.email should be: string but is: null When we set meta.maintainers [ eelco foobar ]; in `hello/default.nix` and foobar is defined as { name = "foobar"; email = null; }; in `maintainers-list.nix`. The path accurately pins down where the type check failed: In the `maintainers` field, the second element of the list (counting from 0) and the field email in that element. Further work: If an unsupported meta field is used, the type error will print the complete type of the meta product, which is quite big. The type error should show a diff of the problematic fields in that case. This is possible but not yet integrated into the type checking logic.

The meta type is changed to either a string, a license or a list of licenses, where license is an attrset with required `fullName`, `shortName` and `free` fields, and two optional fields. Every package that does not conform to this union type is fixed; I noticed that only a few packages used `list of strings`, so I corrected them and removed the alternative.

Especially document the inner workings of the `checkType` function.

@aszlig

`defaults` originally only was a crutch to make products accept optional attributes shallowly, but that has been replaced by `productOpt`, so we don’t need it anymore. Also a few other changes based on input by @aszlig.

Profpatsch · 2018-03-19T06:16:28Z

Okay, rebase onto current master after the changes in check-meta.nix.

@Ericson2314 I noticed you create a lot of types in lib/system/parse.nix. I think changing the checks to lib.types-simple from this pull request would improve the type errors as well as how concise these types work (after adding an enum type). For now I’ve stubbed the meta.platforms fields out with any.

Profpatsch · 2018-03-19T06:20:47Z

pkgs/stdenv/generic/check-meta.nix

+      # TODO: use the types from lib/systems/parse.nix
+      # any should be lib.systems.parsed.types.system
+      system = any;
+      platform = union [ string any ];


@Ericson2314 This is where I stub out the platform types at the moment.

`restrict` is able to restrict the inhabitants of a type, meaning only a subset of a type is allowed, the new type is “smaller”, but still gives nice type errors if a nested value has the wrong type. See the tests for some usage examples.

Profpatsch · 2018-03-19T12:57:24Z

Update: enum and other restricted types are now possible using restrict.

edolstra · 2018-03-19T15:08:04Z

lib/default.nix

@@ -31,6 +31,9 @@ let
    options = callLibs ./options.nix;
    types = callLibs ./types.nix;

+    # type checking plain nix values
+    types-simple = callLibs ./types-simple.nix;


Please use camelCase for attribute names.

edolstra · 2018-03-19T15:08:52Z

What is the memory / evaluation time impact of this?

edolstra · 2018-03-19T15:10:38Z

lib/tests/types-simple.nix

+    };
+
+
+  # -- Products --


What are "products"? Seems like a pretty strange terminology.

types-simple is essentially defining algebraic data types, like ML or Haskell have; "products" is the standard terminology for tuple- or record-like things in this setting.

oxij · 2018-04-18T01:54:31Z

A couple of clarifications. In

I think `types-simple` should replace `types`

I mean that either `types-simple` should expose the merging NixOS modules would use, or `types` should `types-simple` for type checking. In

without duplicating already inferable information

and related I mean the following kind of duplication ```imaginary-typed-nix option :: builtintypes.str option = mkOption { merge = types.str.merge; }; ``` Similarly, in

a semantically very different nix

and related I also mean that it's unclear how you would typecheck ```nix { require = [ <module that defines `option`> ]; config = { option = value; }; } ``` without merging NixOS modules and nix (currently non-existent) modules into a single concept. Also, an idea I wanted to use when I planned implementing pretty much the same thing: I planned to make `bool` into a non-mergable bool and split other uses into `types.any` and `types.all` since `bool` has two different monoids. E.g. you would then use `all` for normal `enable` options (because if somebody disabled it, they probably know what they are doing, why make them use `mkForce`?) and `any` for security-related options (because to make things insecure everyone should agree).

Profpatsch · 2018-04-18T23:11:08Z

I planned to make bool into a non-mergable bool and split other uses into types.any and types.all since bool has two different monoids.

Ah, the Monoid remark is enlightening, yes. Merging is really just <> (the binary operator) and defaults are mempty, (the one-element). Awesome!

oxij · 2018-04-24T22:48:30Z

ping! Progress? Do you have an updated/rebased version of this to play with?

Profpatsch · 2018-04-25T14:06:35Z

@oxij I have just pushed a branch which does away with the in-file changes of check-meta.nix and gives you a type that can check derivation & meta fields: Profpatsch@7a2caa0

I intend to commit all improvements to that branch (types-simple-standalone) first.

`names` was originally intended to be able to match on the specific instance of a type, but it breaks the abstraction in a way. Since the function that used it was removed, we can remove the name attributes as well.

oxij · 2018-04-26T14:12:17Z

@Profpatsch awesome, thanks!

Nadrieril · 2018-10-11T14:27:14Z

Wow, this is great work ! I really like having the typeckecking logic separate from the merging logic. Is there a plan to make nixos types use this for typechecking under the hood ?

Shados · 2018-12-13T00:43:21Z

@Profpatsch this is perhaps somewhat off-topic, but I read through types-simple.nix, and I found it rather tricky to understand the actual type checking code. Primarily, pulling the recursion out of checkType and into cata and fmap made the control flow egregiously difficult for me to follow, and fmap effectively changing the semantics of the nested types in-place was unexpected. In order to understand it, I ended up having to write a version of it wihout either of those.

From the context, I gather that the design is based on ideas from category theory, and would (I assume) be easily comprehensible to people familiar with it. As a result, I am curious: are there other benefits in this specific instance to keeping the implementation so close to the abstract design?

Those comments notwithstanding, I still found it easier to understand than the existing module type system, and having a self-contained type system in place would be very useful.

Profpatsch · 2018-12-13T17:01:08Z

From the context, I gather that the design is based on ideas from category theory

No, not really, it’s from the classic bananas & barbed wire paper. You can find an explanation here: http://reasonablypolymorphic.com/blog/recursion-schemes/
It helps greatly when dealing with recursive data structures, by splitting open their recursion and just looking at one layer at a time.
You are right that it superficially complicates the code in an untyped setting, but in practice it is a great fit for a type checker which is intended to be expanded by users. If I expose the alg definition in checkType (which I should totally do), users can chain new transformations to each layer of recursion, e.g. to change error messages according to their needs.

…g` of NixOS to it This types `nixpkgs.config` of NixOS with `types.opaque` type and applies the resulting evaluated value to the newly introduced `configs` argument of `pkgs`. The new `configs` argument is like the old `config` argument (still available for convenience) but a list. This design was chosen because we can't just make a submodule out of `pkgs/top-level/config.nix` in NixOS because - firstly, we would have to duplicate all the magic `pkgs/top-level/default.nix` does to `config` in NixOS, - secondly, in principle, we might not have all the arguments needed by `pkgs/top-level/config.nix` in NixOS because some of them can be computed internally by `pkgs` (this is not a problem now but it will be a problem in use-flags patchset that comes after), thus all of those computations will need to be duplicated too, - thirdly, doing it this way keeps NixOS and pkgs separated, so that, in principle, one could replace NixOS with an alternative thing without duplicating all of the above once again and/or one could use separate module and type systems between NixOS and pkgs (`types.opaque` is trivial to implement in any type system and pkgs can use "types-simple" of NixOS#37252 today, if we were to wish it so). Note that since this design removes the need to do any preprocessing of `nixpkgs.config` in NixOS all the ad-hoc merging code was removed. Yay!

mmahut · 2019-08-12T11:38:17Z

Are there any updates on this pull request, please?

Profpatsch · 2019-08-12T11:53:43Z

Are there any updates on this pull request, please?

I still think it’s valuable, but I don’t know of any way out of the “we don’t want two type systems in nixpkgs” issue.

stale · 2020-06-01T16:08:21Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

Profpatsch requested review from edolstra, FRidh and nbp as code owners March 17, 2018 15:20

Profpatsch requested a review from grahamc March 17, 2018 15:20

GrahamcOfBorg added the 6.topic: python label Mar 17, 2018

Profpatsch force-pushed the types-simple branch from f75fee9 to ffab4b4 Compare March 17, 2018 16:57

Profpatsch requested a review from aszlig March 17, 2018 16:57

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Mar 17, 2018

aszlig reviewed Mar 18, 2018

View reviewed changes

GrahamcOfBorg added the 2.status: merge conflict label Mar 19, 2018

Profpatsch added 6 commits March 19, 2018 05:41

lib: add types-simple

b75602d

A simple type system to check plain nix values. Please see the module comments for further explanations. Will be used to correct the buggy meta-field checks and help in sanity-checking other parts of nixpkgs.

lib/types-simple: add productOpt and defaults

0ec18b2

productOpt: a product type with the possibility for optional fields defaults: generate (semi-arbitrary) default values for a given type In preparation for the rewrite of `check-meta.nix`.

lib/types-simple: add lots more (inline) documentation

8729bda

Especially document the inner workings of the `checkType` function.

lib/types-simple: remove defaults & small improvement

eefcd09

`defaults` originally only was a crutch to make products accept optional attributes shallowly, but that has been replaced by `productOpt`, so we don’t need it anymore. Also a few other changes based on input by @aszlig.

Profpatsch force-pushed the types-simple branch from f0e032b to eefcd09 Compare March 19, 2018 06:12

Profpatsch requested a review from Ericson2314 March 19, 2018 06:13

GrahamcOfBorg removed the 2.status: merge conflict label Mar 19, 2018

Profpatsch commented Mar 19, 2018

View reviewed changes

lib/types-simple: add restrict

85f28ff

`restrict` is able to restrict the inhabitants of a type, meaning only a subset of a type is allowed, the new type is “smaller”, but still gives nice type errors if a nested value has the wrong type. See the tests for some usage examples.

Profpatsch force-pushed the types-simple branch from e6f1908 to 85f28ff Compare March 19, 2018 12:51

edolstra reviewed Mar 19, 2018

View reviewed changes

Ericson2314 mentioned this pull request Apr 24, 2018

nixpkgs module: Clean up platform options #38485

Merged

8 tasks

Profpatsch added 4 commits April 25, 2018 16:26

lib/types-simple: add path scalar type

99ce0d4

lib/types-simple: test whether restrict works transitively

c872eec

lib/types-simple: improve indentation a bit

58a32c8

lib/types-simple: remove names attribute

b7e4e0a

`names` was originally intended to be able to match on the specific instance of a type, but it breaks the abstraction in a way. Since the function that used it was removed, we can remove the name attributes as well.

oxij mentioned this pull request May 1, 2018

Remove uses of libOnly #39683

Closed

Profpatsch mentioned this pull request Jun 24, 2018

github PRs: utilize nixpkgs-lint #11265

Closed

Ekleog mentioned this pull request Nov 5, 2018

Extensible attrsets NixOS/nix#2511

Closed

oxij mentioned this pull request Mar 9, 2019

Typed nixpkgs.config married to NixOS #57123

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020

Profpatsch closed this Jun 9, 2020


		## -- TYPES --

		# the type with no inhabitants (kind of useless …)

Rewrite meta-check.nix type checking logic. #37252

Rewrite meta-check.nix type checking logic. #37252

Conversation

Profpatsch commented Mar 17, 2018 • edited

Profpatsch commented Mar 17, 2018

aszlig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Profpatsch Mar 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Profpatsch commented Mar 18, 2018 • edited

Profpatsch commented Mar 19, 2018

Choose a reason for hiding this comment

Profpatsch commented Mar 19, 2018

Choose a reason for hiding this comment

edolstra commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxij commented Apr 18, 2018 via email

Profpatsch commented Apr 18, 2018 • edited

oxij commented Apr 24, 2018

Profpatsch commented Apr 25, 2018 • edited

oxij commented Apr 26, 2018

Nadrieril commented Oct 11, 2018

Shados commented Dec 13, 2018

Profpatsch commented Dec 13, 2018

mmahut commented Aug 12, 2019

Profpatsch commented Aug 12, 2019

stale bot commented Jun 1, 2020

Profpatsch commented Mar 17, 2018 •

edited

Profpatsch Mar 18, 2018 •

edited

Profpatsch commented Mar 18, 2018 •

edited

Profpatsch commented Apr 18, 2018 •

edited

Profpatsch commented Apr 25, 2018 •

edited