Skip to content

repl: Assignment of _ allowed with warning #5535

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.

Sign up for GitHub

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

Closed
lance wants to merge 6 commits into from
Closed

repl: Assignment of _ allowed with warning #5535

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6cc2713
repl: Assignment of _ allowed with warning
lance Mar 2, 2016
1dd5e2a
repl: Reset underscore assignment on .clear
lance Mar 3, 2016
ca199e2
doc: Document `_` behavior in REPL.
lance Mar 8, 2016
4062e62
repl: Adjust docs for `_` usage, and tighten test.
lance Mar 11, 2016
428cef6
repl: Test `_` behavior in `REPL_MODE_MAGIC` mode.
lance Mar 11, 2016
1471d13
repl: Change warning text to a complete sentence.
lance Mar 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
repl: Reset underscore assignment on .clear 
Ensures that, when invoking `.clear` in a repl, the underscore
assignment behavior is reset, so that its value is again set to the most
recently evaluated expression.

Additional tests have been added for the behavior of `let` when underscore
assignment has been disabled.
  • Loading branch information
@lance
lance committed Mar 15, 2016
commit 1dd5e2a1aa761a03f5874fe78c796f4ab1867401
30 changes: 16 additions & 14 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ function REPLServer(prompt,
self.useGlobal = !!useGlobal;
self.ignoreUndefined = !!ignoreUndefined;
self.replMode = replMode || exports.REPL_MODE_SLOPPY;
self.setLast = true;
self.underscoreAssigned = false;
self.last = undefined;

self._inTemplateLiteral = false;
Expand Down Expand Up @@ -473,7 +473,9 @@ function REPLServer(prompt,
// the second argument to this function is there, print it.
arguments.length === 2 &&
(!self.ignoreUndefined || ret !== undefined)) {
if (self.setLast) self.last = ret;
if (!self.underscoreAssigned) {
self.last = ret;
}
self.outputStream.write(self.writer(ret) + '\n');
}

Expand Down Expand Up @@ -547,22 +549,22 @@ REPLServer.prototype.createContext = function() {
context.module = module;
context.require = require;

const repl = this;

this.underscoreAssigned = false;
this.lines = [];
this.lines.level = [];

// make built-in modules available directly
// (loaded lazily)
exports._builtinLibs.forEach(function(name) {
exports._builtinLibs.forEach((name) => {
Object.defineProperty(context, name, {
get: function() {
get: () => {
var lib = require(name);
repl.last = context[name] = lib;
this.last = context[name] = lib;
return lib;
},
// allow the creation of other globals with this name
set: function(val) {
set: (val) => {
delete context[name];
context[name] = val;
},
Expand All @@ -572,14 +574,14 @@ REPLServer.prototype.createContext = function() {

Object.defineProperty(context, '_', {
configurable: true,
get: function() {
return repl.last;
get: () => {
return this.last;
},
set: function(value) {
repl.last = value;
if (repl.setLast) {
repl.setLast = false;
repl.outputStream.write('expression assignment to _ now disabled\n');
set: (value) => {
this.last = value;
if (!this.underscoreAssigned) {
this.underscoreAssigned = true;
this.outputStream.write('expression assignment to _ now disabled\n');
Copy link
Copy Markdown
Contributor

@silverwind silverwind Mar 14, 2016

Choose a reason for hiding this comment

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

I'd suggest capitalizing this print, it looks to be more in line with other things the REPL prints.

Copy link
Copy Markdown
Member Author

@lance lance Mar 15, 2016

Choose a reason for hiding this comment

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

As a complete sentence, with a period and everything? Or just start with the first word capitalized?

Copy link
Copy Markdown
Member

@jasnell jasnell Mar 15, 2016

Choose a reason for hiding this comment

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

+1 to making it a complete proper sentence.

Copy link
Copy Markdown
Contributor

@silverwind silverwind Mar 15, 2016

Choose a reason for hiding this comment

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

Maybe The assignment of the last expression's results to _ is now disabled.

}
}
});
Expand Down
136 changes: 108 additions & 28 deletions test/parallel/test-repl-underscore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,46 +4,126 @@ require('../common');
const assert = require('assert');
const repl = require('repl');
const stream = require('stream');
const inputStream = new stream.PassThrough();
const outputStream = new stream.PassThrough();
let output = '';

outputStream.on('data', (data) => {
output += data;
var tests = [
testSloppyMode,
testStrictMode,
testResetContext
];

tests.forEach(function(test) {
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig Mar 11, 2016

Choose a reason for hiding this comment

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

This seems unnecessary. Couldn't you just call the three test cases.

Copy link
Copy Markdown
Member Author

@lance lance Mar 11, 2016

Choose a reason for hiding this comment

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

Sure - I was just modeling this on other tests in the repo.

test();
});

process.on('exit', () => {
const lines = output.trim().split('\n');
function testSloppyMode() {
const r = initRepl(repl.REPL_MODE_SLOPPY);

// cannot use `let` in sloppy mode
r.write(`_; // initial value undefined
var x = 10; // evaluates to undefined
_; // still undefined
y = 10; // evaluates to 10
_; // 10 from last eval
_ = 20; // explicitly set to 20
_; // 20 from user input
_ = 30; // make sure we can set it twice and no prompt
_; // 30 from user input
y = 40; // make sure eval doesn't change _
_; // remains 30 from user input
`);

assert.deepStrictEqual(lines, [
assertOutput(r.output, [
'undefined',
'undefined',
'undefined',
'10',
'10',
'expression assignment to _ now disabled',
'20',
'20',
'30',
'20',
'40',
'30',
'40',
'40'
'30'
]);
});
}

const r = repl.start({
prompt: '',
input: inputStream,
output: outputStream
});
function testStrictMode() {
const r = initRepl(repl.REPL_MODE_STRICT);

r.write(`_; // initial value undefined
var x = 10; // evaluates to undefined
_; // still undefined
let _ = 20; // use 'let' only in strict mode - evals to undefined
_; // 20 from user input
_ = 30; // make sure we can set it twice and no prompt
_; // 30 from user input
var y = 40; // make sure eval doesn't change _
_; // remains 30 from user input
function f() { let _ = 50; } // undefined
f(); // undefined
_; // remains 30 from user input
`);

assertOutput(r.output, [
'undefined',
'undefined',
'undefined',
'undefined',
'20',
'30',
'30',
'undefined',
'30',
'undefined',
'undefined',
'30'
]);
Copy link
Copy Markdown
Contributor

@silverwind silverwind Mar 14, 2016

Choose a reason for hiding this comment

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

Care to explain why expression assignment to _ now disabled is not printed in strict/magic modes? I think I'm missing something.

Copy link
Copy Markdown
Member Author

@lance lance Mar 14, 2016

Choose a reason for hiding this comment

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

It's complicated in this case. The behavior of let is such that the setter on an object is not called in a scenario like this. Because of the way runInContext and runInThisContext work, the behavior is going to be something similar to this: https://gist.github.com/lance/2b2a6a8286b4bb52ddcf. If you run this script, the output is:

$ node test-repl-use-global.js
some value
some value
Setting 'val' a third value
a third value
Setting 'val' a fourth value
a fourth value

Notice that a "Setting 'val'..." message is not printed for when a let statement is used.

** EDIT **
I have updated the gist with a bunch of comments and examples showing what happens with let, this, and var. The behavior in the REPL is, I believe, consistent with what you should expect.

}

function testResetContext() {
const r = initRepl(repl.REPL_MODE_SLOPPY);

r.write(`_ = 10; // explicitly set to 10
_; // 10 from user input
.clear // Clearing context...
_; // remains 10
x = 20; // but behavior reverts to last eval
_; // expect 20
`);

assertOutput(r.output, [
'expression assignment to _ now disabled',
'10',
'10',
'Clearing context...',
'10',
'20',
'20'
]);
}

function initRepl(mode, useGlobal) {
Copy link
Copy Markdown
Contributor

@silverwind silverwind Mar 14, 2016

Choose a reason for hiding this comment

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

Why have this useGlobal argument if it's never used?

Copy link
Copy Markdown
Member Author

@lance lance Mar 15, 2016

Choose a reason for hiding this comment

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

At one point, I suspected useGlobal of altering behavior, so I was providing different values with different tests. This is just residual. I can remove it if necessary.

Copy link
Copy Markdown
Contributor

@silverwind silverwind Mar 15, 2016

Choose a reason for hiding this comment

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

Yes, I think it would be better left out. At some point we may want to lint for unused arguments like this.

const inputStream = new stream.PassThrough();
const outputStream = new stream.PassThrough();
outputStream.accum = '';

outputStream.on('data', (data) => {
outputStream.accum += data;
});

return repl.start({
input: inputStream,
output: outputStream,
useColors: false,
terminal: false,
prompt: '',
useGlobal: !!useGlobal,
replMode: mode
});
}

r.write('_;\n');
r.write('x = 10;\n');
r.write('_;\n');
r.write('_ = 20;\n');
r.write('_;\n');
r.write('y = 30;\n');
r.write('_;\n');
r.write('_ = 40;\n');
r.write('_;\n');
r.resetContext();
r.write('_;\n');
function assertOutput(output, expected) {
const lines = output.accum.trim().split('\n');
assert.deepStrictEqual(lines, expected);
}