-
Notifications
You must be signed in to change notification settings - Fork 111
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
feat: destructuring of structs and messages #856
Conversation
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 my comment for the grammar.ohm
file
@jeshecdom @anton-trunov have a look at the reworked apporach please! |
@Gusarich There are some merge conflicts |
Resolved them, trying to push but knip doesn't let me because for some reason it thinks that |
hmmm, this is weird, because I removed that from ignoreDependencies (see the getter method id pr) precisely because I was getting warnings that it is useless can you try updating your knip installation locally? |
yeah, just had to run |
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 left a few comments and suggestions for the changes. Since this is a complex feature (yeah, codegen is quite simple, but the static semantics is a bit tricky), we need to test it more thoroughly, so let's add at least the following tests:
- positive interpreter (const-eval) tests (including various combinations of punned and mapped fields);
- negative interpreter tests;
- negative typechecker tests with the scoping rules violations, like
let S {x: x, y: x}
and shadowing of previously defined variables; - negative typechecker tests with all the new error messages (that are non-ICE ones);
- pretty-printer tests;
- renamer tests;
@jeshecdom Please have a look at the interpreter changes and suggest more positive and negative tests. And also, please help with the var-scoping and typechecking rules review |
|
If needed, I can quickly help write the docs once all the other suggestions above are resolved :) |
@anton-trunov @jeshecdom I think I've made all the changes you requested in the review. Please have a look! Discovered and fixed a few small bugs while writing all these tests 👍 |
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.
LGTM, @anton-trunov
@anton-trunov resolved everything, have a look please! |
Co-authored-by: Novus Nota <[email protected]>
@novusnota your turn :) |
@anton-trunov on it! UPD: #964 |
Issue
Closes #298.
Checklist