Conversation
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
==========================================
+ Coverage 78.50% 78.75% +0.24%
==========================================
Files 215 215
Lines 12709 12857 +148
==========================================
+ Hits 9977 10125 +148
Misses 2732 2732
Continue to review full report at Codecov.
|
baszalmstra
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Im unable to test this right now, but does it give a proper error in this case?:
type Foo = Bar; // Unknown type `Bar`
Wodann
left a comment
There was a problem hiding this comment.
Overall very impressive that you were able to largely pulled off such a complex task as your first PR.
There are still some corner cases that are not being handled. Could you have a look at those?
|
Thank you for the review. I have applied the review suggestions. |
Wodann
left a comment
There was a problem hiding this comment.
Nice. Thank you for the quick turn around! 🙂
To polish, there is just some missing documentation left, and we are going to locally do a stress test to see if we can discover syntax that breaks the type aliasing.
|
If there is anything I can (or should) do to help with the stress test, please let me know. |
|
You can try to use If you install the Mun language server for VSCode, you should get continuous feedback to pinpoint errors and crashes more easily. |
|
I did a bit of stress testing and found this issue: #[test]
fn recursive_alias() {
infer_snapshot(
r#"
struct Foo {}
type Foo = Foo;
type A = B
type B = A;
"#,
)
}Both cases seem to result in a cycle in the salsa database. Also, would you be able create a PR on this repo: https://github.com/mun-lang/vscode-extension to support type aliases in the syntax highlighting? |
Thank you for testing! Fixed 00d23e8
Of course. I'll fix it. |
|
Very quick fix; nice! There seems to be a merge conflict, but other than that I am happy to merge this. As far as I am concerned, you can go ahead with the rebase 🙂 |
00d23e8 to
aebe7b3
Compare
|
Rebase completed. |
|
Your solution for recursive types is still incomplete. There are no errors reported for the recursive aliases which will crash the codegen. The codegen assumes that if there are no errors the HIR is in a valid state. You can add this test to I see there is actually a comment that states we need to implement cyclic types. 😅 This also doesn't throw an error but does break the compiler: @Wodann WDYT we should do? Merge this PR and file an issue? Or should at least the alias cycle thing be fixed? |
|
Wodann
left a comment
There was a problem hiding this comment.
Thanks for solving the issue with cyclic type references. This seems like a good enough approach for now.
I found one typo, could you fix that? Then I am happy to merge it 🙂
|
Yes! Fine for me too! |
44ae87d to
a5bf10b
Compare
|
Thank you for your comment! Fix typo and squashed the commits. |
|
Thank you very much for your contribution and the quick turnaround regarding comments! Great job! 👍👍 |
|
Yes, thank you so much, @sinato! I can't wait for your next contribution; it's been a pleasure collaborating. We could really use another regular contributor 🙂 |
|
It was a very nice experience. Thank you for your kind comments! |
This PR adds the ability to use type alias in Mun. (maybe close #110 )
The following code should work properly.
And the following code should raise a type mismatch.
NOTE: The following code does not work properly yet.
crates/mun_syntax/src/tests/snapshots/mun_syntax__tests__lexer__keywords.snapcrates/mun_syntax/src/tests/snapshots/mun_syntax__tests__parser__type_alias_def.snapcrates/mun_hir/src/ty/snapshots/mun_hir__ty__tests__infer_type_alias.snap