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

[language/move ir] implement parser & compiler for generics #597

Merged
merged 1 commit into from Aug 23, 2019

Conversation

@vgao1996
Copy link
Contributor

commented Aug 16, 2019

Summary

This implements the parser & compiler for generics. New tests are added.

Test Plan

cargo test
Copy link

left a comment

Just some nits, but what you have is mostly what I would have done. I'll take a look tomorrow to take a look at the lalrpop output

@vgao1996 vgao1996 requested a review from sblackshear Aug 16, 2019
@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch from de7fe77 to aad5c43 Aug 16, 2019
@vgao1996

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Still largely WIP.

Code compiles but there's a missing feature:

  • Generate kind constraints for struct handles

TODO:

@vgao1996 vgao1996 changed the title [language/move ir] implement parser for generics [language/move ir] implement parser & compiler for generics Aug 16, 2019
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct KindConstraint(pub String, pub Kind);

This comment has been minimized.

Copy link
@tnowacki

tnowacki Aug 16, 2019

nit type parameter
And I'd probably use named fields

pub struct TypeParameter {
  pub name: String,
  pub constraint: Kind,
}

This comment has been minimized.

Copy link
@sblackshear

sblackshear Aug 20, 2019

Contributor

Agree with Todd on this one.

language/compiler/ir_to_bytecode/syntax/src/ast.rs Outdated Show resolved Hide resolved
language/compiler/ir_to_bytecode/syntax/src/ast.rs Outdated Show resolved Hide resolved
@vgao1996

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@tnowacki Sorry if it confused you. I haven't made the changes you & Sam suggested, but they'll be included in a later refactor & cleanup. My current focus is to make everything compile and emit correct bytecode. The cleanup will follow shortly after these.

@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch from aad5c43 to 4cc00a9 Aug 19, 2019
@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch from 4cc00a9 to 98165a3 Aug 20, 2019
@vgao1996

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Current status:

  • Parser problem solved.
  • Compiler runs & emits code for generics, some simple tests have been added.
  • Parser trick @tnowacki suggested not yet implemented. This is coming next.
  • Cleanup, renaming etc. not started.
@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch from 98165a3 to 0ada4b0 Aug 20, 2019
@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch from 0ada4b0 to 6594809 Aug 20, 2019
@vgao1996

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@tnowacki I've implemented the parser trick you suggested. Feel free to have a look at that part!

@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch 2 times, most recently from 32c0357 to c6885b4 Aug 20, 2019
language/compiler/ir_to_bytecode/syntax/src/ast.rs Outdated Show resolved Hide resolved
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct KindConstraint(pub String, pub Kind);

This comment has been minimized.

Copy link
@sblackshear

sblackshear Aug 20, 2019

Contributor

Agree with Todd on this one.

language/compiler/ir_to_bytecode/syntax/src/ast.rs Outdated Show resolved Hide resolved
language/compiler/ir_to_bytecode/syntax/src/ast.rs Outdated Show resolved Hide resolved
@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch 4 times, most recently from c18c4c5 to 97fe0a5 Aug 20, 2019
@vgao1996 vgao1996 force-pushed the vgao1996:parser_generics branch from 97fe0a5 to cdc1c77 Aug 23, 2019
@vgao1996 vgao1996 requested review from tnowacki and sblackshear Aug 23, 2019
@vgao1996

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@tnowacki @sblackshear I'm done with the cleanup/renaming and I think it's time to give this PR a proper review. Again thanks for following up on this PR!

Note there is one feature I left out: named fields for type formals

pub struct TypeFormal {
  pub type_var: TypeVar,
  pub constraint: Kind,
}

This is because we currently use (Var, Type) for formals, and I'd prefer keeping them consistent. If one needs to be changed, the other should also be changed. This brings quite a bit of distraction so I suggest we move forward with this PR and put up another one for it.

Copy link
Contributor

left a comment

Nice!

//! no-run: verifier, runtime

module M {
struct Foo<T> { x: T }

This comment has been minimized.

Copy link
@sblackshear

sblackshear Aug 23, 2019

Contributor

Nit: resource Foo so this will typecheck

@@ -0,0 +1,24 @@
//! no-run: verifier, runtime

This comment has been minimized.

Copy link
@sblackshear

sblackshear Aug 23, 2019

Contributor

Is there any harm in running the verifier here (similar for other tests)?

This comment has been minimized.

Copy link
@vgao1996

vgao1996 Aug 23, 2019

Author Contributor

Yes. Currently the in the verifier there's logic like "get the kind of a SignatureToken" (without the context), which panics when a type parameter is seen.

@vgao1996 vgao1996 merged commit 01d919e into libra:master Aug 23, 2019
1 check passed
1 check passed
commit-workflow Workflow: commit-workflow
Details
@vgao1996 vgao1996 deleted the vgao1996:parser_generics branch Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.