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. |
| 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.
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.
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.
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.
What's up here? It looks a little odd to alpha-normalize before beta-normalizing.
There was a problem hiding this comment.
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