-
Notifications
You must be signed in to change notification settings - Fork 3
Add lists #21
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?
Add lists #21
Conversation
|
Both problems listed above are resolved now. I've reorganised the Savina examples so that the original versions are in the main Savina folder, and the extended versions that use lists are in a subfolder (called, unsurprisingly, "lists"). The state of these so far is as follows:
|
This ensures that pattern variables are correctly substituted. It also simplifies each HK solution prior to substitution in order to avoid pattern blowup.
|
Apologies it's taken so long as I've been working on recursive types in parallel, but this is basically ready for review and I've merged with the latest changes from main. However, since commit 56d2076 I'm having some trouble with quasilinearity in a couple of the examples that previously typechecked (which does also affect the recursive types work). See Because so I think there must be something wrong with how quasilinearity is assigned to list (and other recursive) types. Unfortunately, I haven't been able to find it yet! |
|
Hi @starsandspirals, after thinking about it and trying things out, the issue comes because it's only really sound to have returnable elements of a list, because otherwise we can introduce unsafe aliasing. Your examples require lists to contain elements with usable types. Consider the following example: Here what we're doing is packaging x into a list, then deconstructing the list in the cons branch. However we now have both I fixed a small issue with disabling quasilinearity checking so please pull and recompile. Then running ./mbcheck with, and without -q will give two results: it's accepted without QL checking and (rightly) rejected with QL checking. OK, so where does that leave us? In essence what you're trying to do is totally safe because you're not using any unsafe aliasing. In fact I think this is probably solvable in the same way that we solve the same aliasing problem for inter-process communication... So if we restrict the 'cons' branch to contain only free variables of unrestricted type (or mailbox types with a different interface) we should be able to rule this out while allowing your examples. Let me have a play and get back to you. |
It should be possible for lists to be able to contain usable mailbox variables, but only if the 'cons' case is statically guaranteed not to contain any unsafe aliases. This is the same problem as with Receive, so we can adopt the same solution.
|
Hi @SimonJF - thanks very much for fixing the issue with big.pat and barber.pat, but unfortunately the change has broken another example that was working before (banking.pat). I've simplified the logic in that example a bit now to use a sum type instead of a list in one case where the list could only ever contain one mailbox, but this didn't fix the quasilinearity problem. However, when I run this example with the quasilinearity check disabled, I get a failed assertion in type_utils.ml ... so maybe there's something weirder going on with this one as well! |
|
Hi @starsandspirals -- sorry, GH didn't notify me of your comment! I'll have a look now. |
|
All examples are indeed working now (thanks for sorting all the issues we were having!) so marking as ready for review. |
SimonJF
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.
Thanks Danielle @starsandspirals for this great body of work and very welcome contribution! Minor changes requested really:
- Is there any way we can make the list type
List(a)rather than[a], and ideally have an infix::operator for cons? - In fixing the join/intersect cases for lists I've realised there are some missing for tuples / sums. Please can you add the missing cases where indicated?
- As we found out, as long as we are careful about deconstructing things, we can treat lists as returnable regardless of whether their contents are usable or returnable. The same logic applies to tuples and sums, so if you can please make the indicated changes?
- Can you please add the new lists examples and tests to the test suite JSON file?
Thanks again!
lib/common/type.ml
Outdated
| ql = Quasilinearity.Returnable | ||
| | Tuple ts -> List.for_all is_returnable ts | ||
| | Sum (t1, t2) -> is_returnable t1 && is_returnable t2 | ||
| | List t -> is_returnable t |
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.
Again based on my argument in make_returnable these should all be returnable by default.
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.
Fixed these - is make_usable fine as is?
lib/frontend/lexer.mll
Outdated
| "inr", INR | ||
| "inr", INR; | ||
| "nil", NIL; | ||
| "cons", CONS |
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 don't suppose there's any way we can get an infix :: going?
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.
Done - assume okay to leave nil as "nil" rather than "[]" since this could be again confused with quasilinearity annotations?
…and add explanatory comment
…amed/located tests
|
Hi @SimonJF - sorry for the delay but think I've handled all the requested changes. See a couple of minor questions from me above but otherwise hopefully good to go! |
This PR adds lists as a new data type to Pat, and includes some small example programs as well as an extension of one of the Savina benchmarks.
We managed to fix a couple of the problems I was having, but there are still one and a half problems remaining:
test/pat-tests/list-broken.pattest/examples/savina/kfork.pat