Skip to content
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: type alias #251

Merged
merged 1 commit into from
Aug 28, 2020
Merged

feat: type alias #251

merged 1 commit into from
Aug 28, 2020

Conversation

sinato
Copy link
Contributor

@sinato sinato commented Aug 22, 2020

This PR adds the ability to use type alias in Mun. (maybe close #110 )

The following code should work properly.

type Foo = u32;
fn main() {
    let a: Foo = 1;
}

And the following code should raise a type mismatch.

type Foo = u32;
fn main() {
    let a: Foo = 1.0;
}
error: mismatched type
 --> main.mun:3:17
  |
3 |     let a: Foo = 1.0;
  |                  ^^^ expected `u32`, found `{float}`
  |

NOTE: The following code does not work properly yet.

fn main() {
   type Foo = u32;
   let a: Foo = 10;
}
  • lexer: add the type keyword
  • parsing: add parsing of type TypeName = OtherTypeName
  • hir: add type aliases as type definition
  • tests: add test for all of the above stages
    • lexer: crates/mun_syntax/src/tests/snapshots/mun_syntax__tests__lexer__keywords.snap
    • parser: crates/mun_syntax/src/tests/snapshots/mun_syntax__tests__parser__type_alias_def.snap
    • HIR: crates/mun_hir/src/ty/snapshots/mun_hir__ty__tests__infer_type_alias.snap

@codecov
Copy link

codecov bot commented Aug 22, 2020

Codecov Report

Merging #251 into master will increase coverage by 0.24%.
The diff coverage is 87.80%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
crates/mun_codegen/src/ir/file_group.rs 100.00% <ø> (ø)
crates/mun_hir/src/ty.rs 83.33% <0.00%> (-0.67%) ⬇️
crates/mun_syntax/src/lib.rs 69.56% <ø> (ø)
crates/mun_syntax/src/tests/lexer.rs 100.00% <ø> (ø)
crates/mun_hir/src/ty/infer.rs 71.00% <57.14%> (+0.06%) ⬆️
crates/mun_syntax/src/syntax_kind/generated.rs 65.45% <75.00%> (+0.14%) ⬆️
crates/mun_syntax/src/ast/generated.rs 72.11% <76.47%> (+0.07%) ⬆️
crates/mun_hir/src/diagnostics.rs 61.00% <83.33%> (+1.30%) ⬆️
crates/mun_hir/src/code_model.rs 79.73% <83.87%> (+1.23%) ⬆️
crates/mun_hir/src/adt.rs 90.00% <89.47%> (-0.25%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7147826...a5bf10b. Read the comment docs.

@Wodann Wodann added this to the Mun v0.3.0 milestone Aug 22, 2020
@Wodann Wodann added exp: intermediate Achievable by experienced contributors, or with some guidance type: feat New feature or request and removed exp: intermediate Achievable by experienced contributors, or with some guidance labels Aug 22, 2020
Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

crates/mun_hir/src/name_resolution.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/ty.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

crates/mun_syntax/src/parsing/grammar/adt.rs Show resolved Hide resolved
crates/mun_syntax/src/tests/parser.rs Show resolved Hide resolved
crates/mun_hir/src/ty/tests.rs Show resolved Hide resolved
@sinato
Copy link
Contributor Author

sinato commented Aug 23, 2020

Thank you for the review. I have applied the review suggestions.
I will clean up the history after the review.

Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

crates/mun_hir/src/expr/validator.rs Show resolved Hide resolved
@sinato
Copy link
Contributor Author

sinato commented Aug 24, 2020

If there is anything I can (or should) do to help with the stress test, please let me know.

@Wodann
Copy link
Collaborator

Wodann commented Aug 24, 2020

You can try to use type in weird ways or unexpected places. E.g. fn type Foo = u32; or (type Foo =u32)

If you install the Mun language server for VSCode, you should get continuous feedback to pinpoint errors and crashes more easily.

@baszalmstra
Copy link
Collaborator

baszalmstra commented Aug 24, 2020

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?

@sinato
Copy link
Contributor Author

sinato commented Aug 25, 2020

Both cases seem to result in a cycle in the salsa database.

Thank you for testing! Fixed 00d23e8
I referred to the following commit. rust-lang/rust-analyzer@1c622e9

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?

Of course. I'll fix it.

@Wodann
Copy link
Collaborator

Wodann commented Aug 25, 2020

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 🙂

@sinato
Copy link
Contributor Author

sinato commented Aug 25, 2020

Rebase completed.

@baszalmstra
Copy link
Collaborator

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 crates/mun_codegen/src/test.rs to witness the problem:

#[test]
fn recursive_alias() {
    test_snapshot(
        r#"
    type Alias = Alias;

    fn foo() -> Alias {
    }
        "#,
    )
}

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:

#[test]
fn recursive_struct() {
    test_snapshot(
        r#"
    struct(value) Struct {
        a: Struct
    }
        "#,
    )
}

@Wodann WDYT we should do? Merge this PR and file an issue? Or should at least the alias cycle thing be fixed?

@Wodann
Copy link
Collaborator

Wodann commented Aug 26, 2020

@Wodann WDYT we should do? Merge this PR and file an issue? Or should at least the alias cycle thing be fixed?
@sinato do you think you can come up with a fix for the alias cycle based on @baszalmstra's comments or would you like some help with it? As we are aware of the issue before merging, I'd say that we should ideally resolve it first.

@sinato
Copy link
Contributor Author

sinato commented Aug 26, 2020

@Wodann Maybe there are better implementations, but I think the fix would be as 44ae87d .
I'd be happy to get some advice on this fix.

Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

@baszalmstra
Copy link
Collaborator

Yes! Fine for me too!

@sinato
Copy link
Contributor Author

sinato commented Aug 27, 2020

Thank you for your comment! Fix typo and squashed the commits.

@baszalmstra baszalmstra merged commit 56ec04c into mun-lang:master Aug 28, 2020
@baszalmstra
Copy link
Collaborator

Thank you very much for your contribution and the quick turnaround regarding comments! Great job! 👍👍

@Wodann
Copy link
Collaborator

Wodann commented Aug 28, 2020

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 🙂

@sinato
Copy link
Contributor Author

sinato commented Aug 28, 2020

It was a very nice experience. Thank you for your kind comments!

@sinato sinato deleted the feat/type-aliases branch August 28, 2020 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add type aliases
3 participants