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

doc: warn about reassigning _ in REPL #3729

Closed

Conversation

thefourtheye
Copy link
Contributor

When users assign a value to _ in REPL, it is prone to unexpected
results or messing up with REPL's internals. For example,
#3704. This patch issues a warning
about the same.

@thefourtheye thefourtheye added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Nov 10, 2015
@SamuelMarks
Copy link

@thefourtheye how about going one step further, and throwing an error or printing a warning when _ assignment is attempted?

Looks straightforward enough to do, just follow this pattern: repl.js#L384

@thefourtheye
Copy link
Contributor Author

@SamuelMarks The assignment to _ expression will be a string. You meant to say, parse the string and check if it is assigning something to _?

@Fishrock123
Copy link
Contributor

What is _ used for that this happens? (We could probably make it something else?)

@thefourtheye
Copy link
Contributor Author

@Fishrock123 Whenever you evaluate something in the REPL, the last evaluated expression's result is stored in _. For example,

> 1 + 1
2
> _ + 3
5
> _
5

If we assign something else to _, then it will mess up the further evaluations in REPL. For example,

> 1 + 1
2
> _
2
> _ = -2
-2
> _ + 2
0

Even worse, if we make _ a const, REPL will totally break, as it will try to assign the last evaluated expression's result in _. For example,

> const _ = 1;
TypeError: Cannot assign to read only property '_' of #<Object>
    at finish (repl.js:457:24)
    at REPLServer.defaultEval (repl.js:269:5)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:412:12)
    ...
> ;
TypeError: Cannot assign to read only property '_' of #<Object>
    at finish (repl.js:457:24)
    at REPLServer.defaultEval (repl.js:269:5)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:412:12)
    ...

_ is the commonly used name for this purpose I guess. For example, Python's REPL also uses _ only

Python 2.7.6 (default, Jun 22 2015, 17:58:13) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> 1 + 1
2
>>> _
2
>>> _ + 3
5
>>> _
5
>>> _ = -1
>>> _
-1

@Fishrock123
Copy link
Contributor

@thefourtheye Perhaps we can catch assigning to it while parsing and warn?

@thefourtheye
Copy link
Contributor Author

@Fishrock123 I think we can allow users to assign to it (I couldn't think of a case where assigning to it could be a very big trouble), but defining it as const as the first line, would totally break our REPL. What we can possibly do is, if user defines _ explicitly as const in REPL, we can stop storing the last value in _. Will that be okay?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2015

I think we should block the creation of a const _ (and show an appropriate message). That seems to be the only case that disrupts the REPL functionality.

@jasnell
Copy link
Member

jasnell commented Nov 10, 2015

If we pre-init _ to undefined then calling const _ = ought to result in an Identifier '_' has already been declared error.

@targos
Copy link
Member

targos commented Nov 10, 2015

👍 for @jasnell's suggestion

@thefourtheye
Copy link
Contributor Author

PR coming soon with @jasnell's awesome suggestion :-)

@@ -219,6 +219,9 @@ The special variable `_` (underscore) contains the result of the last expression
> _ += 1
4

*NOTE*: The `_` should not be reassigned explicitly in REPL, as it will either
mess up REPL's functionality or produce unexpected results.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps simply: *NOTE*: Explicitly assigning a value to_in the REPL can produce unexpected results.

When users assign a value to `_` in REPL, it is prone to unexpected
results or messing up with REPL's internals. For example,
nodejs#3704. This patch issues a warning
about the same.
@thefourtheye
Copy link
Contributor Author

@jasnell I updated the text as you suggested. Thanks :-) PTAL.

@jasnell
Copy link
Member

jasnell commented Nov 10, 2015

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2015

LGTM, but this should probably just be rolled into the same commit as #3737

@thefourtheye
Copy link
Contributor Author

@cjihrig You are correct. Moving these changes to #3737 and closing this.

@MylesBorins
Copy link
Contributor

removing from lts-watch in lieu of #3737

thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Feb 25, 2016
As `_` is not defined in REPL's context, when it is defined as `const`,
it breaks REPL, as it tries to store the result of the last evaluated
expression in `_`. This patch makes sure that `_` is pre-defined in
REPL's context, so that if users define it again, they will get error.

Refer: nodejs#3729
Refer: nodejs#3704
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Feb 25, 2016
If the `_` is redefined as `const` in REPL, it will break the REPL, as
REPL will store the result of the last evaluated expression in `_`.
This patch has a test to make sure that the REPL doesn't allow
redefining `_` as `const`, also still assiging values to `_` is
permitted.

Refer: nodejs#3729
Refer: nodejs#3704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants