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

Clarification on EmptyStatement #14

Closed
imteekay opened this issue Feb 24, 2023 · 6 comments
Closed

Clarification on EmptyStatement #14

imteekay opened this issue Feb 24, 2023 · 6 comments

Comments

@imteekay
Copy link

Hi @sandersn,
I just wanted to clarify the first exercise "Add EmptyStatement".

Is an EmptyStatement a semicolon in JavaScript/TypeScript (link)? So the idea is to stop the execution of a statement?

@imteekay
Copy link
Author

imteekay commented Feb 25, 2023

By the way, @sandersn, I've opened a PR on my fork about this first exercise I wanted to know if you could give me feedback. Maybe it's not the best implementation or it's not even what you expected as a result (perhaps I should handle it by creating a new ast node and not filtering undefined values) but I could manage to pass all the tests now.

Here it's: imteekay/mini-typescript#2

Thanks!

@kevinramharak
Copy link

Hi @imteekay,
Im not Sanders but since i'm watching this repo I got a notification.

I have the same assumption that the EmptyStatement is the same as the one talked about in the link.
I would not say that it stops the execution of a statement, but that its a statement without anything to execute. MDN is a great source, I suggest also peeking at what part of the language spec its referencing
As you can read at runtime it does nothing but "return empty".
If you are interested in how to read the specification, I suggest googling "how to read the ECMAScript specification". It will provide you with various articles and blog posts that dive into the separate details of how to read the spec.
This is of course not required, but when implementing a compiler for an existing language I find it useful to learn how to read the official specifications.

On your PR:
While its an interesting take, I would argue using undefined to represent an empty statement while passing the tests is not a great approach.
You already mentioned creating a new AST node type, I think that would be a better approach.
The main argument for me would be to have an accurate representation of the source code in the AST.
Using undefined to represent an empty statement is unclear and can cause ambiguity when the compiler increases in complexity. An explicit EmptyStatement type would leave no doubt on what it represents.

@sandersn
Copy link
Owner

Thanks @kevinramharak . I agree with your comments.

@imteekay I'll leave a review on the PR although my overall take is the same as @kevinramharak 's.

@imteekay
Copy link
Author

imteekay commented Apr 10, 2023

@kevinramharak, thanks for the overview!

@sandersn, I made some improvements now using an AST node for the empty statement.
For this test var x: number = 1;;var y: number = 2;, my implementation is still failing because of the separator function as the semicolon separates the var x: number = 1 and ; for this test and when it gets out of the parseSeparated, it expects to the next token to be an EOF token but there're still tokens to be parsed.

I'm still thinking about how to make it work and wanted to ask if there's any relation to the second exercise "Make semicolon a statement ender, not statement separator." or if it's simpler than that.

The changes I made remains in the same PR imteekay/mini-typescript#2

@imteekay
Copy link
Author

Now the PR has the full solution imteekay/mini-typescript#2

  • New token for the empty statement
  • New test
  • Updated the existing tests

@imteekay
Copy link
Author

imteekay commented May 2, 2023

Thanks for all the help guys! Just closing this issue as I merged the PR 🎉

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

No branches or pull requests

3 participants