repl: show lexically scoped vars in tab completion #16591

targos · 2017-10-29T17:26:58Z

Use the V8 inspector protocol to query the list of lexically scoped
variables (defined with let, const or class).

Fixes: #983

This is not ready to be merged. I still have to add tests and handle the case in which the inspector is already open.
Completion is working but there is one issue: every time tab is pressed, this message is printed to the console:

Debugger listening on ws://127.0.0.1:9229/uuid
For help see https://nodejs.org/en/docs/inspector

Can I prevent that from happening?

addaleax

This is really cool, thank you! I can look into a PR for the stderr message if you like

addaleax · 2017-10-29T20:04:42Z

lib/repl.js

@@ -58,6 +58,7 @@ const Module = require('module');
 const domain = require('domain');
 const debug = util.debuglog('repl');
 const errors = require('internal/errors');
+const inspector = require('inspector');


I guess this might be need to be guarded with something for --without-inspector?

addaleax · 2017-10-29T20:42:27Z

lib/repl.js

+  session.disconnect();
+  inspector.close();
+  return names;
+}


Wait – you can use the inspector to do synchronous exception of the running process? This is absolutely amazing?? 😍

Yes, it's cool. However in reality it's actually a released Zalgo:

In some cases, the callback is called synchronously.

In others (such as Runtime.awaitPromise or Runtime.evaluate with awaitPromise option specified) the callback is called asynchronously.

I guess either nobody thought of this problem when the API or added, or synchronous calling in those circumstances under which it's possible is too valuable to force asynchrony, that it remains this way.

or synchronous calling in those circumstances under which it's possible is too valuable to force asynchrony

I would go with that. ;)

Yeah, many thanks to @ofrobots who explained this to me at NINA!

TimothyGu

The conflict with existing connections is a blocker for me.

TimothyGu · 2017-10-29T21:00:04Z

lib/repl.js

@@ -778,6 +779,19 @@ function filteredOwnPropertyNames(obj) {
  return Object.getOwnPropertyNames(obj).filter(intFilter);
 }

