nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.16k stars 29.37k forks source link

Lexical variable declarations (`let`, `const`) unusable in repl if error thrown during declaration/first assignment #8309

Open polybuildr opened 8 years ago

polybuildr commented 8 years ago
let s = Set();

gives TypeError: Constructor Set requires 'new'.

However, after this:

s

gives ReferenceError: s is not defined

and

let s = new Set();

gives TypeError: Identifier 's' has already been declared.

I'm not sure whether this is a real bug or it's expected behaviour, but it sure does seem unusual that s becomes unusable from this point in in the repl.

This isn't a problem when running node on a js file because the TypeError simply terminates execution.

vsemozhetbyt commented 8 years ago

Different with var:

> var  s = Set();
TypeError: Constructor Set requires 'new'
> s
undefined
> var  s = new Set();
undefined
> s
Set {}
polybuildr commented 8 years ago

Yep, works fine with var.

vkurchatkin commented 8 years ago

Here is simplified test case:

var vm = require('vm');

try {
  vm.runInThisContext('let x = f');
} catch (e) {}

vm.runInThisContext('x = 1');
addaleax commented 8 years ago

This is very déjà-vu-ish to me, although I can’t find another issue for this bug… /cc @nodejs/v8?

polybuildr commented 8 years ago

This error also occurs in the Chrome browser, so it's probably a v8 thing and not a Node thing. Sorry for reporting it in the wrong place.

polybuildr commented 8 years ago

Where should I report this considering that it's not specific to node's repl?

Fishrock123 commented 8 years ago

I wouldn't be surprised if this was how the spec worked.

In general, you'll have a better time using var for variables in the repl.

bnoordhuis commented 8 years ago

@polybuildr You can report it over at https://bugs.chromium.org/p/v8/issues/list. It looks spec compliant to me (the binding for s is never fully constructed and stays in the TDZ) but perhaps the error messages can be improved.

Slayer95 commented 8 years ago

This is per spec: https://esdiscuss.org/topic/take-let-variable-out-of-temporal-dead-zone

polybuildr commented 8 years ago

@bnoordhuis, @Slayer95, it does look spec-compliant, so I'm going to close this issue. However, that esdiscuss topic pointed me to something interesting that the Firefox Devtools console does and might be worth a shot.

For any such unresolved bindings (at least in the DevTools), Firefox steps in and turns them into undefined. So a redeclaration with let doesn't work, but simply s = 5 works from that point on. If anyone would like to consider doing this for the repl, feel free to reopen the issue.

MasterJames commented 6 years ago

Not working all variations

> rangeArray = nj.arange(6,12)
ReferenceError: rangeArray is not defined
> rangeArray = new nj.arange(6,12)
ReferenceError: rangeArray is not defined
> var rangeArray = new nj.arange(6,12)
SyntaxError: Identifier 'rangeArray' has already been declared
> let rangeArray = new nj.arange(6,12)
SyntaxError: Identifier 'rangeArray' has already been declared
> let rangeArray = nj.arange(6,12)
SyntaxError: Identifier 'rangeArray' has already been declared
> var rangeArray = nj.arange(6,12)
SyntaxError: Identifier 'rangeArray' has already been declared
gireeshpunathil commented 4 years ago

re-opening, per https://github.com/nodejs/node/issues/32288#issuecomment-599270508, https://github.com/nodejs/node/issues/32288#issuecomment-599300785 and https://github.com/nodejs/node/issues/32288#issuecomment-599333938

devsnek commented 4 years ago

This is fixable pending migration to v8 repl mode pending v8:10539

devsnek commented 3 years ago

Just trying to make this more searchable :innocent:

TheRamann commented 3 years ago

Wait, 5 year old issues are still open? 😨

TiraO commented 2 years ago

+1 this behavior is unpleasant. After making a typo, the only way to proceed is to start over if e.g. you were setting up to paste in a line of code.

mcollina commented 2 years ago

This is fixable pending migration to v8 repl mode pending v8:10539

@devsnek is this still blocked by that issue? It was closed as wontfix.

devsnek commented 2 years ago

@mcollina yeah we could implement repl using runtime.evaluate now, just no one has gotten around to it.

ShogunPanda commented 2 years ago

@devsnek So this is a full reimplementation of the repl?

devsnek commented 2 years ago

I don't think it requires complete reimplementation but it will definitely require some work

ShogunPanda commented 2 years ago

I see.

Once I get some bandwidth I might take a look at this. This looks like and interesting challenge.

gregfenton commented 1 year ago

nudge

ankit142 commented 8 months ago

Use the new keyword when creating a new Set instance:

let s = new Set();
s.add("OldestIssueOnNodeJS"); 
console.log(s);
gregfenton commented 8 months ago

@ankit142 The issue as posted isn't about fixing the code in the example provided. There are many examples that could be provided.

Essentially this issue is that calling let x = <some_code_that_throws_an_error> leads to a situation where you cannot use x and you cannot redeclare let x. So for the remainder of the REPL session, the symbol x is unusable.

ankit142 commented 8 months ago

To address this issue, you can manually reset the variable by using the delete keyword to remove it from the global scope.

let x;

try {
    x = <some_code_that_throws_an_error>;
} catch (error) {
    console.error('Error:', error);
    delete global.x; // Reset variable x
}

// Now you can safely redeclare x or use it again
x = <valid_code>;
gregfenton commented 8 months ago

Again, the issue is not with "fixing the code". It is a bad behaviour of the REPL. You don't know that the code you just typed will hit an error. It does. The symbol you tried to use (x) is now unusable for the remainder of the REPL.

This isn't about "writing good code" vs. "writing bad code". This is about the user experience using the REPL when playing around with code.

RedYetiDev commented 4 months ago

FWIW @1Git2Clone has a issue with chromium at https://issues.chromium.org/issues/345248266.