-
Notifications
You must be signed in to change notification settings - Fork 459
Generalize analyses over the term type #209
Conversation
robrix
left a comment
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.
Ready for review.
| import Control.Effect.Reader hiding (Local) | ||
| import Control.Effect.State | ||
| import Control.Monad ((<=<), guard) | ||
| import qualified Data.Core as Core |
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.
We no longer rely directly on imports of Data.Core or any other syntax. We do still rely on Analysis.Eval for the Analysis record, but it could easily be moved elsewhere (and will eventually be factored into effects anyway).
This means that we could in principle split the analyses off into a new package, which is a good sign that we’ve got the right separation of concerns going.
| concrete | ||
| :: (Foldable term, Show (term User)) | ||
| => (forall sig m | ||
| . (Carrier sig m, Member (Reader Loc) sig, MonadFail m) |
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.
Abstracting this evaluator over any m satisfying these effects (these being the effects required by eval) enables us to avoid coupling the evaluator(s) to implementation details of the analyses, too.
This list of constraints will eventually grow as we factor bits of Analysis out into effects.
| . fix (eval concreteAnalysis) | ||
|
|
||
| concreteAnalysis :: ( Carrier sig m | ||
| , Foldable term |
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.
We need Foldable to collect the free variables in the term to filter the env that closures close over.
| , Semigroup value | ||
| ) | ||
| => Analysis address value m | ||
| => Analysis (Term (Ann :+: Core) User) address value m |
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.
eval is still specialized to Term (Ann :+: Core) User; intuitively, it is the canonical abstract interpreter for Core, and thus has to be specialized to it.
|
|
||
| type Cache name a = Map.Map (Term (Core.Ann :+: Core.Core) name) (Set.Set a) | ||
| type Heap name a = Map.Map name (Set.Set a) | ||
| newtype Cache term a = Cache { unCache :: Map.Map term (Set.Set a) } |
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.
Without the newtype, abstracting the cache over the term type made it impossible to distinguish from Heap.
| ) | ||
| => (Term (Core.Ann :+: Core.Core) name -> NonDetC (ReaderC (Cache name a) (StateC (Cache name a) m)) a) | ||
| -> Term (Core.Ann :+: Core.Core) name | ||
| => proxy address |
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.
We didn’t mention the address type anywhere else, so we needed a dummy parameter to keep it unambiguous. (We could also have used AllowAmbiguousTypes & TypeApplications, but I try to avoid the former since it makes calling the thing significantly more challenging.)
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.
+1 on avoiding AllowAmbiguousTypes, since Proxy has nice behavior with TypeApplications anyway.
| => Analysis term User (Value term) m | ||
| -> (term -> m (Value term)) | ||
| -> (term -> m (Value term)) | ||
| ) |
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.
Most of the extra lines of code introduced by this PR are reformatted signatures + the evaluator parameters.
patrickt
left a comment
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.
Having a richer vocabulary to describe the capabilities of an analysis is such great stuff.
| ) | ||
| => (Term (Core.Ann :+: Core.Core) name -> NonDetC (ReaderC (Cache name a) (StateC (Cache name a) m)) a) | ||
| -> Term (Core.Ann :+: Core.Core) name | ||
| => proxy address |
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.
+1 on avoiding AllowAmbiguousTypes, since Proxy has nice behavior with TypeApplications anyway.
Our analyses are intended to be completely decoupled from the source language. This PR enables that by abstracting
Analysisand various related signatures over the term type.