repl: fix declaring a variable with the name util #38141

EladKeyshawn · 2021-04-07T23:37:42Z

Fixes: #38139

lib/internal/repl/utils.js

EladKeyshawn · 2021-04-07T23:46:14Z

From issue #38139

aduh95

Hi! Thanks for sending the PR. Would it be possible to add tests to make sure this is fixing the issue?

lib/internal/repl/utils.js

EladKeyshawn · 2021-04-08T08:48:49Z

@aduh95 I'll try to do it thanks

EladKeyshawn · 2021-04-09T08:02:02Z

Hi! Thanks for sending the PR. Would it be possible to add tests to make sure this is fixing the issue?

How would you suggest to test this? run the same commands and check no error is printed to screen OR do more of unit test to the specific preview function?

aduh95 · 2021-04-09T10:58:31Z

You can add a test case in test/parallel/test-repl-history-navigation.js:

  {
    env: { NODE_REPL_HISTORY: defaultHistoryPath },
    test: [ 'const util = {}', ENTER,
            'ut', RIGHT, ENTER],
    expected: [],
    clean: false
  },

You need to also add a line to make sure the warning is gone at the top of the file:

process.on('warning', common.mustNotCall());

Make sure this fails on current Node.js version, and passes with your PR:

$ ./tools/test.py test/parallel/test-repl-history-navigation.js
[00:00|% 100|+   1|-   0]: Done
$ node test/parallel/test-repl-history-navigation.js || echo 'All good, we expect this to fail'
All good, we expect this to fail

EladKeyshawn · 2021-04-09T12:59:44Z

You can add a test case in test/parallel/test-repl-history-navigation.js:

  {
    env: { NODE_REPL_HISTORY: defaultHistoryPath },
    test: [ 'const util = {}', ENTER,
            'ut', RIGHT, ENTER],
    expected: [],
    clean: false
  },

You need to also add a line to make sure the warning is gone at the top of the file:

process.on('warning', common.mustNotCall());

Make sure this fails on current Node.js version, and passes with your PR:

$ ./tools/test.py test/parallel/test-repl-history-navigation.js
[00:00|% 100|+   1|-   0]: Done
$ node test/parallel/test-repl-history-navigation.js || echo 'All good, we expect this to fail'
All good, we expect this to fail

@aduh95 Done! Thanks for the guidance 😄

test/parallel/test-repl-history-navigation.js

benjamingr · 2021-04-10T15:29:24Z

I... don't understand this fix?

Doesn't this just fix the error being shown (cannot read property length) instead of the underlying cause (the repl assuming util is available in the global namespace)?

I would strongly prefer a fix that fixes the underlying issue and not the "cosmetic" one. Namely changing code doing something like:

