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

Allow deep combining without failure #114

Closed
AlexeyRaga opened this issue Mar 10, 2018 · 13 comments · Fixed by #923
Closed

Allow deep combining without failure #114

AlexeyRaga opened this issue Mar 10, 2018 · 13 comments · Fixed by #923

Comments

@AlexeyRaga
Copy link

AlexeyRaga commented Mar 10, 2018

Currently, we have two options to combine records:

The (//) operator combines the fields of both records, preferring fields from the right record if they share fields in common

and

The (/) operator also lets you combine records, but behaves differently if the records share fields in common. The operator combines shared fields recursively if they are both records but fails with a type error if either shared field is not a record.

Would it be possible to add a 3rd option to "combine fields recursively if they are both records, preferring non-record fields from the right record"?

In my use case I want to have some "default" value (deeply nested type) and combine it with a user-provided record so that user doesn't have to specify all the fields but the combined record would form a valid type.

@Gabriella439
Copy link
Contributor

Gabriella439 commented Mar 10, 2018

Originally I intended to extend the operator to behave the way you ask (i.e. if there is a conflict between two fields that are not both records then prefer the right field).

The problem is when you have expressions like this:

λ(x : Text)  λ(y : {})  { foo = x }  { bar = y }

What would you expect that to normalize to?

@Gabriella439
Copy link
Contributor

Oops, I meant to say:

λ(x : Text)  λ(y : {})  { foo = x }  { foo = y }

@AlexeyRaga
Copy link
Author

AlexeyRaga commented Mar 11, 2018

In this case I would expect { foo = {} } as a result.
My reasoning would be intuitive and "imperative":

  1. foo is set to x.
  2. combine happens and we check conflicting fields: "if both fields are records then combine them, otherwise RHS wins". Since foo is set to x: Text now, combine prefers RHS value.

@Gabriella439
Copy link
Contributor

The issue with that is that it would require interleaving type-checking with normalization which would significantly complicate the semantics. We would need to know that x has type Text and y has type {} in order to know that they conflict and y takes precedence. Also, in general I try to avoid users having to reason at the type level in order to infer what is happening at the term level.

Another issue with a recursive override operator is that it is no longer associative. For example:

x = { foo = { bar = 1 } }

y = { foo = "ABC" }

z = { foo = { baz = 2 } }


(x ∧ y) ∧ z = { foo = { baz = 2 } }

x ∧ (y ∧ z) = { foo = { baz = 1, baz = 2 } }

@AlexeyRaga
Copy link
Author

Hm, OK, I thought it would be just a check whether both fields are records are not, which would need to be done anyway to determine whether to go deeper.

I am fine with non-associativity here. But I understand why it is a concern and this is why I am not asking to change the existing behaviour but rather to introduce another variation of combine.

It would allow pretty common use case for dealing with "big" APIs and configuration, like dealing Singularity where asking users to specify every single property of these types is impractical.

@Gabriella439
Copy link
Contributor

The reason you require type inference is because some fields might be bound variables, so you can't tell if the bound variable is a record or not without knowing its type

I also think the lack of associativity is an important reason why I wouldn't want to introduce this operator (even as a new operator). One of the design principles of Dhall is that all operators should be associative

Also, note that you can still update a nested record without specifying every field. In the worst case you can use the operator once per nesting level, like this:

    let example = { foo = { bar = { baz = 1 } } }

in  example ⫽ { foo = example.foo ⫽ { bar = example.foo.bar ⫽ { baz = 2 } } }

It's not ideal, but it at least doesn't require specifying the other fields at each nesting level

@AlexeyRaga
Copy link
Author

Yeah, I think I'll have to go this way then, will see how it goes.
Thanks for the explanation.

@Gabriella439
Copy link
Contributor

You're welcome!

@Gabriella439
Copy link
Contributor

I will go ahead and close this, but feel free to reopen if there is more to do here.

@domenkozar
Copy link

I'd just like to add that I always wanted the requested behavior in Nix and I'm sad to see dhall doesn't do it right, even though I understand it goes outside of desired design space.

A quite common configuration pattern is default values. One file has the defaults, the other overrides them for example like ./defaults.dhall /\ { ... }

Currently one can use // if the configuration is nested using Gabriel's trick, but can't use /\ as defaults can't be used.

Having a good story for defaults is important for introducing new fields to the configuration as the application using them gets new settings through it's lifetime.

@Gabriella439
Copy link
Contributor

@domenkozar: My main issue with the requested behavior is that it requires interleaving type-checking and normalization. Not only does this complicate the standard but it also complicates the user's mental model since they can't reason about how behaves without performing type inference in their head.

However, even if we were to accept that tradeoff, it still wouldn't completely solve the problem. For example, consider the case where you have a configuration of type:

{ foo : Optional Text, bar : List { baz : Optional Text } }

The wouldn't be able to recurse into the record inside the List of the bar field.

For a non-contrived example of this, see the menucolors.*.attributes field of the dhall-nethack configuration:

https://github.com/dhall-lang/dhall-nethack/blob/a99c2f7d5b4612d008393819602490047d328e99/examples/validated.dhall#L118-L119

@Gabriella439
Copy link
Contributor

Reopening this as we've been discussing adding a with keyword for deep overrides here: #754 (comment)

@Gabriella439 Gabriella439 reopened this Jan 19, 2020
Gabriella439 added a commit that referenced this issue Feb 25, 2020
Fixes #114

Fixes #340

Related to #754

This adds a new keyword which is syntactic sugar for using `//` to
perform a deep override of the record.  This comes in handy when
working with very large records (such as those found in
`dhall-kubernetes`).  This similar in spirit to Kustomize, albeit not
as powerful and customizable as Kustomize.

The goal is that the user can write the following code to update an
existing default configuration:

```dhall
someDeployment with {
, metadata.labels =
      someDeployment.metadata.labels
    # toMap { `scheduler.alpha.kubernetes.io/critical-pod` = "" }
}
```

You can also see this idiom outside of the Dhall/Kuberentes ecosystem.
For example, many Nix users manually apply deeply nested updates by
hand, like this example from the Nixpkgs manual:

```nix
  {
    haskell = super.haskell // {
      packages = super.haskell.packages // {
        ghc784 = super.haskell.packages.ghc784.override {
          overrides = self: super: {
            ghc-events = self.callPackage ./ghc-events-0.4.3.0.nix {};
          };
        };
      };
    };
  };
```

... where the Dhall equivalent would be:

```dhall
super with
  { haskell.packages.ghc784 = super.haskell.packages.ghc784.override {
      …
    }
  }
```

There is one additional change that I plan to make that builds on top of
this which is to would like to have the record completion syntax desugar
to use `with` instead of `//`, like this:

```dhall
T::r = (T.default with r) : T.Type
```

... which would allow for deep overrides for the record completion
operator, too.  However, I chose to defer that to a separate change to
keep this change as minimal as possible.
@Gabriella439
Copy link
Contributor

The fix for this is up here: #923

Gabriella439 added a commit that referenced this issue Mar 3, 2020
Fixes #114

Fixes #340

Fixes #754

This adds a new keyword which is syntactic sugar for using `//` to
perform a deep override of the record.  This comes in handy when
working with very large records (such as those found in
`dhall-kubernetes`).  This similar in spirit to Kustomize, albeit not
as powerful and customizable as Kustomize.

The goal is that the user can write the following code to update an
existing default configuration:

```dhall
someDeployment
  with metadata.labels =
      someDeployment.metadata.labels
    # toMap { `scheduler.alpha.kubernetes.io/critical-pod` = "" }
```

You can also see this idiom outside of the Dhall/Kuberentes ecosystem.
For example, many Nix users manually apply deeply nested updates by
hand, like this example from the Nixpkgs manual:

```nix
  {
    haskell = super.haskell // {
      packages = super.haskell.packages // {
        ghc784 = super.haskell.packages.ghc784.override {
          overrides = self: super: {
            ghc-events = self.callPackage ./ghc-events-0.4.3.0.nix {};
          };
        };
      };
    };
  };
```

... where the Dhall equivalent would be:

```dhall
super
  with haskell.packages.ghc784 = super.haskell.packages.ghc784.override { … }
```

There is one additional change that I plan to make that builds on top of
this which is to would like to have the record completion syntax desugar
to use `with` instead of `//`, like this:

```dhall
T::r = (T.default with r) : T.Type
```

... which would allow for deep overrides for the record completion
operator, too.  However, I chose to defer that to a separate change to
keep this change as minimal as possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants