[Merged by Bors] - Implement Classes #1976
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
raskad
added
enhancement
raskad
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify and
jedel1043
|
Too many passed tests for github comments... VM implementation
Fixed tests (6766):Broken tests (71): |
Codecov Report
@@ Coverage Diff @@
## main #1976 +/- ##
==========================================
- Coverage 45.90% 43.87% -2.04%
==========================================
Files 206 211 +5
Lines 17148 18662 +1514
==========================================
+ Hits 7872 8188 +316
- Misses 9276 10474 +1198
Continue to review full report at Codecov.
|
Benchmark for aeb6891Click to view benchmark
|
raskad
force-pushed
the
implement-classes
branch
from
ed9b9ae
to
4ee3490
Compare
Benchmark for 9ec374eClick to view benchmark
|
Benchmark for 7ac4292Click to view benchmark
|
Razican
reviewed
Mar 26, 2022
Benchmark for 43ff007Click to view benchmark
|
Razican
reviewed
Mar 27, 2022
There was a problem hiding this comment.
Looks very good from my side :) I added some improvements that could be done. Especially in the parser.
The bytecompiler module is getting very out of hand... but not related to the PR. Almost 3k lines... We really need #1808 and #1608 to be implemented asap, since this is a huge issue for being able to contribute to the compiler.
It would be good to document how the idea behind each opcode is thought of. The order of the operands in the stack, how is the implementation... But maybe can be done in PRs to fix those two issues.
Comment on lines
+53
to
+57
| let _timer = Profiler::global().start_event("ClassExpression", "Parsing"); | ||
| let strict = cursor.strict_mode(); | ||
| cursor.set_strict_mode(true); | ||
|
|
||
| let token = cursor.peek(0, interner)?.ok_or(ParseError::AbruptEnd)?; |
There was a problem hiding this comment.
Looking at the spec, the first token of this parser should be the class keyword, right? I guess this should "expect" that keyword.
I guess it's not here because the parser that calls it consumes it before calling it, but I think that should be changed.
There was a problem hiding this comment.
Yeah that is exactly it. I will open a issue for this, because it will mean replacing next with peek in multiple places and as a result multiple parsers need to be changed. We have this behaviour in lot's of parsers, so doing this in a dedicated issue seems like the best idea.
Comment on lines
+104
to
+106
| TokenKind::Keyword(Keyword::Class) => { | ||
| ClassExpression::new(self.name, false, false).parse(cursor, interner) | ||
| } |
There was a problem hiding this comment.
I guess the "TODO" a bit higher is the issue. I guess this could be fixed in a future PR.
| { | ||
| type Output = Class; | ||
|
|
||
| fn parse( |
There was a problem hiding this comment.
This parser is way too big, and it's not clear how to follow the spec. It seems that this parser should be pretty simple: just check if the extends keyword exists, and if so, call the ClassHeritage parser, and then, expect the {, call the ClassBody parser, then expect the }.
Then, we could have all the different parsers defined (well, ClassBody would be a type alias for ClassElementList, for example). Dividing this parser into multiple smaller parsers should make things more maintainable (and reusable, if any of them gets referenced anywhere else in the future).
There was a problem hiding this comment.
I refactored the parser to match the spec better. I think it got a bit better for sure. Some syntax error handling is now consolidated instead of spread throughout hundreds of lines.
The ClassElement parser is still ~700 lines long. We maybe can get the line count down with some helpers, but I think it is mostly the natural complexity of the parser. I guess this is probably one of the most complex parsers in spec, because we have fields, methods of all kinds, all of that in static, constructors, static blocks and all of that with private identifiers.
Benchmark for 5c84d48Click to view benchmark
|
raskad
force-pushed
the
implement-classes
branch
from
f163dc8
to
c71bffe
Compare
|
Rebased |
Benchmark for 66bc9d5Click to view benchmark
|
Razican
approved these changes
Apr 7, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Apr 7, 2022
This Pull Request fixes/closes #337. It changes the following: - Implement class declaration parsing. - Implement class expression parsing. - Implement class execution. There are still some features like `super` missing and there are some early errors that are not implemented yet. But I think it makes sense to merge this, as we can branch out the missing features from here.
|
Pull request successfully merged into main. Build succeeded: |
Razican
pushed a commit
that referenced
this pull request
Jun 8, 2022
This Pull Request fixes/closes #337. It changes the following: - Implement class declaration parsing. - Implement class expression parsing. - Implement class execution. There are still some features like `super` missing and there are some early errors that are not implemented yet. But I think it makes sense to merge this, as we can branch out the missing features from here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request fixes/closes #337.
It changes the following:
There are still some features like
supermissing and there are some early errors that are not implemented yet. But I think it makes sense to merge this, as we can branch out the missing features from here.