-
Notifications
You must be signed in to change notification settings - Fork 80
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
revise existing tests to match #311 (subtype checking only for refere… #323
base: master
Are you sure you want to change the base?
Conversation
I guess we should implement the new rules in Haskell and Rust to compare notes on these updated tests. (And we means me and Yan respectively) |
The fact that no-one is confident about how this is meant to behave strikes me as slightly worrying ;-> |
Just because it changes every few months doesn’t mean we are not confident? ;-) |
@nomeata thanks for adjusting the tests to use a method name - most of my tests now pass but there's another bug I'll need to figure out on the motoko side (see below). More pretty printing crap, I'm afraid. Should be possible to sort out (by me). Output form a private branch (don't try this at home)
|
Including LICENSE and a proper author/description/keywords. More could be added later but this fixes the requirements for open sourcing. Fixes dfinity#322 Co-authored-by: Kyle Peacock <[email protected]>
assert blob "DIDL\02\6a\00\01\01\00\6c\01\00\01\01\00\01\01\00\01m" | ||
== "(null)" : (opt func () -> (empty)) "(µ record) </: empty"; |
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.
assert blob "DIDL\02\6a\00\01\01\00\6c\01\00\01\01\00\01\01\00\01m" | |
== "(null)" : (opt func () -> (empty)) "(µ record) </: empty"; | |
// assert blob "DIDL\02\6a\00\01\01\00\6c\01\00\01\01\00\01\01\00\01m" | |
// == "(null)" : (opt func () -> (empty)) "(µ record) </: empty"; |
I will comment out this test for now. It's a bit implementation dependent. In Rust, we replace µ record
with empty
after parsing the type table, so we actually get opt func
instead of null
.
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.
Can you explain why? I don't quite follow.
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.
In the Haskell code I think I too replace mu record
with empty, but only after subtype checking?
I believe Motoko gets this right as spec'ed.
If we want to allow implementations to differ here we should say so explicitly in the spec. But I don't know if that's a good idea.
Could you introduce in the rust code a MuRecord
type that you can replace it with, which does the job of preventing your code to run into these vicious cycles, while still having the right subtypes relations?
Hmm, but mu record <: mu (record opt)
, so you probably really can't throw away the structure before subtype checking.
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.
Resolved offline. \mu record
is shorthand for \mu t. record { 0 = t }
which I agree is isomorphic to empty with least fixed points. Do you agree @nomeata or is it actually not safe to identify them in the type table, even if they are isomorphic.
My worry is that Motoko will still distinguish the types that Rust is conflating, which might lead to (obscure) trouble elsewhere.
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.
It’s safe, I think, but it is not according to specification. If we go that route, it would be consistent to equate all other empty types as well (empty
, {empty, empty}
etc.), and it should be part of the spec. And I would guess that @rossberg doesn’t like it :-)
Co-authored-by: Yan Chen <[email protected]>
Co-authored-by: Yan Chen <[email protected]>
…nce types)
I think these tests are need to change in light of #311 (as verified by Motoko testing)