Chakracore: Adding small REPL for development purpose

[navidR]

I am trying to read and understand ChakraCore architecture. In process of doing that, when I want to change or fix small part of ChakraCore runtime, I have to run tests or something, which is unnecessary for most of the time.

I am developing small REPL inside of ChakraCore project which could be helpful to many people (I think) so every time I am trying something new or messing with something I can run and test it simply inside Visual Studio without going through all the pain of testing and other stuff. It is for my own usage right now, but we can work on it to make it something better and ultimately adding it to main repo.

What is your opinion to add small REPL like subproject (or it could be "-repl" flag for ch.exe) ?

[dilijev]

I talked about adding this before, but since it's easy enough to invoke ch.exe test.js most people didn't view this as valuable. I, for one, would welcome a ch.exe -repl mode, or even simply something like ch.exe -e "console.log(3+5)" (where -e stands for execute).

Also keep in mind that https://github.com/nodejs/node-chakracore effectively provides a ChakraCore repl (+ Node.js extensions), so probably the more valuable option would be the -e flag I mentioned to allow for quick execution of one-off scripts.

I got the idea of -e from sed -e but there is also python -c, cmd /c, powershell -Command etc. Really any reasonable flag name that doesn't conflict with existing flag names is fine :)

[navidR]

That -e is nice.So simple to implement too.

About node-chakra , that idea crossed my mind too, but that doesn't improve the cycle of dev/compile/run because after compiling ChakraCore you should every time embed the newly compiled chakracore inside nodejs (unless I am missing something) so nodejs-chakra would not improve situation in this specific case.

[obastemur]

ChakraCore you should every time embed the newly compiled chakracore inside nodejs (unless I am missing something) so nodejs-chakra would not improve situation in this specific case.

At this very moment, there are some minor difference between the node-chakracore and chakracore repos. (xplat API differences) but we will have them sorted out very soon. Once they share the same code base, you might clone your chakracore fork into node-chakracore, play with it + make changes and compile node-chakracore. There you have your changes in place.

Yet, -e and -pe options sounds like a good improvement though.

ch -pe "1.245.toFixed(2)"

or similarly;

ch -e "print(1.245.toFixed(2)"

Honestly, I find -pe option is similarly useful.

[dilijev]

@navidR are you planning to implement this? I'll assign it to you for now :)

[navidR]

@dilijev yes, but it may take a little while (because of my exams). Thanks

[dilijev]

@navidR Just checking. Good luck with exams. Thanks!

[mathiasbynens]

IMHO -e and REPL are two different features. One does not imply that the other isn’t needed anymore. I’d like to have both!

Ideally ch would support piping as well, e.g. echo 'print(1+2)' | ch.

Note that the --repl flag doesn’t seem necessary — ch could default to the REPL when invoked without getting a file passed to it.

[dilijev]

@mathiasbynens - Agreed. On the issue of time investment, though, the -e flag would be minimal effort and add a lot of value, whereas the REPL will take a much more significant engineering effort. We would still very much welcome PRs for that, though!

[navidR]

Sorry for the delay, I was so busy in recent months, but in coming week I have quite free time to devote completely to ChakraCore.

I do have a personal implementation for "-e", let me polish it a little bit, I will submit a patch. I think pipe idea is very good too. Command line argument parser tries to separate arguments based on white space, which does make the problem for passing js code to the chakra engine. But I did find a way to make it work.

I will polish it a little bit in next week and will submit a small patch to multiple files.
I think I can add pipe with little effort too, but I have to look at Windows documentation, but I am not 100% sure about how the pipe is implemented on Windows or it is different the way we use in Linux or not.

But repl do need a little bit restructuring of code in Ch. Ch code kind of designed to read files, for adding -e i did hack where ch read files and change it to read it from the string which I do get it from the parser. But for REPL I think there should be better way and with current code base it does need a little bit of restructuring.

[dilijev]

@navidR p1 | p2 sends stdout of p1 to stdin of p2. In order to get piping to ch to work, you would just have to implement handling for stdin. stdin processing could take an entire script followed by EOT (^D / \x04) or EOF and basically behave like -e, OR (possibly later) behave like a REPL except not to display a prompt until stdin is empty of all input lines (thus effectively behaving the same as before, with an interactive option). This is basically how most programs with a REPL seem to behave anyway.

