cli: ensure --check flag works for piped-in code by not-an-aardvark · Pull Request #11689 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Previously, the --check CLI flag had no effect when run on code piped
from stdin. This commit updates the bootstrap logic to handle the
--check flag the same way regardless of whether the code is piped from
stdin.

Fixes: #11680

Affected core subsystem(s)

lib

Why drop the filename?

If I'm not mistaken, the filename doesn't affect whether a script is syntactically valid, and piped-in code has no filename.

I think the filename gets displayed if there is an error, which might be helpful. Dropping that information would probably be a semver major. The piped-in scenario could just use [stdin].

Ok, I'll add it back.

Fixes: #11680

But only the first part of it, if I’m reading the code here correctly? I think we should support -c -e as well (or error out when those arguments are passed).

Correct, this currently does not fix -c -e. What do you think should be the behavior with -c -e? It seems like flags for "check syntax only" and "evaluate this code" would contradict each other, so I'm not sure about the correct behavior.

(or error out when those arguments are passed).

What do you think should be the behavior with -c -e?

+1 to erroring out (with a suitable message).

I added a commit to throw when -c and -e are used together.

I couldn't find any other logic like this for flags, so I sort of improvised with the error message. Let me know if there's a better way to do this/a convention to follow.

The exit code should be 9, see https://nodejs.org/dist/latest-v7.x/docs/api/process.html#process_exit_codes.

#12087 is doing something similar for the mutually exclusive --use-bundled-ca and --use-openssl-ca options.

Duplicate "should"

The exit code should be 9, see https://nodejs.org/dist/latest-v7.x/docs/api/process.html#process_exit_codes.

#12087 is doing something similar for the mutually exclusive --use-bundled-ca and --use-openssl-ca options.

Updated to address comments.

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

Quick note: if the intention is to have this land for 8.0.0, it needs to be completed by April 4th.

Should these be checked here or should they be checked in node.cc? Note that there are other mutually exclusive flags and there is an open PR that does this kind of check directly within node.cc

the assert message should likely read === since we're testing strictEqual

Updated, new CI: https://ci.nodejs.org/job/node-test-pull-request/7147/

@jasnell I couldn't find the other mutually exclusive flags that you were referring to, but I moved the check to node.cc. Let me know if there's another place in node.cc that it should go instead.

Actually, I see that another mutually-exclusive-flag check landed on master and is now causing a conflict with this PR. I suppose I put the check in the right place then. 😃

Previously, the --check CLI flag had no effect when run on code piped from stdin. This commit updates the bootstrap logic to handle the --check flag the same way regardless of whether the code is piped from stdin. Fixes: nodejs#11680

The -c flag ("check script syntax") and -e flag ("evaluate given code") have contradictory meanings. Make them mutually exclusive by throwing when both of them are provided.

Rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/7148/

comments have been addressed

All ==s here should probably be ===, but that can be fixed while landing

Previously, the --check CLI flag had no effect when run on code piped from stdin. This commit updates the bootstrap logic to handle the --check flag the same way regardless of whether the code is piped from stdin. PR-URL: nodejs#11689 Fixes: nodejs#11680 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

The -c flag ("check script syntax") and -e flag ("evaluate given code") have contradictory meanings. Make them mutually exclusive by throwing when both of them are provided. Fixes: nodejs#11680 PR-URL: nodejs#11689 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Landed in 3209a8e and a5f91ab.

