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

Added `normalizeWith` function. #79

Merged
merged 5 commits into from Jul 5, 2017

Conversation

Projects
None yet
2 participants
@aleator
Copy link
Contributor

commented Jun 28, 2017

Added normalizeWith function to complement typeWith.
Dhall is so very attractive base for simple DSLs that I felt it
needed to be pushed over the edge.

Naturally, using normalizeWith loses all the nice features of Dhall.
If your context isn't total or strongly normalizing then embedding it
to Dhall will not improve things.

I didn't spend too much time testing the change since I have some doubts whether this is an acceptable regarding the original mission of Dhall.

Added `normalizeWith` function.
Added `normalizeWith` function to complement `typeWith`.
Dhall is so very attractive base for simple DSLs that I felt it
needed to be pushed over the edge.

Naturally, using `normalizeWith` loses all the nice features of Dhall.
If your context isn't total or strongly normalizing then embedding it
to Dhall will not improve things.
@Gabriel439

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2017

I think this is a great idea, but I would suggest a different approach, which is to change the type of normalizeWith to CtxFun a -> Expr s a -> Expr t a, and you apply the CtxFun at the top-level instead of in the App case, like this:

normalizeWith :: CtxFun a -> Expr s a -> Expr t a
normalizeWith (CtxFun f) e = case f e of
    ...

normalize = normalizeWith (CtxFun id)

The reason why is that you might want to implement functions that cannot be normalized until they are fully saturated. An example of such a function in Dhall is List/head: the first argument is the type to specialize to, and that's not enough to begin the normalization process

The other reason I suggest this change is that I expect this to be more efficient. Doing a Map lookup on every App will decrease the performance of the standard normalize function, whereas applying id at the top-level is cheap and the compiler will just inline that away.

@aleator

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

I've experimented a little bit with applying CtxFun on the top level. If I understood correctly, there is a slight difficulty (or a decision point) here. On the top level, the argument part of the app is still wrapped in bunch of Notes, making it more tricky to actually write the CtxFuns.

Any opinions how this should be handled? Just (shift 0 .) the CtxFuns before use?

@Gabriel439

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2017

Yeah, that would work. It might also be simpler and more efficient to shift 0 the entire expression just once before beginning recursion, like this:

normalizeWith (CtxFun f) e0 = loop (shift 0 e0)
  where
    loop e = case f e of
        ...
@Gabriel439
Copy link
Collaborator

left a comment

Looks great! Just a few minor suggestions

-- that the @a@ and @s@ type parameters are never used
e'' = bimap (\_ -> ()) (\_ -> ()) e

text = "(loop) (" <> Data.Text.pack (show e'') <> ")"

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Jul 1, 2017

Collaborator

Perhaps expand loop here to normalizeWith.loop

This comment has been minimized.

Copy link
@aleator

aleator Jul 1, 2017

Author Contributor

Ah. Of course.

text = "normalize (" <> Data.Text.pack (show e'') <> ")"
-- | Use this to wrap you embedded functions (see `normalizeWith`) to make them
-- polymorphic enough to be used.
type CtxFun a = forall s. Expr s a -> Expr s a

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Jul 1, 2017

Collaborator

Perhaps inline the CtxFun type synonym into the type of normalizeWith since it's only used once

This comment has been minimized.

Copy link
@aleator

aleator Jul 1, 2017

Author Contributor

I left this alone, since it saved some typing when writing tests.

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Jul 1, 2017

Collaborator

In that case, maybe rename it to something like Normalizer since it no longer is part of a context any longer

normalize :: Expr s a -> Expr t a
normalize e = case e of
normalize :: Expr s a -> Expr t a
normalize = normalizeWith (id)

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Jul 1, 2017

Collaborator

Parentheses are not necessary around the id

This comment has been minimized.

Copy link
@aleator

aleator Jul 1, 2017

Author Contributor

Fixed. I also applied this to other places where search/replace has ended up with extra parentheses.

@aleator

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2017

Here is a new version of this. However, I hit a snag when trying to write tests. Should there be isNormalized that corresponds with normalizeWith, since it now also may depend on the context of normalization?

@Gabriel439

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2017

