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

Implement ES2015 const #939

Open
p-bakker opened this issue Jun 22, 2021 · 11 comments
Open

Implement ES2015 const #939

p-bakker opened this issue Jun 22, 2021 · 11 comments
Labels
behavior block-scope bug Issues considered a bug Ecma Incompatibility Issues about Rhino being incompatible with the EcmaScript spec
Milestone

Comments

@p-bakker
Copy link
Collaborator

p-bakker commented Jun 22, 2021

While Rhino has a const implementation, it is based on a draft EcmaScript specification that precedes the official ECMAScript 2015 specification in which const got introduced. As such, the current const implementation is far from standards compliant.

The const specification in ECMAScript 2015 is pretty closely aligned with the let implementation in the same spec, with the major difference being that const's cannot be updated, while let's can.

So, a possible implementation path is to piggy-back off of the let implementation and put the old const implementation behind a flag. Need to decide which flag this will be. Should the flag just be the JavaScript version flag and make a breaking change to Context.VERSION_ES6?

Note 1: the let implementation also has some non-standard extensions based on old draft specifications (that should not be enabled for the const implementaiton when piggy-backing off of the let implementation), see #923
Note 2: the let implementation doesn't support Temporal Dead Zone, see #969
Note 3: looks like the let implementation has some scoping issues: #834

@p-bakker
Copy link
Collaborator Author

p-bakker commented Jun 22, 2021

Related issues: #647, #719 and #326

@p-bakker p-bakker changed the title Implement ECMAScript 2015 const Implement ES2015 const Jun 23, 2021
@p-bakker p-bakker added this to the ES2015 milestone Jun 29, 2021
@p-bakker p-bakker added the Ecma Incompatibility Issues about Rhino being incompatible with the EcmaScript spec label Jun 30, 2021
@tonygermano
Copy link
Contributor

I'm honestly not sure that the old behavior needs to be preserved at all. Are there any situations where fixing const would break code that was previously working?

To my knowledge the current implementation is more restrictive than it needs to be, preventing its use in the first place where it should be allowed.

@p-bakker
Copy link
Collaborator Author

Have a look at the related cases mentioned above: I think there are several cases where, if we'd just implement the correct behavior, existing code that depends on the current behavior, would break.

How likely is that and does it warrant putting the new (or old) behavior behind a (language version) flag idk...

@p-bakker p-bakker added the test262 harness Issues related to failing test262 tests, due to Rhino not yet support certain JS features label Jan 31, 2022
@p-bakker
Copy link
Collaborator Author

Due to the non-standard const impl. of Rhino, a for (const x of/in ...) { const y = ...} loop bombs out for various reasons.

As the test262 harness code uses this (conditionally for testing unicode regexes), this case is sorta a blocker for Support ES2015 Regex u flag (Unicode).

Locally one can change the harness code to use a let instead (for (let x of string) {...}) and see if all the unicode regex tests pass, but in our CI the test would fail, so would have to be marked as failing not to fail the build

@tuchida
Copy link
Contributor

tuchida commented Feb 1, 2022

How about transpiling the harness code with Babel?
I could use this.

diff --git a/build.gradle b/build.gradle
index a05bfcb6..282bcbbd 100644
--- a/build.gradle
+++ b/build.gradle
@@ -9,6 +9,7 @@ plugins {
     id 'checkstyle'
     id 'com.diffplug.spotless' version "5.12.1"
     id 'com.github.spotbugs' version "4.7.1"
+    id 'com.github.bryncooke.babel-gradle' version '1.0.1'
 }
 
 tasks.withType(JavaCompile) {
@@ -513,4 +514,10 @@ distTar {
     archiveExtension = 'tar.gz'
 }
 
+babel {
+    presets "es2015"
+    inputDir = file("test262/harness/")
+    outputDir = file("testsrc/build/test262/harness/")
+}
+
 distZip.dependsOn javadoc, jar, sourceJar, runtimeSourceJar
diff --git a/testsrc/org/mozilla/javascript/tests/Test262SuiteTest.java b/testsrc/org/mozilla/javascript/tests/Test262SuiteTest.java
index 1ed9dfb5..70687de2 100644
--- a/testsrc/org/mozilla/javascript/tests/Test262SuiteTest.java
+++ b/testsrc/org/mozilla/javascript/tests/Test262SuiteTest.java
@@ -69,7 +69,7 @@ public class Test262SuiteTest {
     static final int[] OPT_LEVELS;
 
     private static final File testDir = new File("test262/test");
-    private static final String testHarnessDir = "test262/harness/";
+    private static final String testHarnessDir = "testsrc/build/test262/harness/";
     private static final String testProperties;
 
     private static final boolean updateTest262Properties;

./package.json

{
  "name": "example",
  "version": "1.0.0",
  "dependencies": {
    "babel-preset-es2015": "^6.24.1"
  },
  "devDependencies": {
    "babel-cli": "^6.26.0"
  },
  "author": "",
  "license": "ISC",
  "description": ""
}

Now it works.

$ npm install
$ ./gradlew babel
$ ./gradlew test --tests Test262SuiteTest --rerun-tasks -DupdateTest262properties

You can get it to work by having the developer install npm or by including the converted code in the repository.
I have not checked the license.

@p-bakker
Copy link
Collaborator Author

p-bakker commented Feb 1, 2022

Its definetly something that would at least allow us to run against the latest test262 version, so from that point of view we could do this

If we do, I think that:

The only downside of going this route is that there might be less urgency to fix the issues/implement the missing features in Rhino that require us to use Babel.

Anyone else got opinions?

@p-bakker
Copy link
Collaborator Author

p-bakker commented Feb 1, 2022

And maybe also somehow have a way to run test262 without transpiling the harness code using Babel

@henning-meinhardt
Copy link

Hi @p-bakker, do you have a rough estimate when this could be fixed?

@p-bakker
Copy link
Collaborator Author

Don't have an eta: Rhino is maintained by volunteers, who work on things when they have spare time

Personally, I'd like to see this one fixed sooner rather than later and I've tried to take a stab at it in the past, but not sure when I may get back to looking into it

@rbri
Copy link
Collaborator

rbri commented Aug 19, 2024

@henning-meinhardt if you only like to only run some js code that uses 'const' you can give https://github.com/HtmlUnit/htmlunit-core-js a try. This is repackaged Rhino with some enhancement/patches.

One is the handling of 'const' like 'let'; this allows to run more js code but this will not fail if your code refines a const var....
see HtmlUnit@82c5041

@tonygermano
Copy link
Contributor

I had a comment written that I was about to post, and realized it was almost exactly the same as what I already said in #326 (comment).

I don't think backwards compatibility is important on this one, especially considering it's been preventing people from using const for years. I think a big warning in the release notes when it changes will be good enough to alert the possibly non-existent users who depend on the archaic behavior and actually upgrade Rhino to the current release.

I think the main use cases that people want to use and we currently don't support have to do with iterative loops. Rather than letting backwards compatibility or having a fully functional temporal dead zone hold us back, I think supporting these common uses should be a priority.

In a loop block statement,

for (let i = 0; i < arrayLike.length; i++) {
  const element = arrayLike[i]
  // ...
}

In a for-of/for-in declaration,

for (const [k,v] of entries) {
  // ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior block-scope bug Issues considered a bug Ecma Incompatibility Issues about Rhino being incompatible with the EcmaScript spec
Projects
None yet
Development

No branches or pull requests

5 participants