Node: REPL crashes after let process;

[mik-jozef]

• Version: v6.0.0
• Platform: Windows 10 64bit
• Subsystem: REPL

Steps to replicate:

1. Open REPL
2. Type "let process;"

Node crashes instead of displaying something like TypeError: Identifier 'process' has already been declared.

[bnoordhuis]

let process is basically equivalent to process = undefined because it runs in the top-level (i.e., global) scope.

The REPL (and node in general) will let you do that but it won't end well. let Object or let Array have the same issue. I don't know if I'd say this is an actual bug, just mildly unexpected behavior.

[mik-jozef]

But why crash instead of displaying an error as if I typed "let x" two times?

[Fishrock123]

Interesting. var process; doesn't crash. Here's the stack:

> var process
undefined
> let process
undefined
domain.js:99
        process._emittingTopLevelDomainError = false;
                                             ^

TypeError: Cannot set property '_emittingTopLevelDomainError' of undefined
    at Domain.errorHandler [as _errorHandler] (domain.js:99:46)
    at process._fatalException (node.js:265:33)

[jasnell]

More interesting is that var process; does not actually redefine process

> var process;
undefined
> process;
{ process object output }

As @bnoordhuis indicates, I don't think this is an actual bug but it is rather odd.

[Fishrock123]

cc @nodejs/v8 probably

[bnoordhuis]

More interesting is that var process; does not actually redefine process

That's consistent with var semantics, it reuses the existing 'slot' if there is one.

The undefined return value that the REPL prints is because var process; is a statement that doesn't produce a value.

[addaleax]

Confirmed that this would be fixed by #5703 while going through the open PRs.

[lance]

This does not seem like a bug to me.

> let process;

This creates a process variable in the current lexical environment, shadowing the global process object. It's uninitialized, therefore undefined, but node depends on the process object, and now it's undefined. There is no way to recover from this.

@mik-jozef what would you suggest, display an error and then what? There's no way to continue with process now undefined. You could isolate this assignment in its own lexical environment, similar to this below, but there is no way at the top level to set process to something else and expect it to not be broken.

> function foo() {
... let process;
... console.log(process.nextTick);
... }
undefined
> console.log(process.nextTick);
[Function: nextTick]
undefined
> foo();
TypeError: Cannot read property 'nextTick' of undefined
    at foo (repl:3:20)
    at repl:1:1
    at REPLServer.defaultEval (repl.js:272:27)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:441:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:188:7)
    at REPLServer.Interface._onLine (readline.js:219:10)
    at REPLServer.Interface._line (readline.js:561:8)
>  console.log(process.nextTick)
[Function: nextTick]
undefined
>

As for var process, as @bnoordhuis said, this is how var works. From the ES2015 spec, "Within the scope of any VariableEnvironment a common BindingIdentifier may appear in more than one VariableDeclaration but those declarations collective define only one variable." So just typing var process doesn't do anything at all, really. Try typing var process = undefined and you'll find that the behavior is the same as that of let because now that common variable name has been redefined.

@addaleax I'm not sure how https://github.com/nodejs/node/pull/5703 would affect this. In fact, all of the examples I've shown in this comment were run on that branch. Can you help me understand what would be different here?

[cjihrig]

By not using the global context, you can define a variable named process without overwriting Node's global.

[lance]

@cjihrig ok that makes sense, of course. I'll take a look at the PR and write a test.

[bnoordhuis]

I'm closing this as not-a-bug for now, reopen if you feel that is mistaken. #5703, if/when it lands, should alleviate this gotcha.

[cjihrig]

5703 was just reverted, so I'm going to reopen this. @bnoordhuis (or anyone else), if you think this should remain closed, feel free.

[benjamingr]

I wonder, if we define process with a const binding in the process it should and not a var binding, basically, kind of like doing:

> const _p = process;
undefined
> const process = _p;
undefined
> let process
SyntaxError: Identifier 'process' has already been declared

What do you think about fixing this ad-hoc for process?

[gibfahn]

What do you think about fixing this ad-hoc for process?

I guess the only argument against doing that is that there might be a legitimate reason for someone to modify/overwrite the process object. IDK if there is one.

[TimothyGu]

@benjamingr Huh? Is that related to this issue?

[benjamingr]

@TimothyGu sorry - wrong PR :D

[benjamingr]

I guess the only argument against doing that is that there might be a legitimate reason for someone to modify/overwrite the process object. IDK if there is one.

The possibility kind of sways me from warning to fix this. Just like we don't guard against people modifying process or anything else inside Node - I don't think the REPL should be special - and while ES scoping rules are quirky here they _are_ ES scoping rules.

[gibfahn]

Maybe the repl could just print a warning if you change any of the built-in objects, e.g.:

Doing this overwrites a global object, which can destroy this REPL instance. I hope you know what you're doing!

We could also in theory have an Are you sure you want to do this? Enter/Ctrl+C prompt, but I think I'd be -1 on that, it'd be a pain, and reopening the repl isn't exactly a big problem.

I'm assuming the issue is people not realising that process is a built-in object, so printing a warning should be enough.

[lance]

My instinct is to consider this not a bug and leave it as-is. There is no way to recover if the user sets or undefines the global process object. It would be possible check for this assignment in the REPL code and recover somewhat gracefully by printing a warning, as you suggest @gibfahn. Naively, that would mean adding code to the REPL to parse each line looking for the assignment. But why should code in the REPL be treated differently than in a standard runtime? I don't think it's a good idea.

The only reasonable approach, IMO is to define global.process as a const at startup. The process object is created here and set in the environment through a series of macros. Unwinding all of that and treating process differently is perhaps beyond my pay grade at the moment. :)

I recommend closing this issue.

[addaleax]

@lance Thanks for commenting here – I think this might actually be easily fixed now that we have a different module wrapper for built-ins? PR to try it out @ https://github.com/nodejs/node/pull/17198