double is reserved keyword by ethanalee · Pull Request #15110 · emscripten-core/emscripten

ethanalee · 2021-09-23T20:17:35Z

Encountered an error while running es-check

Apparently double is considered a reserved keyword, but it hasn't been since ES3?

https://stackoverflow.com/questions/32171223/double-keyword-in-javascript

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar

Error Message:

ES-Check Error:
          ----
          · error: SyntaxError: The keyword 'double' is reserved (1804:12)
          · see the printed err.stack below for context
          ----

sbc100

Hm.. do you know why this doesn't show up in our test suite when we run es-check?:

emscripten/tests/common.py

Lines 537 to 538 in 8b7c8f6

    
           def verify_es5(self, filename): 
        
             es_check = shared.get_npm_cmd('es-check')

sbc100 · 2021-09-23T20:42:27Z

      assert(ch === 100/*'d'*/ || ch === 102/*'f'*/ || ch === 105 /*'i'*/);
 #endif
-      // A double takes two 32-bit slots, and must also be aligned - the backend
+      // A double or readAsmConstArgsDouble takes two 32-bit slots, and must also be aligned - the backend


Did you mean o change the comment here?

I just put the or there in case someone became confused as to what readAsmConstArgsDouble was.

I don't think you want the "or readAsmConstArgsDouble" here since "A double" is referring the a type, not a variable name.

Ok I can revert it

Will hold off until we have this resolved

sbc100 · 2021-09-23T20:44:50Z

    // Memory resize rules:
    // 1. Always increase heap size to at least the requested size, rounded up to next page multiple.
-    // 2a. If MEMORY_GROWTH_LINEAR_STEP == -1, excessively resize the heap geometrically: increase the heap size according to 
+    // 2a. If MEMORY_GROWTH_LINEAR_STEP == -1, excessively resize the heap geometrically: increase the heap size according to


Personally I prefer not to mix semantic changes with whitespace changes (I think it makes PRs a little harder to review).

(For this reason I configure my editor to trim whitespace on demand rather than on save.. so it lets me decide when I want to do it).

Not a huge deal we can land this PR as is.

Noted. I will change this in my editor as well.

kripken · 2021-09-23T20:53:38Z

Hm.. do you know why this doesn't show up in our test suite when we run es-check?:

I discussed this with @ethanalee earlier today, yeah - this is very strange. I was hoping you'd have a theory @sbc100 ...

If not, then we should probably investigate that, as it suggests we are not actually running full es-check somehow.

@ethanalee was this the only error you had? That is, after fixing it, did es-check pass on your PR?

ethanalee · 2021-09-23T21:07:56Z

Hm.. do you know why this doesn't show up in our test suite when we run es-check?:

I discussed this with @ethanalee earlier today, yeah - this is very strange. I was hoping you'd have a theory @sbc100 ...

If not, then we should probably investigate that, as it suggests we are not actually running full es-check somehow.

@ethanalee was this the only error you had? That is, after fixing it, did es-check pass on your PR?

Hi, @kripken and @sbc100 . Yep, I just double-checked and renaming the var double ensures the test passes.

test_intvars (test_core.wasm0) ... ES-Check: there were no ES version matching errors!  🎉
wasmfs
ES-Check: there were no ES version matching errors!  🎉
ok

----------------------------------------------------------------------
Ran 1 test in 4.579s

OK

sbc100 · 2021-09-23T22:19:59Z

Hm.. do you know why this doesn't show up in our test suite when we run es-check?:

I discussed this with @ethanalee earlier today, yeah - this is very strange. I was hoping you'd have a theory @sbc100 ...

If not, then we should probably investigate that, as it suggests we are not actually running full es-check somehow.

@ethanalee was this the only error you had? That is, after fixing it, did es-check pass on your PR?

I'd like to investigate a little more. @ethanalee can you give me the exact command that you ran that generated the error? Were you running a test from our test suite?

ethanalee · 2021-09-23T22:23:31Z

I'd like to investigate a little more. @ethanalee can you give me the exact command that you ran that generated the error? Were you running a test from our test suite?

Yeah sure, I was basically running the test_hello_world test in test_core.py from this PR: #15104

I added the @also_with_wasmfs decorator to test_hello_world

I ran this command: tests/runner test_hello_world

