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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix / Enhancement for Omitted Object Literals #1229

Merged
merged 10 commits into from
Apr 21, 2020

Conversation

torch2424
Copy link
Contributor

closes #1226

This fixes the bug where omitted Object literals would just give random memory from the runtime. By introducing an error for class fields that cannot be omitted.

This Enhances Object literals, by allowing the basic AS Types. To be omitted, with default values of zero 馃槃

TODO: Either we add support for nullable types to default to null and/or add support for making a new instance of a class if the constructor has no parameters in this same PR. Or we can do that work in a seperate PR 馃槃

cc @dcodeIO @jtenner

P.S. Yay! My first real AS Compiler PR 馃槃 馃帀

Screenshots

Screenshot from 2020-04-16 17-45-32
Screenshot from 2020-04-16 17-45-06
Screenshot from 2020-04-16 17-44-29

@torch2424
Copy link
Contributor Author

Oops, seems like I am failing the self-compilation tests 馃檭

@dcodeIO did you get things self compiling? Should I go back and fix these? Shouldn't be too hard, all I think I really need to do is get rid of the phat arraw function I added 馃槃 But how do I test the self compiler from my machine? 馃

Copy link
Member

@dcodeIO dcodeIO left a comment

Choose a reason for hiding this comment

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

Had an initial look and found a couple places that are fine in TS, but a bit tricky in the compiler's "portable AssemblyScript" codebase :)

src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
@dcodeIO
Copy link
Member

dcodeIO commented Apr 17, 2020

Regarding self-compilation: Yes, this is indeed part of CI now. The relevant scripts are npm run asbuild to build it, and npm run astest to test it. Unless on top of the vtable branch, the test will not do much and ultimately fail, which is expected, while on top of the vtable branch, it is good enough to output a simple module in text format.

@MaxGraey
Copy link
Member

Isn't this relate to #1155 ?

@torch2424
Copy link
Contributor Author

@dcodeIO Thanks for the quick review! 馃槃 馃憤 Gonna start addressing comments in a little bit!

@MaxGraey Ummmm, so I am not too sure? I think it's similar, but a different issue? I'll let @dcodeIO answer that.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 17, 2020

It's somewhat related conceptually, yeah. Let's say we call the (default) ctor on object creation, then the ctor may initialize some of these fields, so specifying these is not necessary even though the field is not nullable. However, we are not calling the ctor here even though we should.

Suggesting to do the following to be safe for now: As soon as the class has a constructor, disallow creating it from an object literal. This should avoid any issues, and we can look into actually calling ctors in a later PR.

@jtenner
Copy link
Contributor

jtenner commented Apr 17, 2020

It's unfortunate that we aren't like rust, and get to have traits where object literals might make sense.

@torch2424
Copy link
Contributor Author

@dcodeIO Made all requested changes, and asbuild works on my machine 馃槃 馃憤 This is good to go!

Screenshot from 2020-04-17 15-53-45

@jtenner
Copy link
Contributor

jtenner commented Apr 17, 2020

Nice work @torch2424

Copy link
Member

@dcodeIO dcodeIO left a comment

Choose a reason for hiding this comment

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

Thanks! Got a few additional comments, with one RC issue not introduced by this PR, but seems good to fix as well.

src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
@torch2424
Copy link
Contributor Author

Thanks @jtenner ! 馃槃 馃帀

And made the requested changes @dcodeIO ! 馃槃

Screenshot from 2020-04-20 11-31-32
Screenshot from 2020-04-20 11-31-15
Screenshot from 2020-04-20 11-30-07

Copy link
Member

@dcodeIO dcodeIO left a comment

Choose a reason for hiding this comment

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

Great! Just a couple nits left, and a suggestion how to support omitting fields that are nullable anyway :)

src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
src/compiler.ts Outdated Show resolved Hide resolved
@torch2424
Copy link
Contributor Author

@dcodeIO Made requested changes, tests still pass, asbuild still working 馃槃 馃憤

@dcodeIO dcodeIO merged commit 53e314c into AssemblyScript:master Apr 21, 2020
@dcodeIO
Copy link
Member

dcodeIO commented Apr 21, 2020

Thank you! :)

@torch2424
Copy link
Contributor Author

Yayyyy! 馃槃 @dcodeIO Thank you very much for the review. Literally tweeting this out now 馃槀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Check for Omitted fields in compileObjectLiteral
4 participants