-
Notifications
You must be signed in to change notification settings - Fork 3
refactor to use Lattice as a typeclass and remove InternalLattice #557
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
base: main
Are you sure you want to change the base?
Conversation
|
I think this PR does make the code nicer, but my main worry is about the learning curve for new basillers and the discoverability of typeclass instances. What're your thoughts? Since typeclasses separate the typeclass methods away from the type they operate on, there is no link in the docs to go from, for example, Haskell solves this by adding an "Instances" list to each datatype (showing all its implemented typeclasses) and each typeclass (showing all types which implement that typeclass). This bidirectional cross-referencing would help a lot. I can imagine some ways to help with this problem:
|
|
I think its probably OK to teach new basilers about type-classes through documentation of a specific example e.g. a tutorial or documentation links in a representative lattice (or even just this tutorial https://web.archive.org/web/20250317112721/https://continuation.passing.style/blog/writing-abstract-interpreter-in-scala.html). The custom error messages probably help too? When I looked at this problem I decided it was more straightforward to explicitly pass around
I imagine you will end up doing this regardless of what I say :) |
|
@j-tobler I would be grateful for your review of this change request. In particular, does the motivation to you and do the changes make sense? I think you have worked with the Lattice trait before, but maybe not InternalLattice. Bother me immediately if there are any questions. todo:
|
Some initial progress towards cleaning up RunUtils. Created classes Transform, StaticAnalysis and AnalysisManager. Refactored doCleanup, prepareForTranslation, generateRelyGuaranteeConditions, and generateFunctionSummaries to use the Transform class.
ci: `mill -i` to disable daemon
Conflicts: src/main/scala/ir/IRLoading.scala src/main/scala/util/RunUtils.scala
cc @b-paul for LatticeCollections :)
This does a major refactor of the lattices to use Lattice[T] as /the/ representation for lattice operations. The Lattice[T] trait itself is largely unchanged, but it is instantiated in more places as an type class instance and it fully takes over the role of InternalLattice.
The InternalLattice abstraction had two major problems, stemming from the fact that it implements the lattice functions as instance methods within the lattice element class.
T <: InternalLattice[T]forces the trait to be implemented on the class type itself. This makes it impossible to have multiple lattices on the same type, and it makes it very difficult to do certain compositions of lattices to derive other lattices.In making this change, we gain notable benefits:
Lattice[T]value which precisely scopes its purpose. This is rather than an implicitTlattice element which happens to be a subtype ofInternalLattice[T])<:<proof was needed, but I guess it was because of certain subtypings of InternalLattice.Along the way, I have made some incidental changes:
???. i think it's better to be explicit here..joinon values of type T to access the lattice functions. That is,x.join(y)simply invokeslattice.lub(x, y). This matches the previous API of InternalLattice and makes it similarly convenient to use.All of the tests pass just the same, so I'm reasonably confident I haven't broken anything.
I am also very happy to take feedback on the design. Type classes are another Scala feature to learn, but I think it is much more familiar than implicit vals and leads to a more regular interface.
The relevant scala documentation pages are:
Note that we have to use the "previous" given syntax since we're on Scala 3.3 LTS. Also, note that these docs are actually good - this is because it's a new feature in Scala 3.