Yeah, you would need an isNormalizedWith function

Fixed an issue with normalizeWith
Turns out that the previous `normalizeWith` didn't actually normalize.
For example, if `min` is a custom function, then the expression
`min (min +1 +2) +3` would have normalized as `min +1 +3`.

This is now fixed by

a) Allowing `CtxFun` to return `Maybe (Expr s t)` so that normalization
   can decide whether to keep normalizing or not.
b) Applying the Ctx fun only to `App`. Luckily, this is also the
   sensible thing to do in general. Having a context which freely edits
   the ast *anywhere* would be scary.
@aleator

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2017

I've now moved the application of CtxFun back to App-case from the top level. The previous attempt at this didn't work out, since the normalization was cut short. Also, having all of the ast available to CtxFun seems like an overkill ("Hey, lets go and replace all True's with 42's"). When it is applied as the last case of App you know that it won't be likely to mess up pre-existing stuff that is outside of the captured App.

There are also two tests, which demonstrate how this works.

@aleator aleator referenced this pull request Jul 4, 2017

Merged

Raw input #85

@Gabriel439

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2017

This seems fine. My only remaining suggestion is to change the type of the function back to forall a . Expr s a -> Expr s a, and change the logic to:

_ ->  ctx (App f' a')

... and then normalizeWith becomes:

normalize = normalizeWith id

... for the same reason as before: efficiency


doubleReduction :: TestTree
doubleReduction = testCase "doubleReduction" $ do
let tyCtx = insert "min" (Pi "_" Natural (Pi "_" Natural Natural))

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Jul 4, 2017

Collaborator

Minor suggestion: you can use code to input these types:

minType        <- code "Natural → Natural → Natural"
fiveorlessType <- code "Natural → Natural"
wurbleType     <- code "Natural → Integer
let tyCtx = insert "min"        minType
          . insert "fiveorless" fiveorlessType
          . insert "wurble"     wurbleType
          $ empty

I don't feel very strongly about this suggestion, so feel free to ignore me :)

This comment has been minimized.

Copy link
@aleator

aleator Jul 5, 2017

Author Contributor

Looks better that way. Will do.

text = "normalize (" <> Data.Text.pack (show e'') <> ")"
-- | Use this to wrap you embedded functions (see `normalizeWith`) to make them
-- polymorphic enough to be used.
type CtxFun a = forall s. Expr s a -> Maybe (Expr s a)

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Jul 4, 2017

Collaborator

I still suggest naming this something like Normalizer instead of CtxFun since it no longer uses a context

This comment has been minimized.

Copy link
@aleator

aleator Jul 5, 2017

Author Contributor

Sorry. I missed your original comment. Will do!

@aleator

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

I don't think _ -> ctx (App f' a') would work. Suppose that we have a custom App (matching, say, App (Var "foo") (NaturalLit x)) sitting at the top level, and we hit this case. Then ctx turns this into (App (Var "bar") (NaturalLit x) and the normalizer terminates.

What if the ctx contains a definition for (App (Var "bar") (NaturalLit x) as well?

We also can't loop here without looping forever. We need to know if ctx actually did something before continuing.

The other alternative is to require that all custom primitives produce their results in normal form but this is somewhat more limited approach.

I can probably whip up a criterion for testing how much this slows things down if you are really worried. Can you think of a good test case? (Normalize the prelude? The tutorial?).

Gabriel439 added a commit that referenced this pull request Jul 5, 2017

Raw input (#85)
Added a function that allows doing `input` from a closed
Dhall `Expr` instead of text. This is hugely useful when
working with Dhall in AST level (for example, using custom
types / normalization). For example, suppose that you have
built a custom primitive that requires a record as an argument.
With `rawInput` you can just extract the record into a Haskell
data type and process it without need to work with maps and other
bits of AST.

For context of this commit, see #79 (and also #26).
@Gabriel439

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2017

Oh yeah, I hadn't though about the need to continue looping. Then I think it's fine as is

I think the performance impact will probably be minimal since it only affects the case where normalization is blocked, which is not that common and not on the critical path

@Gabriel439 Gabriel439 merged commit 28179ce into dhall-lang:master Jul 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.