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

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

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Oct 24, 2024

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 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
@hsjobeki hsjobeki force-pushed the types/defaultTypeMerge branch from 7de011e to 81ca8b5 Compare November 8, 2024 12:27
@infinisil
Copy link
Member

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

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

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

@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

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
35 checks passed
@nixos-discourse
Copy link

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants