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

repl: support previews by eager evaluating input #30811

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 5, 2019

This adds input previews by using the inspectors eager evaluation
functionality.
It is implemented as additional line that is not counted towards
the actual input. In case no colors are supported, it will be visible
as comment. Otherwise it's grey.
It will be triggered on any line change. It is heavily tested against
edge cases and adheres to "dumb" terminals (previews are deactived
in that case).

Small videos that outline the behavior:

https://asciinema.org/a/YwJ92D8vrj7bD5XGNGZzFhxU8
https://asciinema.org/a/bgalyUuG49w96RSzwfhYzLpXL

This is an alternative to #22875.

Fixes: #20977

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Dec 5, 2019
[ 'var ll = await Promise.resolve(2);', 'undefined' ],
[ 'll', ['// 2\r', '2'] ],
[ 'foo(await koo())',
[ 'f', '// 5oo', '// [Function: foo](await koo())\r', '4' ] ],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: this output looks strange but it's correct. As soon as f is entered, the preview is generated and printed. Right afterwards oo is entered and the preview for the next variable is printed.

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 5, 2019
@nodejs-github-bot

This comment has been minimized.

@mscdex
Copy link
Contributor

mscdex commented Dec 5, 2019

Can we have this be configurable in the repl API (other than relying on changing terminal)?

@jasnell
Copy link
Member

jasnell commented Dec 5, 2019

+1 to have this be configurable on the repl API. But I really like it being enabled by default for the default repl.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

I just updated the code and made it configurable. PTAL.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@@ -203,6 +204,9 @@ function REPLServer(prompt,
}
}

const preview = options.terminal &&
(options.preview !== undefined ? !!options.preview : true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this still prevent previews from being enabled if terminal is false-y? Why not only take .terminal into account if options.preview === undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. I would have liked to do that but the current proposal does not support previews for non-terminals yet. That requires additional work and I would rather do that in a follow-up PR.

@antsmartian
Copy link
Contributor

@BridgeAR While I was expecting few reviews for my PR (#22875), saw this PR :) Anyways, I would love to make the preview feature our own Repl. Looks like this PR handles more cases like color etc (which my PR doesn't), so happy to close my PR and get this PR land soon! Thanks.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

I just pushed some small fixes that improve the preview even further: it should now be able to print everything that has no side effects correctly and the preview is even going to stick around after a window resize.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 6, 2019
@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR added the review wanted PRs that need reviews. label Dec 6, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 7, 2019

lib/repl.js Show resolved Hide resolved
lib/internal/repl/utils.js Show resolved Hide resolved
This adds input previews by using the inspectors eager evaluation
functionality.
It is implemented as additional line that is not counted towards
the actual input. In case no colors are supported, it will be visible
as comment. Otherwise it's grey.
It will be triggered on any line change. It is heavily tested against
edge cases and adheres to "dumb" terminals (previews are deactived
in that case).

Fixes: nodejs#20977
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 8, 2019

Rebased due to conflicts.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR removed the review wanted PRs that need reviews. label Dec 9, 2019
BridgeAR added a commit that referenced this pull request Dec 9, 2019
This adds input previews by using the inspectors eager evaluation
functionality.
It is implemented as additional line that is not counted towards
the actual input. In case no colors are supported, it will be visible
as comment. Otherwise it's grey.
It will be triggered on any line change. It is heavily tested against
edge cases and adheres to "dumb" terminals (previews are deactived
in that case).

PR-URL: #30811
Fixes: #20977
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 9, 2019

Landed in 6bdf8d1 🎉

@BridgeAR BridgeAR closed this Dec 9, 2019
@BridgeAR BridgeAR mentioned this pull request Dec 9, 2019
4 tasks
targos pushed a commit that referenced this pull request Dec 10, 2019
This adds input previews by using the inspectors eager evaluation
functionality.
It is implemented as additional line that is not counted towards
the actual input. In case no colors are supported, it will be visible
as comment. Otherwise it's grey.
It will be triggered on any line change. It is heavily tested against
edge cases and adheres to "dumb" terminals (previews are deactived
in that case).

PR-URL: #30811
Fixes: #20977
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
MylesBorins added a commit that referenced this pull request Dec 16, 2019
This is a security release.

This release includes a single commit, an update to npm to 6.13.4.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  - update npm to 6.13.4
    #30904
  - update uvwasi (Anna Henningsen)
    #30745
  - upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  - docs deprecate http finished (Robert Nagy)
    #28679
* events:
  - add captureRejection option (Matteo Collina)
    #27867
* http:
  - add captureRejection support (Matteo Collina)
    #27867
  - llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  - implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  - implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  - support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  - add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  - implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  - expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  - add argv constructor option (legendecas)
    #30559

PR-URL: #30937
MylesBorins added a commit that referenced this pull request Dec 16, 2019
This is a security release.

This release includes a single commit, an update to npm to 6.13.4.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  * update npm to 6.13.4
    #30904
  * update uvwasi (Anna Henningsen)
    #30745
  * upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  * docs deprecate http finished (Robert Nagy)
    #28679
* events:
  * add captureRejection option (Matteo Collina)
    #27867
* http:
  * add captureRejection support (Matteo Collina)
    #27867
  * llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  * implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  * implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  * support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  * add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  * implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  * expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  * add argv constructor option (legendecas)
    #30559

PR-URL: #30937
MylesBorins added a commit that referenced this pull request Dec 16, 2019
This is a security release.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  * update npm to 6.13.4
    #30904
  * update uvwasi (Anna Henningsen)
    #30745
  * upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  * docs deprecate http finished (Robert Nagy)
    #28679
* events:
  * add captureRejection option (Matteo Collina)
    #27867
* http:
  * add captureRejection support (Matteo Collina)
    #27867
  * llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  * implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  * implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  * support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  * add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  * implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  * expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  * add argv constructor option (legendecas)
    #30559

PR-URL: #30937
MylesBorins added a commit that referenced this pull request Dec 17, 2019
This is a security release.

For more details about the vulnerability please consult the npm blog:

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

Notable Changes:
* deps:
  * update npm to 6.13.4
    #30904
  * update uvwasi (Anna Henningsen)
    #30745
  * upgrade to libuv 1.34.0 (Colin Ihrig)
    #30783
* doc:
  * docs deprecate http finished (Robert Nagy)
    #28679
* events:
  * add captureRejection option (Matteo Collina)
    #27867
* http:
  * add captureRejection support (Matteo Collina)
    #27867
  * llhttp opt-in insecure HTTP header parsing (Sam Roberts)
    #30567
* http2:
  * implement capture rection for 'request' and 'stream' events (Matteo Collina)
    #27867
* net:
  * implement capture rejections for 'connection' event (Matteo Collina)
    #27867
* repl:
  * support previews by eager evaluating input (Ruben Bridgewater)
    #30811
* stream:
  * add support for captureRejection option (Matteo Collina)
    #27867
* tls:
  * implement capture rejections for 'secureConnection' event (Matteo Collina)
    #27867
  * expose IETF name for current cipher suite (Sam Roberts)
    #30637
* worker:
  * add argv constructor option (legendecas)
    #30559

PR-URL: #30937
@targos targos added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Jan 14, 2020
@BridgeAR BridgeAR deleted the implement-repl-previews branch January 20, 2020 12:07
@targos targos removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This adds input previews by using the inspectors eager evaluation
functionality.
It is implemented as additional line that is not counted towards
the actual input. In case no colors are supported, it will be visible
as comment. Otherwise it's grey.
It will be triggered on any line change. It is heavily tested against
edge cases and adheres to "dumb" terminals (previews are deactived
in that case).

PR-URL: nodejs#30811
Fixes: nodejs#20977
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This adds input previews by using the inspectors eager evaluation
functionality.
It is implemented as additional line that is not counted towards
the actual input. In case no colors are supported, it will be visible
as comment. Otherwise it's grey.
It will be triggered on any line change. It is heavily tested against
edge cases and adheres to "dumb" terminals (previews are deactived
in that case).

PR-URL: #30811
Fixes: #20977
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@mvduin
Copy link

mvduin commented Sep 22, 2020

So ehm... how can I turn it off ?

When I'm typing a command in a repl I want to focus on what I'm typing and I find it very distracting to have text dancing around my input line. I've also had cases where it was eagerly evaluating fairly expensive computations and the repl started lagging, and I've had a case where I was editing a line that invoked a v8 native (which are rather intolerant to bad inputs) and eager evaluation resulted in a v8 crash.

@BridgeAR
Copy link
Member Author

@mvduin it is possible to pass through any options that you like when starting the REPL directly. It's possible to deactivate the preview mode by passing through preview: false as in: node -e 'repl.start({ preview: false })'.

Do you still remember the command that caused the crash? Any method that might cause a side effect is normally not triggered. If it is marked as side effect free while it's not, than that's a bug and we should fix it.

@mvduin
Copy link

mvduin commented Sep 23, 2020

That invocation worked but lacks persistent history. I tried node -e 'repl.start({ preview: false }).setupHistory( path.join( os.homedir(), ".node_repl_history" ) )' but it appears the callback is mandatory and I have no idea what it wants from me.

It would be nice if there were a simple way to set default options for the repl instance created when you invoke nodejs interactively, via a config file or environment variable. I did just come up with a gross but functional workaround: I set NODE_OPTIONS="-r $HOME/.node_defaults.js" where that file contains;

'use strict';
const repl = require('repl');
const orig_start = repl.start;
repl.start = options => orig_start({
	...options,
	preview: false,
});

And the problem wasn't side-effects, it's that all v8 natives use asserts rather than exceptions for type-checking their arguments. An example expression that crashes if typed with preview enabled is %HasFastPackedElements(1).

I also just realized another danger of preview: all of its evaluations still contribute to the optimizer statistics and it's not hard to imagine accidently calling things with abnormal types while editing a line and making a mess. This could be especially bad if you're trying to do benchmarks or analyze performance problems.

However, while trying to make an example to demonstrate that, I found a significantly more serious problem: start node --allow-natives-syntax (I tried both node v12 and v14) and copy-paste:

const foo = (x) => [ x, x, x, x ];
for( let i = 0; i < 100; i++ ) foo(42);
%OptimizeFunctionOnNextCall(foo)
let x = foo(42)  // x = prevents preview
x = foo(42)  // x = prevents preview
assert.equal( %GetOptimizationStatus(foo), 0b110001 );  // TurboFan optimized

Now type foo(42) to preview it and the optimization is instantly gone from foo and I have no clue why since it should have no reason for deopt. Using a debug build of node with --trace-deopt shows it indeed happening though only one line of the deopt log is visible, no doubt due to the state of the terminal when the deopt happens.

@BridgeAR
Copy link
Member Author

--allow-natives-syntax is not supported. Activating that is definitely going to cause issues. It should be fine to deactivate the preview if this flag is active during bootstrapping though.

@mvduin
Copy link

mvduin commented Sep 23, 2020

It is a very useful tool to better understand v8 and optimization.

The demonstration that preview of foo(42) is inexplicably triggering deoptimization of foo is a good example of that: instead of %OptimizeFunctionOnNextCall you can just call foo(42) enough times to trigger V8 to optimize it and instead of %GetOptimizationStatus you can use a performance measurement to notice deoptimization (although that will require replacing foo by something that's slow enough to be able to tell the difference) or use you can use a debug build of node with --trace-deopt. But using natives is just more convenient.

@bl-ue
Copy link
Contributor

bl-ue commented Mar 3, 2021

@BridgeAR this was a great feature, but it has a problem, see #37585.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement "eager evaluation" in the REPL
9 participants