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

Template Literal support #904

Merged
merged 17 commits into from
Jun 10, 2021
Merged

Template Literal support #904

merged 17 commits into from
Jun 10, 2021

Conversation

p-bakker
Copy link
Collaborator

@p-bakker p-bakker commented May 21, 2021

First of all: tnx for @anba for the initial implementation in #81

This PR rebases the work done in #81 and updates it to be compatible with the latest

This is a Draft PR, just to get it out there and get feedback. So, any feedback appreciated :-)

Left to be done:

  1. fix 3 breaking tests from Test262 (see Question: ScriptableObject implements the Map interface, but the Map.get impl. doesn't take into account the ScriptableObject prototype #903, Release 1.7.13 #734 (comment) and https://groups.google.com/g/mozilla-rhino/c/StUgOIRoOFY)
  2. formatting (rather not do some of the formatting work already done in Refactor SlotMap and Slot classes #896)
  3. code review

closes #243
closes #325
closes #773
closes #294

Enables: updating to a newer version of Test262 (see 6c1582a, #817)

@p-bakker
Copy link
Collaborator Author

One of the failing tests is related to this within a function executed in strict mode: https://github.com/tc39/test262/blob/main/test/language/expressions/tagged-template/call-expression-context-strict.js

Am wondering if this is just something that Rhino just doesn't support (yet) in general, that this referenced within functions in strict mode should not be a reference to globals.

If so, I can just mark this test to be excluded and then I'm down to 2 failing tests, which are both the same freezing issue

@tonygermano
Copy link
Contributor

tonygermano commented May 21, 2021

closes #325
closes #773
closes #294
closes #434

Edit: Looks like maybe I can't link these since I'm not the author. It should link them all if you update the first comment to list them one one per line.

@gbrail
Copy link
Collaborator

gbrail commented May 21, 2021

There are lots of problems with strict mode, unfortunately. I think it's fine to disable individual tests if they don't work in strict mode.

@gbrail
Copy link
Collaborator

gbrail commented May 21, 2021

This is a lot of work but it's looking very promising. Probably the main thing I'd do is look at the code coverage from "./gradlew jacocoTestReport" and see that the new aspects of the code are being tested.

@p-bakker
Copy link
Collaborator Author

p-bakker commented May 21, 2021 via email

@p-bakker
Copy link
Collaborator Author

Is it correct that jacocoTestReport doesn't generate any output if there are failing tests? Cause I'm not getting any output from jacoco (but I have some failing tests)

Found an issue in CodeGen wrt Escape Sequences which only manifests itself when not running at optLevel -1

@gbrail
Copy link
Collaborator

gbrail commented May 21, 2021 via email

@p-bakker
Copy link
Collaborator Author

p-bakker commented Jun 1, 2021

Update on where things are with this PR:

  1. All formatting changes spotless desired have been applied
  2. There are currently 2 failing tests in Test262:
    1. one related to strict mode. Commented out in test262.properties, as it depends on Bound functions leak global scope into strict functions  #442
    2. one related to proper freezing on Objects in Tagged Templates. Commented out for now, but still needs to be fixed before this PR would be ready to merge
  3. I ran the code coverage reports and these are the findings:
    1. One piece of code is not covered, imho due to Test262 not providing full coverage. Created Test missing for \u{10000} ... \u{10FFFF} in Template Literals tc39/test262#3001 for this. Also led me to create Support ES2015 unicode escape sequence additions  #917
    2. Some code paths related to parse exceptions aren't hit, like an unfinished Template Literal at the end of a file. Must these paths all be covered? I see similar exception handling code in other classes that also doesn't get coverage
    3. the ast/TaggedTemplateLiteral, ast/TemplateCharacters and ast/TemplateLiteral have stuff like .visitor(...) and .toSource(...) that aren't being covered. Should they? I see those methods are also not covered in BigIntLiteral for example
    4. Lastly, ast/TemplateLiteral has a couple of public methods the rounds out it's api nicely, but those methods aren't being called anywhere from the Rhino source. I didn't author these myself, they came from @anba's PR, so not 100% sure what to do... leave as is, remove or make sure they are covered with tests?

All in all, it's getting pretty close :-)

manual rebase of Patch 2 & 3 for
mozilla#81
Not 100% yet, 3 tests are still failing, 2 of them due to not properly
freezing some objects
- removed omit for tests that now passes
- omitted strict mode test for tagged template
- temporarily omitted test for proper freezing within template literal
implementation (still to be fixed before this PR could be merged)
Just for code added/changed in this PR
For code in files touched for changes in this PR
@p-bakker p-bakker marked this pull request as ready for review June 8, 2021 08:18
@p-bakker
Copy link
Collaborator Author

p-bakker commented Jun 8, 2021

So, from my POV this PR is ready to merge!

As per #904 (comment), there's one failing test (due to general strict mode issues in Rhino) and a few area's of code not covered by testing code, but those uncovered area's are also uncovered in similar classes.

@gbrail
Copy link
Collaborator

gbrail commented Jun 10, 2021

This looks good to me. I'm going to go ahead and merge it. Thanks!

@gbrail gbrail merged commit 064130e into mozilla:master Jun 10, 2021
@tonygermano
Copy link
Contributor

I think there needs to be some kind of celebration for this one.

@dh-pt
Copy link

dh-pt commented Sep 30, 2021

This ... is an amazing pr. Wow, thanks team.

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