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 custom contexts that are ill-formed #259

Merged
merged 4 commits into from Feb 15, 2018

Conversation

Projects
None yet
2 participants
@Gabriel439
Copy link
Collaborator

commented Feb 13, 2018

Fixes #253

This protects typeWith to reject any custom-context that might trigger a
type-checking loop

Reject custom contexts that are ill-formed
Fixes #253

This protects `typeWith` to reject any custom-context that might trigger a
type-checking loop

@Gabriel439 Gabriel439 requested a review from ocharles Feb 13, 2018

{-| This function verifies that a custom context is well-formed so that
type-checking will not loop
-}
checkContext :: Context (Expr s X) -> Either (TypeError s X) ()

This comment has been minimized.

Copy link
@ocharles

ocharles Feb 13, 2018

Collaborator

I think you meant to export this. If not, please export it 😄

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Feb 13, 2018

Author Collaborator

Actually, I intentionally did not export it since typeWith calls it once before entering the type-checking loop. This avoids the performance issue since it's not called on every iteration of the loop

@ocharles

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2018

@Gabriel439

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2018

Oh, whoops! You're right that there is one unnecessary call to checkContext since it already uses typeWith which already uses checkContext. I'll fix that

I'll also go ahead and export it, too

Gabriel439 added some commits Feb 13, 2018

Expose `checkContext`
... as suggested by @ocharles
@ocharles

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2018

Oh, I actually meant https://hackage.haskell.org/package/dhall-1.9.1/docs/src/Dhall-Import.html#loadWithContext which calls typeWith, so we end up validating the context twice if you do import resolution and then final type checking. It's hardly the end of the world though.

@Gabriel439

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2018

Imports are type-checked twice anyway even without custom contexts. This is normal to ensure that imports are closed and also to localize error messages better.

@ocharles

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2018

Sure, it's just a pinch more overhead. But I think it's inconsequential

@Gabriel439

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2018

I'll go ahead and merge this, then, since I think I've addressed your requests

@Gabriel439 Gabriel439 merged commit fbac034 into master Feb 15, 2018

1 check passed

hydra Hydra build #58 of dhall-haskell:259:dhall
Details

@Gabriel439 Gabriel439 deleted the gabriel/253 branch Feb 15, 2018

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.