sbc100 · 2021-09-23T22:27:04Z

And that same test passed without #15104?

sbc100 · 2021-09-23T22:29:24Z

Its seems like any test that includes readAsmConstArgs should have triggered this issue, and I would have thought that was most tests. Any idea why your new test is different?

ethanalee · 2021-09-23T22:48:59Z

And that same test passed without #15104?

It passed before because I didn't add the new @also_with_wasmfs decorator

ethanalee · 2021-09-23T22:49:45Z

Its seems like any test that includes readAsmConstArgs should have triggered this issue, and I would have thought that was most tests. Any idea why your new test is different?

I am not too sure

sbc100 · 2021-09-23T23:39:43Z

I tried to reproduce this by checkout out #15104 and running ./tests/runner test_hello_world but its seems to work just fine.

I verified that it does contains the var double line:

$ grep "var double" /tmp/emscripten_test/test_hello_world.js
        var double = ch < 105;

But es-check does not complain:

$ node node_modules/.bin/es-check es5 /tmp/emscripten_test/test_hello_world.js
ES-Check: there were no ES version matching errors!  🎉
$ node node_modules/.bin/es-check --version
5.2.3

Can you check your es-check version?

ethanalee · 2021-09-23T23:44:41Z

$ node node_modules/.bin/es-check --version
5.2.4

Hmm, it's 5.2.4, it might be something on my end then or the version

sbc100 · 2021-09-24T00:07:07Z

Can you run node node_modules/.bin/es-check es5 test.js on a file containing var double; ? I can't get 5.2.3 or 5.2.4 to complain about it

curiousdannii · 2021-09-24T02:12:43Z

double isn't a reserved word in ES5. Es-check must have been run in ES3 mode? But if so wouldn't loads of other things fail too?

sbc100 · 2021-09-24T02:16:02Z

I can't get es-check to fail even when I pass es3 to it. Can you?

$ node node_modules/.bin/es-check es3 test.js
ES-Check: there were no ES version matching errors!  🎉

ethanalee · 2021-09-24T04:42:53Z

Hmm, the only thing I did differently was build with the -s WASMFS flag on the checkout of #15104

./emcc tests/core/test_hello_world.c -o test_hello_world.js -s WASMFS

Then node node_modules/.bin/es-check es5 test_hello_world.js

Output:

ES-Check Error:
          ----
          · erroring file: test_hello_world.js
          · error: SyntaxError: The keyword 'double' is reserved (1806:12)
          · see the printed err.stack below for context
          ----

double is reserved keyword

f7385e6

ethanalee requested review from kripken, sbc100 and tlively September 23, 2021 20:18

sbc100 reviewed Sep 23, 2021

View reviewed changes

sbc100 approved these changes Sep 23, 2021 •

edited

Loading

View reviewed changes

ethanalee enabled auto-merge (squash) September 23, 2021 21:09

ethanalee merged commit 552b4b8 into main Sep 23, 2021

ethanalee deleted the double-reserved-keyword branch September 23, 2021 21:10

ethanalee mentioned this pull request Sep 24, 2021

[minor] remove 'or readAsmConstArgsDouble' #15123

Merged

	def verify_es5(self, filename):
	es_check = shared.get_npm_cmd('es-check')

Uh oh!

Conversation

ethanalee commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

ethanalee Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

ethanalee Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

ethanalee Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

ethanalee Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kripken commented Sep 23, 2021

Uh oh!

ethanalee commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Sep 23, 2021

Uh oh!

ethanalee commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Sep 23, 2021

Uh oh!

sbc100 commented Sep 23, 2021

Uh oh!

ethanalee commented Sep 23, 2021

Uh oh!

ethanalee commented Sep 23, 2021

Uh oh!

sbc100 commented Sep 23, 2021

Uh oh!

ethanalee commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Sep 24, 2021

Uh oh!

curiousdannii commented Sep 24, 2021

Uh oh!

sbc100 commented Sep 24, 2021

Uh oh!

ethanalee commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ethanalee commented Sep 23, 2021 •

edited

Loading

ethanalee Sep 23, 2021 •

edited

Loading

ethanalee commented Sep 23, 2021 •

edited

Loading

ethanalee commented Sep 23, 2021 •

edited

Loading

ethanalee commented Sep 23, 2021 •

edited

Loading