session.post('Runtime.callFunctionOn', {
            functionDeclaration: `(v) => util.inspect(v, ${inspectOptions})`,
            objectId: result.objectId,
            arguments: [result]
          }

To not rely on the global util (either by requireing it, "hiding" it as a non-completed global, or passing it to Runtime.evaluate)

benjamingr

Requesting changes to make sure this doesn't land before everyone is on the same page regarding the kind of fix this needs @nodejs/repl

(Thank you for working on this @EladKeyshawn !)

Linkgoron · 2021-04-10T15:53:02Z

To not rely on the global util (either by requireing it, "hiding" it as a non-completed global, or passing it to Runtime.evaluate)

I think it makes sense to change that relevant code from util.inspect to require('util').inspect at least (in addition to the undefined fix, so that doing const require = {} won't break the code). The change would change the "bad" word to require, but I think that it's much less likely to be used.

test/parallel/test-repl-history-navigation.js

aduh95 · 2021-04-10T17:43:53Z

I think it makes sense to change that relevant code from util.inspect to require('util').inspect at least (in addition to the undefined fix, so that doing const require = {} won't break the code). The change would change the "bad" word to require, but I think that it's much less likely to be used.

~~That shouldn't be an issue, require is not on the global scope.~~ actually it is, the function still inherits from the REPL scope.

aduh95 · 2021-04-10T21:38:03Z

@benjamingr I arrive at the same conclusion as @Linkgoron, we need to rely on either util or require being unchanged –unless there's a way to use dynamic import? Maybe what we can do is to use (v) => Reflect.getOwnPropertyDescriptor(globalThis, 'util').get().inspect(v, ${inspectOptions}), which would fail only if the global util property is re-defined, and wouldn't be "exploitable" unless the user goes all the way of defining a credible mock:

> const util = {}
undefined
> util
{}
> globalThis.util = { inspect() { return console.log(arguments) } }
{ inspect: [Function: inspect] }
> util
{}
> Reflect.defineProperty(globalThis, 'util', { get() { return {inspect(){ console.log(arguments) }}}})
true
> ut[Arguments] {
  '0': {},
  '1': {
    showHidden: false,
    depth: 1,
    colors: false,
    customInspect: true,
    showProxy: true,
    maxArrayLength: 100,
    maxStringLength: 10000,
    breakLength: Infinity,
    compact: true,
    sorted: false,
    getters: false
  }
}

aduh95 · 2021-04-11T08:57:05Z

@EladKeyshawn can you please update the code at

node/lib/internal/repl/utils.js

Lines 353 to 357 in 7216eb6

    
           session.post('Runtime.callFunctionOn', { 
        
             functionDeclaration: `(v) => util.inspect(v, ${inspectOptions})`, 
        
             objectId: result.objectId, 
        
             arguments: [result] 
        
           }, (error, preview) => {

with my above suggestion? It would be nice to also add a test case fro when globalThis.util is re-assigned.

EladKeyshawn · 2021-04-11T09:10:40Z

@EladKeyshawn can you please update the code at

node/lib/internal/repl/utils.js

Lines 353 to 357 in 7216eb6

session.post('Runtime.callFunctionOn', {

functionDeclaration: `(v) => util.inspect(v, ${inspectOptions})`,

objectId: result.objectId,

arguments: [result]

}, (error, preview) => {

with my above suggestion? It would be nice to also add a test case fro when globalThis.util is re-assigned.

No problem :) I'm on it

benjamingr

LGTM with green CI

EladKeyshawn · 2021-04-11T12:42:53Z

with my above suggestion? It would be nice to also add a test case fro when globalThis.util is re-assigned.

If globalThis is reassigned we're back with the same problem. Do we count on the fact that it's unlikely to happen? anyways my original patch solves the Cannot read property 'length' of undefined warning in this case anyways.

EladKeyshawn · 2021-04-11T13:19:07Z

@aduh95 Can you explain why the last tests pass the expected output but are not marked as called, I believe obviously repl.once('closed'... is not triggered but is that normal?

test/parallel/test-repl-history-navigation.js

aduh95 · 2021-04-11T17:12:13Z

If globalThis is reassigned we're back with the same problem. Do we count on the fact that it's unlikely to happen?

Right, I think it's fine to fail when globalThis is overwritten, I don't think there is a way around it. I'd say it's possibly less likely to happen than util.

Can you explain why the last tests pass the expected output but are not marked as called, I believe obviously repl.once('closed'... is not triggered but is that normal?

I'm not sure I understand what you mean, I'd expect the closed event to be emitted for each test case. What make you think it's not triggered?

test/parallel/test-repl-history-navigation.js

EladKeyshawn · 2021-04-15T06:09:14Z

@benjamingr @aduh95 What's blocking this?

nodejs-github-bot · 2021-04-16T14:18:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37515/

nodejs-github-bot · 2021-04-17T13:30:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37532/

aduh95 · 2021-04-17T13:41:58Z

test-repl-history-navigation is consistently failing on ubuntu1804_sharedlibs_withoutintl_x64 and ubuntu1804_sharedlibs_withoutssl_x64 jobs.

EladKeyshawn · 2021-04-17T17:07:37Z

test-repl-history-navigation is consistently failing on ubuntu1804_sharedlibs_withoutintl_x64 and ubuntu1804_sharedlibs_withoutssl_x64 jobs.

Do you have any idea what could cause this? for me on OSx it works.

aduh95

One way of working around this would be to disable the output checks when Intl or crypto is missing.

aduh95 · 2021-04-17T17:21:44Z

test/parallel/test-repl-history-navigation.js

+    env: { NODE_REPL_HISTORY: defaultHistoryPath },
+    test: ['const util = {}', ENTER,
+           'ut', RIGHT, ENTER],
+    expected: [


Suggested change

expected: [

expected: common.hasIntl && common.hasCrypto ? [

Can I ask why do these requirements affect the console output? :)

No clue! If you'd like to investigate that, you can compile node with ./configure --without-intl on your local machine.

test/parallel/test-repl-history-navigation.js

nodejs-github-bot · 2021-04-17T21:47:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37547/

PR-URL: #38141 Fixes: #38139 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

benjamingr · 2021-04-18T13:58:21Z

Landed in b2baa3d 🎉

benjamingr · 2021-04-18T14:07:57Z

I had to revert because of lint failures. I will land this manually.

aduh95 · 2021-04-18T14:11:54Z

@benjamingr I have this ready to land on my local machine, do you want me to push?

benjamingr · 2021-04-18T14:12:25Z

@aduh95 yes please, thanks :) I want to investigate what's went wrong and I don't want to hold this PR up just for my root-cause check

The REPL no longer relies on `util` being a reference to the `util` core module. It still relies on `globalThis` refering to the global object, but no longer emits warnings when it's overwritten by the user. PR-URL: nodejs#38141 Fixes: nodejs#38139 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

aduh95 · 2021-04-18T14:16:13Z

Landed in d666964 :)

EladKeyshawn · 2021-04-18T16:59:12Z

Thanks 😊

nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Apr 7, 2021

EladKeyshawn mentioned this pull request Apr 7, 2021

The Node REPL can't handle declaring a variable with the name util #38139

Closed

EladKeyshawn commented Apr 7, 2021

View reviewed changes

lib/internal/repl/utils.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 8, 2021

View reviewed changes

lib/internal/repl/utils.js Outdated Show resolved Hide resolved

aduh95 approved these changes Apr 9, 2021

View reviewed changes

aduh95 reviewed Apr 9, 2021

View reviewed changes

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

BridgeAR reviewed Apr 10, 2021

View reviewed changes

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

BridgeAR changed the title ~~Suggested fix for issue #38139 - Node REPL can't handle declaring a variable with the name util~~ repl: fix declaring a variable with the name util Apr 10, 2021

aduh95 reviewed Apr 10, 2021

View reviewed changes

EladKeyshawn commented Apr 10, 2021

View reviewed changes

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

benjamingr requested changes Apr 10, 2021

View reviewed changes

aduh95 reviewed Apr 10, 2021

View reviewed changes

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

benjamingr self-requested a review April 11, 2021 12:41

benjamingr approved these changes Apr 11, 2021

View reviewed changes

aduh95 reviewed Apr 11, 2021

View reviewed changes

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 12, 2021

View reviewed changes

test/parallel/test-repl-history-navigation.js Outdated Show resolved Hide resolved

test/parallel/test-repl-history-navigation.js Show resolved Hide resolved

aduh95 approved these changes Apr 15, 2021

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2021

This comment has been minimized.

Sign in to view

aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 17, 2021

aduh95 reviewed Apr 17, 2021

View reviewed changes

benjamingr approved these changes Apr 18, 2021

View reviewed changes

benjamingr pushed a commit that referenced this pull request Apr 18, 2021

repl: fix declaring a variable with the name util

b2baa3d

PR-URL: #38141 Fixes: #38139 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

benjamingr closed this Apr 18, 2021

benjamingr reopened this Apr 18, 2021

aduh95 force-pushed the eladkeyshawn-repl-redeclare-util-bricks branch from 770a015 to d666964 Compare April 18, 2021 14:14

aduh95 merged commit d666964 into nodejs:master Apr 18, 2021

BethGriggs mentioned this pull request Apr 20, 2021

2021-04-20, Version 16.0.0 (Current) #37678

Merged

aduh95 mentioned this pull request May 5, 2021

>=node-16.0.0: failed test # 12 in parallel/test-repl-history-navigation #38558

Closed

targos added the backport-blocked-v14.x label May 30, 2021

repl: fix declaring a variable with the name util #38141

repl: fix declaring a variable with the name util #38141

Conversation

EladKeyshawn commented Apr 7, 2021 • edited by aduh95 Loading

EladKeyshawn commented Apr 7, 2021

aduh95 left a comment

Choose a reason for hiding this comment

EladKeyshawn commented Apr 8, 2021

EladKeyshawn commented Apr 9, 2021

aduh95 commented Apr 9, 2021

EladKeyshawn commented Apr 9, 2021 • edited Loading

benjamingr commented Apr 10, 2021

benjamingr left a comment

Choose a reason for hiding this comment

Linkgoron commented Apr 10, 2021 • edited Loading

aduh95 commented Apr 10, 2021 • edited Loading

aduh95 commented Apr 10, 2021 • edited Loading

aduh95 commented Apr 11, 2021

EladKeyshawn commented Apr 11, 2021

benjamingr left a comment

Choose a reason for hiding this comment

EladKeyshawn commented Apr 11, 2021

EladKeyshawn commented Apr 11, 2021

aduh95 commented Apr 11, 2021

EladKeyshawn commented Apr 15, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Apr 16, 2021

nodejs-github-bot commented Apr 17, 2021

aduh95 commented Apr 17, 2021

EladKeyshawn commented Apr 17, 2021

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 Apr 17, 2021

Choose a reason for hiding this comment

EladKeyshawn Apr 17, 2021 • edited Loading

Choose a reason for hiding this comment

aduh95 Apr 18, 2021

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 17, 2021

benjamingr commented Apr 18, 2021

benjamingr commented Apr 18, 2021

aduh95 commented Apr 18, 2021

benjamingr commented Apr 18, 2021 • edited Loading

aduh95 commented Apr 18, 2021

EladKeyshawn commented Apr 18, 2021

EladKeyshawn commented Apr 7, 2021 •

edited by aduh95

Loading

EladKeyshawn commented Apr 9, 2021 •

edited

Loading

Linkgoron commented Apr 10, 2021 •

edited

Loading

aduh95 commented Apr 10, 2021 •

edited

Loading

aduh95 commented Apr 10, 2021 •

edited

Loading

EladKeyshawn Apr 17, 2021 •

edited

Loading

benjamingr commented Apr 18, 2021 •

edited

Loading