+function getGlobalLexicalScopeNames() {
+  inspector.open();


inspector.open() opens the WebSocket server, and is not needed for inspector.Session.

Thanks, I didn't know you could use the inspector without the server xD

TimothyGu · 2017-10-29T21:02:26Z

lib/repl.js

+function getGlobalLexicalScopeNames() {
+  inspector.open();
+  const session = new inspector.Session();
+  session.connect();


This will conflict with any existing open connections, e.g. when the user is experimenting with the inspector.Session themself in the REPL. Last I heard @eugeneo was working on multi-connection Inspector support but even if it is now supported in V8, the Node.js part isn't ready yet.

> new (require('inspector').Session)().connect() undefined > new (require('inspector').Session)().connect() TypeError: Session is already attached at Session.connect (inspector.js:27:9) at repl:1:38

TimothyGu · 2017-10-29T21:08:06Z

lib/repl.js

+  session.disconnect();
+  inspector.close();
+  return names;
+}


Yes, it's cool. However in reality it's actually a released Zalgo:

In some cases, the callback is called synchronously.

In others (such as Runtime.awaitPromise or Runtime.evaluate with awaitPromise option specified) the callback is called asynchronously.

I guess either nobody thought of this problem when the API or added, or synchronous calling in those circumstances under which it's possible is too valuable to force asynchrony, that it remains this way.

targos · 2017-10-30T07:53:10Z

I can look into a PR for the stderr message if you like

@addaleax Thanks, but now that I don't start the server, there is no message :)

TimothyGu

An idea I've been toying around with is always have an internal reference to the currently open inspector.Session, so that we can internally use that for side-effect-free operations like this. Doesn't have to be in this PR though.

TimothyGu · 2017-10-30T08:08:01Z

lib/repl.js

+  try {
+    const session = new inspector.Session();
+    session.connect();
+    let names;


I'd initialize names to an empty array so that if the inspector callback is actually called asynchronously things won't crash and burn.

TimothyGu · 2017-10-30T08:08:08Z

lib/repl.js

+    session.connect();
+    let names;
+    session.post('Runtime.globalLexicalScopeNames', (error, result) => {
+      names = result.names;


Check if !error?

targos · 2017-10-30T12:01:26Z

I am having trouble writing the test (see 0980529 for current state).

I think the problem is that the test repl does not run in the main context, so Runtime.globalLexicalScopeNames returns an empty array. It is possible to optionally pass an ExecutionContextId to the command. I just have no idea how to get this id.

/cc @nodejs/v8-inspector

Fishrock123 · 2017-10-30T17:20:17Z

test/parallel/test-repl-tab-complete.js

@@ -24,6 +24,7 @@
 const common = require('../common');
 const assert = require('assert');
 const fixtures = require('../common/fixtures');
+const hasInspector = process.config.variables.v8_enable_inspector === 1;


This doesn't seem to be used?

I'm going to use it when I figure out how to make the test work

eugeneo · 2017-11-20T18:50:56Z

Soft non-LGTM.

Wouldn't this make it harder/impossible to use modules that rely on Inspector APIs in the REPL? Also, I wonder how common it is for the users to want to connect inspector to REPL node instance.

Please note that https://bugs.chromium.org/p/chromium/issues/detail?id=129539 is fixed now. I would prefer to give Chrome some time to detect/fix any related issues and then add concurrent inspector sessions support to Node, probably in Node10 timeframe.

@TimothyGu

An idea I've been toying around with is always have an internal reference to the currently open
inspector.Session, so that we can internally use that for side-effect-free operations like this. Doesn't
have to be in this PR though.

This would require cooperation from other API clients. They may rely on specific inspector session state (e.g. one client may rely on a breakpoint while another one might disable all the breakpoints for some time). Multi-session will be a cleaner solution.

TimothyGu · 2017-11-20T19:54:53Z

Multi-session will be a cleaner solution.

Certainly. Just wasn't sure about the timeline of such support in V8, and it could be a useful addition if such support in V8 will be a long time in coming.

targos · 2017-11-20T19:59:47Z

@eugeneo

Wouldn't this make it harder/impossible to use modules that rely on Inspector APIs in the REPL?

I am not exactly sure what your concern is. If an inspector session is already open, the completion should behave as it does now. If not, I connect just to get the names and disconnect immediately. The function is synchronous, so I don't see how it can affect other modules?

IMO this is better than nothing, while we wait for multi-session support.

Fishrock123 · 2017-11-23T21:05:16Z

cc @nodejs/v8-inspector again, any idea about #16591 (comment)?

TimothyGu · 2017-11-24T01:06:35Z

@targos You can get the ExecutionContextIds by listening for Runtime.executionContextCreated events, and then pass in Runtime.enable so that created events start coming in. You also get some extra information about the context in the form of ExecutionContextDescription, and you'll be able to filter out the main context by looking at the name field of the description. After that, Runtime.disable would stop new Runtime.executionContextCreated events from coming in.

However, if there are more than two contexts in play (i.e. one main, more than one REPL context), you would have to execute Runtime.enable first and wait until all the Runtime.executionContextCreated for existing contexts are gone, then wait for a final executionContextCreated event when the REPL context for which the ID is desired is created.

targos · 2017-11-24T15:15:13Z

@TimothyGu thank you, this was very informative!

I think I finally got it right.

CI: https://ci.nodejs.org/job/node-test-pull-request/11687/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1076/

BridgeAR

JS part LGTM

BridgeAR · 2017-12-10T01:40:31Z

@targos this needs a rebase. And would you be so kind and start a new CI? I guess the failed tests were not an issue but it would be good to be able to look at it again.

TimothyGu · 2017-12-10T02:40:36Z

lib/internal/util/inspector.js

+function sendInspectorCommand(cb, onError) {
+  if (!hasInspector) return onError();
+  try {
+    const session = new inspector.Session();


Instead of allocating a new Session on every attempt to send a command, you could allocate one Session per REPL instance to save GC some work. Just make sure to session.disconnect() at the end.

Doesn't that make it impossible to use the inspector's JS API from the REPL? Inspector sessions are like Highlander, there can be only one.

As long as it stays disconnected between invocations of this function, I don't think keeping a Session object around is a problem.

$ ./node > const xxx = 1 undefined > xxx # tab completion works 1 > const inspector = require('inspector') undefined > const session = new inspector.Session() undefined > session.connect() undefined > xx # tab completion does not work ReferenceError: xx is not defined > session.disconnect() undefined > xxx # tab completion works again 1 >

@TimothyGu I think one session instance per process is enough. I made that change.

@targos it would probably be good to add the example you provided as test (while I would not test for not having the completion while a session is open but that the user session works as expected).

TimothyGu · 2017-12-10T03:07:41Z

lib/repl.js

@@ -158,6 +160,7 @@ function REPLServer(prompt,
  self.last = undefined;
  self.breakEvalOnSigint = !!breakEvalOnSigint;
  self.editorMode = false;
+  self[kContextId] = undefined;


Would be helpful to clarify that this ID refers to the context ID in the inspector protocol,

targos · 2017-12-12T12:17:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12060/

targos · 2017-12-15T07:29:39Z

New ARM run: https://ci.nodejs.org/job/node-test-commit-arm-fanned/13405/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1116/

Original commit message: [inspector] added Runtime.globalLexicalScopeNames method The method returns names for all available top-level scope variables in giving context. R=dgozman@chromium.org,jgruber@chromium.org Bug: chromium:681333 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I2d0b600e1afbfef9087f53ea9c26abe1e112047c Reviewed-on: https://chromium-review.googlesource.com/719409 Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#48618} Refs: v8/v8@50f7455 PR-URL: #16591 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Use the V8 inspector protocol, if available, to query the list of lexically scoped variables (defined with `let`, `const` or `class`). PR-URL: #16591 Fixes: #983 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Original commit message: [inspector] added Runtime.globalLexicalScopeNames method The method returns names for all available top-level scope variables in giving context. R=dgozman@chromium.org,jgruber@chromium.org Bug: chromium:681333 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I2d0b600e1afbfef9087f53ea9c26abe1e112047c Reviewed-on: https://chromium-review.googlesource.com/719409 Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#48618} Refs: v8/v8@50f7455 PR-URL: #16591 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Use the V8 inspector protocol, if available, to query the list of lexically scoped variables (defined with `let`, `const` or `class`). PR-URL: #16591 Fixes: #983 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

MylesBorins · 2018-01-10T06:43:36Z

This is breaking arm64 odroid on 9.x

https://ci.nodejs.org/job/node-test-commit-arm/13011/nodes=ubuntu1604-arm64_odroid_c2/

g++: internal compiler error: Killed (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-5/README.Bugs> for instructions.
deps/v8/src/v8_base.target.mk:598: recipe for target '/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/v8_base/deps/v8/src/api.o' failed
make[2]: *** [/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/v8_base/deps/v8/src/api.o] Error 4
make[2]: *** Waiting for unfinished jobs....
rm fa4b5595818ee3c6b8e353e33594a7e9a94ec498.intermediate
Makefile:89: recipe for target 'node' failed
make[1]: *** [node] Error 2
Makefile:457: recipe for target 'build-ci' failed
make: *** [build-ci] Error 2
Build step 'Conditional steps (multiple)' marked build as failure
Run condition [Always] enabling perform for step [[]]
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
Jenkins Text Finder: File set '*.tap' is empty
Notifying upstream projects of job completion
Finished: FAILURE

I've opted to back it out of the v9.4.0

please backport if you can

Original commit message: [inspector] added Runtime.globalLexicalScopeNames method The method returns names for all available top-level scope variables in giving context. R=dgozman@chromium.org,jgruber@chromium.org Bug: chromium:681333 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I2d0b600e1afbfef9087f53ea9c26abe1e112047c Reviewed-on: https://chromium-review.googlesource.com/719409 Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#48618} Refs: v8/v8@50f7455 PR-URL: #16591 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Use the V8 inspector protocol, if available, to query the list of lexically scoped variables (defined with `let`, `const` or `class`). PR-URL: #16591 Fixes: #983 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

MylesBorins · 2018-01-10T08:14:22Z

Never mind... I don't think failure is related as https://ci.nodejs.org/job/node-test-commit-arm/13013/ passed with this Pr included. I've added it back to the release.

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

targos added wip Issues and PRs that are still a work in progress. repl Issues and PRs related to the REPL subsystem. labels Oct 29, 2017

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 29, 2017

targos removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 29, 2017

addaleax reviewed Oct 29, 2017

View reviewed changes

TimothyGu reviewed Oct 29, 2017

View reviewed changes

TimothyGu approved these changes Oct 30, 2017

View reviewed changes

Fishrock123 reviewed Oct 30, 2017

View reviewed changes

targos mentioned this pull request Nov 20, 2017

src: use executable + pid as inspector context id #17087

Merged

targos force-pushed the repl-lexical-scope branch 2 times, most recently from 53b40e5 to d02761d Compare November 24, 2017 14:51

targos removed the wip Issues and PRs that are still a work in progress. label Nov 24, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

BridgeAR approved these changes Dec 10, 2017

View reviewed changes

TimothyGu approved these changes Dec 10, 2017

View reviewed changes

targos force-pushed the repl-lexical-scope branch from d02761d to 63fa11c Compare December 12, 2017 12:14

targos added notable-change PRs with changes that should be highlighted in changelogs. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 15, 2017

targos force-pushed the repl-lexical-scope branch from 63fa11c to 2b91ac4 Compare December 19, 2017 07:23

targos deleted the repl-lexical-scope branch December 21, 2017 13:49

targos added dont-land-on-v4.x labels Dec 21, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the backport-requested-v9.x label Jan 10, 2018

targos removed the backport-requested-v9.x label Jan 15, 2018

repl: show lexically scoped vars in tab completion #16591

repl: show lexically scoped vars in tab completion #16591

Conversation

targos commented Oct 29, 2017 • edited Loading

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Oct 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Oct 30, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Nov 20, 2017

TimothyGu commented Nov 20, 2017

targos commented Nov 20, 2017

Fishrock123 commented Nov 23, 2017

TimothyGu commented Nov 24, 2017 • edited Loading

targos commented Nov 24, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Dec 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Dec 12, 2017

targos commented Dec 15, 2017

MylesBorins commented Jan 10, 2018 • edited Loading

MylesBorins commented Jan 10, 2018

targos commented Oct 29, 2017 •

edited

Loading

TimothyGu Oct 29, 2017 •

edited

Loading

TimothyGu commented Nov 24, 2017 •

edited

Loading

MylesBorins commented Jan 10, 2018 •

edited

Loading