Windows CI is not green, test-cli-syntax fails on Windows (https://ci.nodejs.org/job/node-test-binary-windows/7416/):

not ok 32 parallel/test-cli-syntax
  ---
  duration_ms: 3.104
  severity: fail
  stack: |-
    
    assert.js:81
      throw new assert.AssertionError({
      ^
    AssertionError: 'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\1\\VS_VERSION\\vcbt2015\\label\\win10\\Release\\node.exe: either --check o === 'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\1\\VS_VERSION\\vcbt2015\\label\\win10\\Release\\node.exe: either --check o
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:127:12
        at Array.forEach (native)
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:123:20
        at Array.forEach (native)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:122:19)
        at Module._compile (module.js:607:30)
        at Object.Module._extensions..js (module.js:618:10)
        at Module.load (module.js:516:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
  ...

Edit: the CI mangled the error message, it should be:
AssertionError: '(...): either --check or --eval can be used, not both\r\n' === '(...): either --check or --eval can be used, not both\n'

Sorry about that. I'll be able to create a PR to fix it in about 2 hours if no one gets around to it before then.

See #12212

Not 100% sure I agree, it could be nice to pass source code directly to -c I guess.

I doubt anyone would use it though, so really... ¯\_(ツ)_/¯

The test introduced in a5f91ab accidentally introduced failures on some windows builds. Update the assertion that was causing the failures. PR-URL: nodejs#12212 Ref: nodejs#11689 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

@Fishrock123 To clarify, it's possible to do echo foo | node -c (added in this PR). It's just that node -c -e foo is disallowed.

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 5, 2017

mscdex added cli Issues and PRs related to the Node.js command line interface. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 5, 2017

cjihrig reviewed Mar 6, 2017

View reviewed changes

not-an-aardvark force-pushed the check-syntax-stdin branch from 670932b to 36d5d3f Compare March 6, 2017 02:13

not-an-aardvark changed the title lib: ensure --check flag works for piped-in code cli: ensure --check flag works for piped-in code Mar 6, 2017

not-an-aardvark force-pushed the check-syntax-stdin branch from 36d5d3f to fcd00d2 Compare March 30, 2017 06:41

not-an-aardvark added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 30, 2017

not-an-aardvark commented Mar 30, 2017

View reviewed changes

cjihrig approved these changes Mar 30, 2017

View reviewed changes

richardlau requested changes Mar 30, 2017

View reviewed changes

richardlau approved these changes Mar 30, 2017

View reviewed changes

jasnell added this to the 8.0.0 milestone Mar 31, 2017

jasnell previously requested changes Mar 31, 2017

View reviewed changes

not-an-aardvark added 7 commits April 1, 2017 03:42

cli: ensure --check flag works for piped-in code

6e255ad

Previously, the --check CLI flag had no effect when run on code piped from stdin. This commit updates the bootstrap logic to handle the --check flag the same way regardless of whether the code is piped from stdin. Fixes: nodejs#11680

squash: ensure the filename is still used

bc820d5

cli: throw when -c and -e are used simultaneously

24ad2bf

The -c flag ("check script syntax") and -e flag ("evaluate given code") have contradictory meanings. Make them mutually exclusive by throwing when both of them are provided.

squash: change exit code to 9 for invalid args

913d121

squash: remove duplicate word in commit

5578701

squash: make assertion error message more specific

299155d

squash: move flag validation to c++

fab87a9

not-an-aardvark force-pushed the check-syntax-stdin branch from 36051a8 to fab87a9 Compare April 1, 2017 07:47

addaleax approved these changes Apr 3, 2017

View reviewed changes

jasnell approved these changes Apr 3, 2017

View reviewed changes

not-an-aardvark closed this Apr 4, 2017

not-an-aardvark deleted the check-syntax-stdin branch April 4, 2017 02:16

not-an-aardvark mentioned this pull request Apr 4, 2017

test: fix test-cli-syntax assertions on windows #12212

Merged

2 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

jdalton mentioned this pull request Jan 29, 2018

--check work with --require #18425

Closed

+                const match = c.stderr.match(/^SyntaxError: Unexpected identifier$/m);
+                assert(match, 'stderr incorrect');
+                assert.strictEqual(c.status, 1, 'code == ' + c.status);

cli: ensure --check flag works for piped-in code by not-an-aardvark · Pull Request #11689 · nodejs/node · GitHub

Uh oh!

Uh oh!

cli: ensure --check flag works for piped-in code - #11689

cli: ensure --check flag works for piped-in code#11689not-an-aardvark wants to merge 7 commits intonodejs:masternodejs/node:masterfrom not-an-aardvark:check-syntax-stdinnot-an-aardvark/node:check-syntax-stdinCopy head branch name to clipboard

Conversation

not-an-aardvark commented Mar 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Mar 6, 2017

Uh oh!

not-an-aardvark commented Mar 8, 2017

Uh oh!

gibfahn commented Mar 12, 2017

Uh oh!

not-an-aardvark commented Mar 30, 2017

Uh oh!

not-an-aardvark Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Mar 30, 2017

Uh oh!

jasnell commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Apr 1, 2017

Uh oh!

not-an-aardvark commented Apr 1, 2017

Uh oh!

not-an-aardvark commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Apr 4, 2017

Uh oh!

bzoz commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

not-an-aardvark commented Apr 4, 2017

Uh oh!

not-an-aardvark commented Apr 4, 2017

Uh oh!

Fishrock123 commented Apr 4, 2017

Uh oh!

not-an-aardvark commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

cli: ensure --check flag works for piped-in code#11689
not-an-aardvark wants to merge 7 commits into
nodejs:masterfrom
not-an-aardvark:check-syntax-stdin

not-an-aardvark Mar 30, 2017 •
edited
Loading

bzoz commented Apr 4, 2017 •
edited
Loading