-
Notifications
You must be signed in to change notification settings - Fork 9
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
Implement Dhall Haskell's semi-semantic import caching #49
Conversation
It transpires that dhall haskell's behaviour is a _lot_ more complex than what is mandated by the spec. We believe that following its lead should give us much better performance. In particular, we should avoid having to normalize or typecheck resolved imports in lots of cases.
Including semi-semantic cache
Nice, thanks! Taking a look now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I have a few comments but all things we can handle in a follow-up.
implicit Client: Client[F], | ||
F: Sync[F] | ||
) extends LiftVisitor[F](F) { | ||
private var duplicateImportsCache: MMap[ImportContext, String] = MMap.empty | ||
private var duplicateImportsCache: MMap[ImportContext, Expr] = MMap.empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: we could change this back to a constructor parameter now. I'll do that when I rebase #45 after merging this.
case Env(value) => | ||
for { | ||
vO <- F.delay(sys.env.get(value)) | ||
v <- vO.fold( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but I think F.fromOption(vO, new ResolutionFailure(s"Missing import - env import $value undefined"))
is a little nicer here and elsewhere. We can do that in a follow-up, though—I know this isn't new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice! I've never noticed that method before. Yes, that should definitely be in a bunch of places
) | ||
val expected = parse("let x = 1 in x").normalize | ||
val expected = parse("let x = 1 in x").alphaNormalize.normalize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's up here? It looks a little odd to alpha-normalize before beta-normalizing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it? In my head alpha-normalization belongs at the same stage as computing De Bruijn indices (although obviously not necessary in the same way as Dhall variables are unambiguous)
Significant improvements in import resolution performance (we do a lot less type checking now)
Allows us to enable the type-checking prelude test