Skip to content

lib.types.defaultTypeMerge: refactor functor.{payload,wrapped} merging#350906

Merged
infinisil merged 1 commit intoNixOS:masterfrom
hsjobeki:types/defaultTypeMerge
Nov 24, 2024
Merged

lib.types.defaultTypeMerge: refactor functor.{payload,wrapped} merging#350906
infinisil merged 1 commit intoNixOS:masterfrom
hsjobeki:types/defaultTypeMerge

Conversation

@hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Oct 24, 2024 •
edited
Loading

Refactors defaultTypeMerge

  • all possible combinations are now obviously covered.
  • types with functors that declare both payload and wrapped at the same time are now an explicit error.
    This shouldn't have worked before and will now be an explicit error.
    If anyone relies on having both they should decide. While I recommend using payload.elemType together with binOp.
    See the existing types that use functor.payload for examples.

What also changed:

  • payload is merged first if it exists
  • wrapped is merged lastly if it exists

This change is related to #344216 because it is a type that needed both wrapped and payload.

In the long term .wrapped should be removed because all of its purpose can also be done via payload.elemType this removes one order of complexity from the merge logic matrix

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Oct 24, 2024
@hsjobeki hsjobeki mentioned this pull request Nov 7, 2024
Merged
13 tasks
@hsjobeki hsjobeki force-pushed the types/defaultTypeMerge branch 3 times, most recently from 7f14388 to dbc5ee1 Compare November 7, 2024 17:13
@hsjobeki hsjobeki marked this pull request as ready for review November 7, 2024 17:18
@hsjobeki hsjobeki requested a review from infinisil November 7, 2024 17:18
@nix-owners nix-owners bot requested a review from roberth November 7, 2024 17:19
@hsjobeki hsjobeki force-pushed the types/defaultTypeMerge branch 2 times, most recently from 82f7630 to 7de011e Compare November 7, 2024 17:22
@hsjobeki hsjobeki changed the title lib/types: defaultTypeMerge move wrapped into payload lib/types: defaultTypeMerge merge payload before wrapped Nov 7, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 7, 2024
@hsjobeki hsjobeki force-pushed the types/defaultTypeMerge branch from 7de011e to 81ca8b5 Compare November 8, 2024 12:27
@infinisil
Copy link
Member

infinisil commented Nov 8, 2024

The need to flip the order isn't obvious to me, can you explain that?

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Nov 9, 2024 •
edited
Loading

The need to flip the order isn't obvious to me, can you explain that?

I have two arguments mainly:

  1. In this example lib.types: attrsWith named placeholder #344216

    We have a type that has both wrapped and payload but the merge function returns the merged wrapped early without merging the payload. The other way around would be better.

  2. In the next couple of PRs want to refactor the merge behavior for composed types such that they use payload exclusively.
    i.e. payload.elemType instead of wrapped until we can remove this codepath completely. I have some ideas what we need to change for that. Giving wrapped less precedence is also an necessary preparation before one could do that.

@infinisil
Copy link
Member

infinisil commented Nov 18, 2024 •
edited
Loading

As a reminder, me and @hsjobeki just had a meeting where we looked at this and improved it to a point where I think it's ready to merge. Still needs to be pushed and the PR title/description updated though :)

@hsjobeki hsjobeki force-pushed the types/defaultTypeMerge branch from 81ca8b5 to b978799 Compare November 19, 2024 07:59
@hsjobeki hsjobeki changed the title lib/types: defaultTypeMerge merge payload before wrapped lib.types.defaultTypeMerge: refactor functor.{payload,wrapped} merging Nov 19, 2024
@hsjobeki
Copy link
Contributor Author

hsjobeki commented Nov 19, 2024

@infinisil Just pushed all the updates we made.
I am wondering: Since we now decided that wrapped + payload is an error. This means attrsWith and the resulting types attrsOf and layzAttrsOf must use payload exlucsively. This could potentially break downstream code that relies on functor.wrapped

So maybe we should add a warning instead of a hard failure?

@infinisil
Copy link
Member

infinisil commented Nov 24, 2024

This would be a potential problem with the attrsWith PR, it doesn't apply here, so we're good :)

(written next to @hsjobeki in Zürich #ZurichZHF :))

@infinisil infinisil merged commit b234fd8 into NixOS:master Nov 24, 2024
@nixos-discourse
Copy link

nixos-discourse commented Dec 11, 2024

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

https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@infinisil infinisil infinisil approved these changes

@roberth roberth Awaiting requested review from roberth

Assignees

No one assigned

Labels

6.topic: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hsjobeki @infinisil @nixos-discourse