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

Reject record projection when there is a field type mismatch #1027

Merged
merged 7 commits into from
Jun 28, 2019

Conversation

Gabriella439
Copy link
Collaborator

Fixes #1020

This also includes small fixes to the MissingField error message,
which I found along the way.

Fixes #1020

This also includes small fixes to the `MissingField` error message,
which I found along the way.
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff!

Should this wait for #1023 and possibly even dhall-lang/dhall-lang#601 though!?

dhall/src/Dhall/TypeCheck.hs Show resolved Hide resolved
dhall/src/Dhall/TypeCheck.hs Show resolved Hide resolved
dhall/src/Dhall/TypeCheck.hs Outdated Show resolved Hide resolved
dhall/src/Dhall/TypeCheck.hs Outdated Show resolved Hide resolved
short = "Projection type mismatch"

long =
"Explanation: You can project a subset of fields from a record by specifying the \n\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great explanation! 👍

I was dreading to write one myself!

@sjakobi
Copy link
Collaborator

sjakobi commented Jun 27, 2019

Ah, I thought we'd need to pass put nf_t instead of t to ProjectionTypeMismatch to ensure that both diff arguments are Records. I hadn't seen that prettyDiff normalizes the expressions.

I'm wondering whether the code would actually be easier to understand if we didn't have the prettyDiff alias and used Dhall.Diff.diffNormalized directly?!

Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :)

@Gabriella439
Copy link
Collaborator Author

@sjakobi: prettyDiff used to be a more complex function and the alias is just scar tissue that we forgot to remove:

prettyDiff :: (Eq a, Pretty a) => Expr s a -> Expr s a -> Builder
prettyDiff exprL exprR = builder
where
doc =
fmap Dhall.Pretty.annToAnsiStyle (Dhall.Diff.diffNormalized exprL exprR)
opts = Pretty.LayoutOptions { Pretty.layoutPageWidth = Pretty.Unbounded }
stream = Pretty.layoutPretty opts doc
lazyText = Pretty.renderLazy stream
builder = Builder.fromLazyText lazyText

Gabriella439 added a commit that referenced this pull request Jun 28, 2019
... as suggested by @sjakobi in #1027 (comment)

After some internal refactors, `prettyDiff` is now just an
alias for `Dhall.Diff.diffNormalized`, so it's no longer
necessary.
@Gabriella439 Gabriella439 merged commit 250fdfc into master Jun 28, 2019
@Gabriella439 Gabriella439 deleted the gabriel/1020 branch June 28, 2019 04:55
Gabriella439 added a commit that referenced this pull request Jun 28, 2019
... as suggested by @sjakobi in #1027 (comment)

After some internal refactors, `prettyDiff` is now just an
alias for `Dhall.Diff.diffNormalized`, so it's no longer
necessary.
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 this pull request may close these issues.

Illegal projection is caught only on second try
2 participants