-
Notifications
You must be signed in to change notification settings - Fork 2
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
Multiple inheritance of GATs #147
Conversation
This looks awesome. Long have I waited for such features :) |
@olynch I've just committed the work (unfinished) I started to work on renaming. You're right it's complex; it took me some time to develop a better understanding of the design after our discussion, so the work here is somewhat minimal. As part to learning the design, I started a separate |
src/syntax/TheoryInterface.jl
Outdated
@@ -4,6 +4,7 @@ export @theory, @signature, Model, invoke_term | |||
using ..Scopes, ..GATs, ..ExprInterop | |||
|
|||
using MLStyle | |||
using Folds |
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.
I missed this. this should be removed
As discussed in private conversation, we want to strip out all of the renaming support here, because we have decided that renaming is tricky to get right. So this should hopefully end up being a pretty simple PR. |
…y definition, but a test in models is now broken
…what cleaned up after my debugging.
…being created but it needs clean up and review.
…gSort} type issue when reidenting MethodResolver
The following is a summary of the work done so far on multiple-inheritance. While a @rename macro solution was proposed to close out the branch, I'm requesting help getting the using-as feature of multiple-inheritance over the finish line. In March, @olynch and I discussed stripping out the renaming in favor of a @rename macro. That is implemented
but after a ~three-month break, I once again took another stab at using default renaming with some success in defining ThModule through using statements. After some tweaking from catching the repo up with main, the following is a working theory in GATlab and has examples in
I'm happy with the progress made to implement multiple-inheritance, but naturally over the course of learning the GATlab architecture, I likely made several suboptimal kludgey design choices, whereas I'd be more satisfied with an elegant solution. However because I'm somewhat invested in the design, and have been working rather too close to it, I want to take this opportunity to step back and allow others to comment on it. Here, I'll explain the procedure for implementing
This works in the following order: 1.1) 1.2) 1.3) 1.3.1) If there is a rename dictionary, we generate a new dictionary of instructions for the 1.3.1.1) Reident gives the "host" and "guest" theories a "nametag," a dictionary associating their names with their tags. As the dead code in (PROBLEM: I was unsure how to populate the LID, and used a placeholder 1.3.1.2) A new dictionary is created associating each old ident to a new one. 1.3.2) the
as there is no ISSUES (non-exhaustive)
causes the module axioms over Form1 (and Form2) to be omitted, as the segment itself has the same tag. For the sake of demonstrating the problem further, I've defined three theories:
We can see that the type declarations are different, but the segments defining \oplus have the same tag:
My impression is that this problem could be fixed in the current design if reident could reidentify a segment if it contains bindings which contain a term that will be renamed, e.g.,
contains a
Note: Not every theory has been rewritten for using-as statements. I decided not to do this in favor of just fixing the underlying problem. EDIT After some thought, I think for the time being a more sophisticated dictionary for renaming would be the quickest fix. For example, we can reident
would probably require more declarations of form-form interaction, e.g.
This particular example treads out of scope into graded theories, however. |
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.
These are just some comments, not a full review
# sigmoid(x) ⊣ [x] | ||
# end | ||
# TODO test theory equality | ||
@theory ThRig begin |
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.
Why not: const ThRig = ThSemiRing
?
MethodResolver(reident(reps, key, m.bysignature)) | ||
end | ||
|
||
function reident(reps::Dict{Ident}, key::Ident, bysignature::Dict{AlgSorts, Ident}) |
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.
This seems like just a helper function for reident(_, _, MethodResolver)
, it shouldn't "claim" the reident
overload for reident(_, _, Dict{AlgSorts, Ident})
I think...
MethodResolver(rename(tag, renames, m.bysignature)) | ||
end | ||
|
||
function rename(tag::ScopeTag, renames::Dict{Symbol,Symbol}, bysignature::Dict{AlgSorts, Ident}) |
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.
See comment for reident below
There are actually two uses for renaming: renaming operations and renaming types. Of the two, I think that renaming operations is "more destructive". For instance, we would use renaming operations to make However, for renaming types, we might want to take a slightly more gentle approach, inspired by trait systems in Rust/Haskell/Scala. Specifically, we should have parameterized theories like @theory ThGroup{T} begin
using ThMonoid{T}
inv(a::T)::T
# axioms...
end Then the DEC can be written as @theory ThDEC begin
Scalar::TYPE
Form0::TYPE
Form1::TYPE
Form2::TYPE
using ThRing{Scalar}
using ThAlgebra{Scalar, Form0}
using ThModule{Form0, Form1}
using ThModule{Form0, Form2}
end The key thing is that these The one thing that we'd have to worry about is the methods that check for validity. I think that when we make a parameterized module, we simply don't create the methods for the types that are in the parameters. Then |
src/syntax/TheoryInterface.jl
Outdated
This allows us to safely check if a theory has a name (with `hastag`) and also retrieve the associated tag. | ||
|
||
""" | ||
function nametag(T::GAT) |
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.
This can be accomplished via the namelookup
field in ScopeList
, which is the actual type of T.segments
.
src/syntax/gats/gat.jl
Outdated
@@ -248,6 +364,23 @@ Scopes.AppendContext(c::GATContext, context::Context{AlgType}) = | |||
|
|||
function methodlookup(c::GATContext, x::Ident, sig::AlgSorts) | |||
theory = c.theory | |||
if theory.name == :ThTuple |
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 going on here....
src/syntax/gats/exprinterop.jl
Outdated
@@ -267,7 +269,8 @@ function parseaxiom!(theory::GAT, localcontext, sort_expr, e; name=nothing) | |||
c = GATContext(theory, localcontext) | |||
equands = fromexpr.(Ref(c), [lhs_expr, rhs_expr], Ref(AlgTerm)) | |||
sorts = sortcheck.(Ref(c), equands) | |||
@assert allequal(sorts) | |||
# XXX I am removing the assertion to test something |
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 should take this out
test/models/SymbolicModels.jl
Outdated
@theory ThCategory begin # should this be deleted? | ||
Ob::TYPE | ||
Hom(dom::Ob, codom::Ob)::TYPE | ||
# @theory ThCategory begin # should this be deleted? |
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.
Yes, we should delete this.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
- Coverage 94.52% 93.85% -0.68%
==========================================
Files 38 38
Lines 2082 2229 +147
==========================================
+ Hits 1968 2092 +124
- Misses 114 137 +23 ☔ View full report in Codecov by Sentry. |
No description provided.