[dilijev]

Hi @navidR - Gentle reminder, and just wondering, have you made any progress on this?

[navidR]

Sorry for the delay. Yes. I did implement it but I only could add this kind of flag (after a little bit of tweaking in ConsoleFlagParser ) to ch :

ch.exe -eval:"Console.Write('Hello World')"

What do you think ? Do you think this kind of flag is good enough ?
The reason I hesitate is that personally I prefer to have standard unix style flag, and to have that we should completely redesign flag part of the ch. But ch only have this kind of flag. (those other flags you see when you type help, all redirected to ChakraCore itself).

I haven't spare time to add pipe to ch.exe, yet.

[dilijev]

@navidR Do you mean you couldn't add a -e or you couldn't add --eval (double-hyphen prefix)? Or, do you mean you needed to use the colon instead of parsing the next parameter as a string that belongs to the -eval flag?

We don't explicitly support Unix-style short flags (in the sense that you cannot chain flags under a single - like in rm -rf which is a shorthand for rm -r -f. We therefore also don't make a distinction between single-character and multiple-character flags. However, for every supported flag, IIRC we allow any of the prefixes / (Windows-only), -, or --. The first is for backwards compat on Windows but we prefer not to use it (it's not supported on xplat anyway). By convention we prefer - over -- for brevity and general consistency.

You're right that most flags are passed through to ChakraCore itself. However, you can add logic directly to ch to parse flags ahead of time for ch-specific behavior like this. See the implementation of ch -version for example.

Or were you talking about needing to use a colon to associate the parameter with the flag? I think you could do something like reading the next parameter if you detect the flag -e or -eval and then advancing by 2 for the next parameters. I believe we already do this in a few places.

I haven't spare time to add pipe to ch.exe, yet.

Don't worry about pipes or REPL yet (those are a relatively taller order than a simple -eval flag. Let's add one feature at a time. Let's isolate a change and review a PR for the -eval flag and go from there.

[navidR]

You're right that most flags are passed through to ChakraCore itself. However, you can add logic directly to ch to parse flags ahead of time for ch-specific behavior like this. See the implementation of ch -version for example.

I knew, the problem with this approach is we cannot send flags to ChakraCore (guest) engine itself while we using it in eval mode, I want to be able :

./ch -eval="WScript.Echo('Hello Chakra')" --flags-for-chakracore --flags-for-chakracore

While in "version", "help" the writer knows after printing the message they will exit. Because of that they happily add simple if at the main.

I have added eval flag to host flags in Ch (host). It works for this syntax right now without problem :

./ch -eval:"WScript.Echo('Hello Chakra')" --flags-for-chakracore --flags-for-chakracore

I am trying to find a way to make it work for

./ch -[eval|e]="WScript.Echo('Hello Chakra')" --flags-for-chakracore --flags-for-chakracore

too.

BTW I tried hard to only use existing code instead of adding haphazard flag handling, and Ch's existing flag handling only handles long flags like "-eval" (no way to include two version for the same flag without complete modification to HostConfigFlag class) . What do you think? do you think it is enough to have only one of "-eval" or "-e" or I should find a new workaround?

This is non-cleaned up version and I had better version (but I couldn't find it right now, but this is enough for you to get the idea), so if you wanna use it, use it with caution:

diff --git a/bin/ch/HostConfigFlagsList.h b/bin/ch/HostConfigFlagsList.h
index 3f73f2b..ae125e7 100644
--- a/bin/ch/HostConfigFlagsList.h
+++ b/bin/ch/HostConfigFlagsList.h
@@ -11,5 +11,6 @@ FLAG(int,  InspectMaxStringLength,          "Max string length to dump in locals
 FLAG(BSTR, Serialized,                      "If source is UTF8, deserializes from bytecode file", NULL)
 FLAG(bool, OOPJIT,                          "Run JIT in a separate process", false)
 FLAG(bool, EnsureCloseJITServer,            "JIT process will be force closed when ch is terminated", true)
+FLAG(BSTR, eval,                            "Evaluate javascript code from command line", NULL)
 #undef FLAG
 #endif
diff --git a/bin/ch/ch.cpp b/bin/ch/ch.cpp
index 0e4abdd..3ee1d21 100644
--- a/bin/ch/ch.cpp
+++ b/bin/ch/ch.cpp
@@ -312,7 +312,7 @@ HRESULT RunScript(const char* fileName, LPCSTR fileContents, JsFinalizeCallback

     IfJsErrorFailLogLabel(ChakraRTInterface::JsSetPromiseContinuationCallback(WScriptJsrt::PromiseContinuationCallback, (void*)messageQueue), ErrorRunFinalize);

-    if(strlen(fileName) >= 14 && strcmp(fileName + strlen(fileName) - 14, "ttdSentinal.js") == 0)
+    if(!HostConfigFlags::flags.evalIsEnabled && strlen(fileName) >= 14 && strcmp(fileName + strlen(fileName) - 14, "ttdSentinal.js") == 0)
     {
         if(fileContentsFinalizeCallback != nullptr)
         {
@@ -595,7 +595,7 @@ HRESULT ExecuteTest(const char* fileName)
     JsRuntimeHandle runtime = JS_INVALID_RUNTIME_HANDLE;
     UINT lengthBytes = 0;

-    if(strlen(fileName) >= 14 && strcmp(fileName + strlen(fileName) - 14, "ttdSentinal.js") == 0)
+    if(!HostConfigFlags::flags.evalIsEnabled && strlen(fileName) >= 14 && strcmp(fileName + strlen(fileName) - 14, "ttdSentinal.js") == 0)
     {
 #if !ENABLE_TTD
         wprintf(_u("Sentinel js file is only ok when in TTDebug mode!!!\n"));
@@ -631,8 +631,19 @@ HRESULT ExecuteTest(const char* fileName)
         char fullPath[_MAX_PATH];
         size_t len = 0;

-        hr = Helpers::LoadScriptFromFile(fileName, fileContents, &lengthBytes);
-        contentsRaw; lengthBytes; // Unused for now.
+        if(HostConfigFlags::flags.evalIsEnabled)
+        {
+            // Debug
+            wprintf(_u("eval is enabled \n"));
+            wprintf(_u("%s\n"), HostConfigFlags::flags.eval);
+            // Debug
+            hr = WideStringToNarrowDynamic(HostConfigFlags::flags.eval, const_cast<LPSTR *>(&fileContents));
+        }
+        else
+        {
+            hr = Helpers::LoadScriptFromFile(fileName, fileContents, &lengthBytes);
+            contentsRaw; lengthBytes; // Unused for now.
+        }

         IfFailGo(hr);
         if (HostConfigFlags::flags.GenerateLibraryByteCodeHeaderIsEnabled)
@@ -706,17 +717,28 @@ HRESULT ExecuteTest(const char* fileName)
             IfFailGo(E_FAIL);
         }

-        if (_fullpath(fullPath, fileName, _MAX_PATH) == nullptr)
+        if (!HostConfigFlags::flags.evalIsEnabled)
         {
-            IfFailGo(E_FAIL);
-        }
+            if(_fullpath(fullPath, fileName, _MAX_PATH) == nullptr)
+            {
+                IfFailGo(E_FAIL);
+            }

-        // canonicalize that path name to lower case for the profile storage
-        // REVIEW: Assuming no utf8 characters here
-        len = strlen(fullPath);
-        for (size_t i = 0; i < len; i++)
+            // canonicalize that path name to lower case for the profile storage
+            // REVIEW: Assuming no utf8 characters here
+            len = strlen(fullPath);
+            for (size_t i = 0; i < len; i++)
+            {
+                fullPath[i] = (char)tolower(fullPath[i]);
+            }
+        }
+        else 
         {
-            fullPath[i] = (char)tolower(fullPath[i]);
+            len = strlen(fullPath);
+            for (size_t i = 0; i < len; i++)
+            {
+                fullPath[i] = (char)NULL;
+            }
         }

         if (HostConfigFlags::flags.GenerateLibraryByteCodeHeaderIsEnabled)
@@ -739,6 +761,10 @@ HRESULT ExecuteTest(const char* fileName)
         {
             CreateAndRunSerializedScript(fileName, fileContents, WScriptJsrt::FinalizeFree, fullPath);
         }
+        else if (HostConfigFlags::flags.evalIsEnabled)
+        {
+            IfFailGo(RunScript(NULL, fileContents, WScriptJsrt::FinalizeFree, nullptr, fullPath));
+        }
         else
         {
             IfFailGo(RunScript(fileName, fileContents, WScriptJsrt::FinalizeFree, nullptr, fullPath));
@@ -1043,7 +1069,7 @@ int _cdecl wmain(int argc, __in_ecount(argc) LPWSTR argv[])
     OnChakraCoreLoaded(OnChakraCoreLoadedEntry);
 #endif

-    if (argInfo.filename == nullptr)
+    if (!HostConfigFlags::flags.evalIsEnabled && argInfo.filename == nullptr)
     {
         WideStringToNarrowDynamic(argv[1], &argInfo.filename);
     }
diff --git a/lib/Common/Core/CmdParser.h b/lib/Common/Core/CmdParser.h
index 9ce8a80..a7cbe50 100644
--- a/lib/Common/Core/CmdParser.h
+++ b/lib/Common/Core/CmdParser.h
@@ -26,7 +26,7 @@ class CmdLineArgsParser : private ICmdLineArgsParser
 {
 // Data
 private:
-    static const int  MaxTokenSize  = 512;
+    static const int  MaxTokenSize  = 8192;

     Js::ConfigFlagsTable& flagTable;
     LPWSTR           pszCurrentArg;

Any feedback, advice, suggestion greatly appreciated.

[dilijev]

@navidR Can you push that change to a branch on your fork and share a link to the branch so we can more easily build and evaluate? (You don't need to create a PR to share a branch.)

As for alternatives of the same flag, you could do something like having an alternative boolean for each one, have the presence of either set the same boolean, etc.

IMO --eval doesn't belong in the HostConfigFlagList, but rather it belongs to the same area of code as the --version etc. It's possible others will disagree, however I think that will be the easiest way to do this.

I would make the ([-]-eval|-e) flag handled in ch itself, using the same code pattern that parses out ([-]-version|-v), load the script string instead of loading a script file (which is also done by ch), and then ch passes all remaining flags to the underlying engine via the HostConfigFlags path you're extending. I'm not really sure I understand the problem you're encountering.

[dilijev]

FWIW there's a tool called eshost-cli that might serve the purpose of the --eval you're trying to add here (also doesn't have a REPL). https://gist.github.com/dilijev/d8f2fd58551360409b2ba3f50af144fd

That said I still think it would be good to have this functionality built into the ch host we publish on our end.

[navidR]

https://github.com/navidR/ChakraCore/tree/eval
e.g.

[navid@workstation ChakraCore]$ ./out/Debug/ch -eval:"function hello(){var regex = /blah/;WScript.Echo('blah: ' + regex.blah);regex.blah = 1;WScript.Echo('blah: ' + regex.blah);}
hello();hello();"
function hello(){var regex = /blah/;WScript.Echo('blah: ' + regex.blah);regex.blah = 1;WScript.Echo('blah: ' + regex.blah);}hello();hello();
blah: undefined
blah: 1
blah: undefined
blah: 1
[navid@workstation ChakraCore]$ cat test/Object/regex.baseline
blah: undefined
blah: 1
blah: undefined
blah: 1
[navid@workstation ChakraCore]$

IMO --eval doesn't belong in the HostConfigFlagList, but rather it belongs to the same area of code as the --version etc. It's possible others will disagree, however I think that will be the easiest way to do this.

I disagree, I think "-version" and other stuff should have used Ch existed class for flag handling.

[dilijev]

I disagree, I think "-version" and other stuff should have used Ch existed class for flag handling.

@navidR You're probably right that this change belongs there, although as you've seen it doesn't support flag aliases. As for --version the reason we couldn't put it there is because we wanted it to be available in release builds as well without having to refactor the existing class (which is debug/test only) to be enabled and to make a distinction between test and release flags.

[853419196]

In order to facilitate the development, I wrote a REPL compatible with ChakraCore in JavaScript.
https://github.com/853419196/JavaScript_REPL