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

add CLI option to syntax check script #2411

Closed
wants to merge 1 commit into from

Conversation

bahamas10
Copy link
Contributor

original PR: nodejs/node-v0.x-archive#9447
original discussion: nodejs/node-v0.x-archive#9426

examples

$ cat ./good.js
var foo = 'bar';
$ ./out/Release/iojs -c ./good.js
$ echo $?
0
$ cat ./bad.js
var foo bar;
$ ./out/Release/iojs -c ./bad.js
var foo bar;
        ^^^

SyntaxError: Unexpected identifier
    at startup (node.js:99:13)
    at node.js:961:3
$ echo $?
1

tests pass

$ ./out/Release/iojs test/parallel/test-cli-syntax.js 
calling /Users/dave.eddy/dev/node-1/out/Release/iojs -c /Users/dave.eddy/dev/node-1/test/fixtures/syntax/good_syntax.js
ok
calling /Users/dave.eddy/dev/node-1/out/Release/iojs --check /Users/dave.eddy/dev/node-1/test/fixtures/syntax/good_syntax.js
ok
calling /Users/dave.eddy/dev/node-1/out/Release/iojs -c /Users/dave.eddy/dev/node-1/test/fixtures/syntax/good_syntax_shebang.js
ok
calling /Users/dave.eddy/dev/node-1/out/Release/iojs --check /Users/dave.eddy/dev/node-1/test/fixtures/syntax/good_syntax_shebang.js
ok
calling /Users/dave.eddy/dev/node-1/out/Release/iojs -c /Users/dave.eddy/dev/node-1/test/fixtures/syntax/bad_syntax.js
ok
calling /Users/dave.eddy/dev/node-1/out/Release/iojs --check /Users/dave.eddy/dev/node-1/test/fixtures/syntax/bad_syntax.js
ok
calling /Users/dave.eddy/dev/node-1/out/Release/iojs -c /Users/dave.eddy/dev/node-1/test/fixtures/syntax/bad_syntax_shebang.js
ok
calling /Users/dave.eddy/dev/node-1/out/Release/iojs --check /Users/dave.eddy/dev/node-1/test/fixtures/syntax/bad_syntax_shebang.js
ok

if (process._syntax_check_only != null) {
var vm = NativeModule.require('vm');
var fs = NativeModule.require('fs');
var source = fs.readFileSync(process.argv[1], 'utf-8').replace(/^#.*/, '');
Copy link
Member

Choose a reason for hiding this comment

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

I see a couple of issues here:

  1. It won't work for filenames without an extension (node app vs. node app.js)
  2. It won't skip the UTF-8 BOM (byte order marker) like require() does.
  3. The regular expression doesn't match the one in lib/module.js that matches the string #!.

It's probably unavoidable that fixing 2 and 3 results in some duplicated code but it should come with regression tests that ensure the implementations won't diverge.

@bnoordhuis
Copy link
Member

Left some comments. Basic premise of this PR seems alright to me.

@bahamas10
Copy link
Contributor Author

Thanks @bnoordhuis for the review.

  1. I've updated the code to use Module._resolveFilename on process.argv[1] to fix the node app vs node app.js problem.
  2. I've exposed stripBOM as Module._stripBOM which is used by this code in node.js (to avoid repeated code)
  3. updated regex to match that in lib/module.js
  4. updated tests to use const and 'use strict';... it turns out I was testing joyent/node and not nodejs/node accidentally before... so i've made sure this all passes make test in this repo
  5. removed all logging on success in the test supplied

Additionally I've added to test for file vs file.js, as well as files that don't exist.

@@ -2843,6 +2844,11 @@ void SetupProcessObject(Environment* env,
READONLY_PROPERTY(process, "_print_eval", True(env->isolate()));
}

// -c, --check
if (syntax_check_only) {
READONLY_PROPERTY(process, "_syntax_check_only", True(env->isolate()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer using consistent camel casing. I am not sure if we can fix the _print_eval above. cc @bnoordhuis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to change this if everyone agrees this should be done - I just tried to match the style I saw in this file.

Copy link
Member

Choose a reason for hiding this comment

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

It's consistent with _print_eval. I'm fine with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay then. I was just worried that we would introduce non camel case identifiers in to the js land.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? the syntax check won't actually launch the script so there will never be access to process._syntax_check_only.

Copy link
Contributor

Choose a reason for hiding this comment

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

wait. nm. was thinking from the users' perspective. forgot about internal tooling.

@rvagg
Copy link
Member

rvagg commented Aug 18, 2015

Hey @bahamas10, welcome!

This does seem like something that could easily be achieved by a userland module, surely there's already something in npm that does this, so what's the case for putting it in core?

@bahamas10
Copy link
Contributor Author

Hey @rvagg

The indirect answer is here nodejs/node-v0.x-archive#9426

But to be more specific, to syntax check scripts:

  • bash -n somescript
  • ruby -c somescript.rb
  • perl -c somescript.pl

etc.

Most interpreted language interpreters support some way of saying "are the contents of the file given valid for interpretation?". This does not check style nor does it lint the content - just syntax.

However, I realize the argument that "a bunch of people are doing it" is not sufficient.

The node core is already the source of truth when it comes to compiling JavaScript source and determining if it is valid. For example, an npm module would be nothing more than a thin wrapper around the vm module and new vm.Script(...). Since the node core is already doing the heavy lifting, and it itself contains the logic to determine the validity of JavaScript source code, it alone should be responsible for it.

As an example, imagine the following script (downloaded from npm or wherever) called syntax-check

#!/usr/bin/env node
var vm = require('vm');
var fs = require('fs');
var source = fs.readFileSync(process.argv[2], 'utf8').replace(/^#.*/, '');
new vm.Script(source, {displayErrors: true});

You could then call this script in the following way to achieve almost the same results as this PR

$ ./syntax-check syntax-check
$ echo $?
0

However, this only checks the validity of it against whichever node binary is available first in the users $PATH (because of the shebang). So test other versions of node a user must run something like

$ ~/dev/node/out/Release/node "$(npm config get prefix)/bin/syntax-check" script.js

This requires a lot of knowledge about the underlying system, and also forces a specific interpreter for the script, going against what is in the shebang (ie. what the user intended it to be executed with) - thus running a script downloaded from npm for a specific version of node with a different version of node.

I think it makes the most sense to cut out anything in the middle - any 3rd party script - and just ask the node binary itself "can you run this?"

@bahamas10
Copy link
Contributor Author

  1. moved stripBOM to lib/internal/module.js and use it in both lib/module.js and src/node.js
  2. syntax_args => syntaxArgs
  3. args.concat([file]) => args.concat(file)

make test passes

@brendanashworth brendanashworth added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 18, 2015
@bnoordhuis
Copy link
Member

The commit log should conform to the guidelines from CONTRIBUTING.md but otherwise LGTM. Let's see what the CI thinks: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/104/

@bahamas10
Copy link
Contributor Author

updated commit message based on CONTRIBUTING.md guidelines.

@rvagg
Copy link
Member

rvagg commented Aug 18, 2015

OK, I'm -0 on this, I won't hold up merging if other collaborators feel strongly enough that this is a good thing to have

@bahamas10
Copy link
Contributor Author

(merge conflicts resolved and rebased with latest master)

@bahamas10
Copy link
Contributor Author

@rvagg I don't feel comfortable about this being merged if you are having reservations about it. Is there anything specific about this you don't like? or is it an overall negative feeling about adding something extra to the node core?

If you're available I'm happy to talk about this and make sure we are both on the same side with regards to this change.

@bnoordhuis
Copy link
Member

I didn't comment on it last time but FWIW, it seems like a reasonable addition to me with precedent in other scripting environments. Putting it in a npm package makes it markedly less convenient.

@rvagg
Copy link
Member

rvagg commented Sep 5, 2015

yeah, sorry for not responding earlier, this is fine by me, the fact that other runtimes have the same functionality makes this easier to swallow

@rvagg
Copy link
Member

rvagg commented Sep 5, 2015

https://ci.nodejs.org/job/node-test-pull-request/254/ again, the last one looked pretty flaky

// check if user passed `-c` or `--check` arguments to Node.
if (process._syntax_check_only != null) {
var vm = NativeModule.require('vm');
var fs = NativeModule.require('fs');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use const wherever the values are assigned once and never changed again.

Copy link
Member

Choose a reason for hiding this comment

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

this could go either way cause it's not at all consistent in the file, we might be shifting that way but there's only one use of const amongst the 20 NativeModule.require calls already in this file, someone's probably going to do a round of cleanup at some point and this change could be done then

Copy link
Contributor

Choose a reason for hiding this comment

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

@rvagg Oh, okay. I thought if we allowed the new code being landed to follow this rule strictly, then we just have to cleanup only the old code.

@rvagg
Copy link
Member

rvagg commented Sep 5, 2015

all green, https://ci.nodejs.org/job/node-test-commit/506/, that's pretty good given the recent state of our suite, lgtm


const common = require('../common');

var node = process.argv[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@bahamas10 could you change this please?

@Fishrock123
Copy link
Contributor

Seems fine.

@yosuke-furukawa
Copy link
Member

+1

@rvagg
Copy link
Member

rvagg commented Sep 25, 2015

@bahamas10 I see 2 outstanding items, pointed them out inline, then I'll merge this and we should be able to get it into v4.x

@bahamas10
Copy link
Contributor Author

thanks @thefourtheye for the problems you've spotted. @rvagg i've force pushed a new commit with the changes... here is the intermediate diff to easily spot the changes

$ gd
diff --git a/test/parallel/test-cli-syntax.js b/test/parallel/test-cli-syntax.js
index 059e3ef..20fdfdc 100644
--- a/test/parallel/test-cli-syntax.js
+++ b/test/parallel/test-cli-syntax.js
@@ -6,7 +6,7 @@ const path = require('path');

 const common = require('../common');

-var node = process.argv[0];
+var node = process.execPath;

 // test both sets of arguments that check syntax
 var syntaxArgs = [
@@ -49,9 +49,13 @@ var syntaxArgs = [
     var _args = args.concat(file);
     var c = spawnSync(node, _args, {encoding: 'utf8'});

-    // no output should be produced
+    // no stdout should be produced
     assert.equal(c.stdout, '', 'stdout produced');
-    assert(c.stderr, 'stderr not produced');
+
+    // stderr should have a syntax error message
+    var match = c.stderr.match(/^SyntaxError: Unexpected identifier$/m);
+    assert(match, 'stderr incorrect');
+
     assert.equal(c.status, 1, 'code == ' + c.status);
   });
 });
@@ -68,9 +72,13 @@ var syntaxArgs = [
     var _args = args.concat(file);
     var c = spawnSync(node, _args, {encoding: 'utf8'});

-    // no output should be produced
+    // no stdout should be produced
     assert.equal(c.stdout, '', 'stdout produced');
-    assert(c.stderr, 'stderr not produced');
+
+    // stderr should have a module not found error message
+    var match = c.stderr.match(/^Error: Cannot find module/m);
+    assert(match, 'stderr incorrect');
+
     assert.equal(c.status, 1, 'code == ' + c.status);
   });
 });

@Fishrock123
Copy link
Contributor

I'd say to please use const when you can if you add or update a variable. :)

@rvagg
Copy link
Member

rvagg commented Sep 29, 2015

grf, bikeshed, can we defer the var vs const thing please @Fishrock123 & @thefourtheye? This 1/2 way state we're in is too messy and I haven't seen anything other than a vague idea amongst some collaborators that const is a must now.

rvagg pushed a commit that referenced this pull request Oct 3, 2015
PR-URL: #2411
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@rvagg
Copy link
Member

rvagg commented Oct 3, 2015

landed @ 2e6ece4 with commit summary modification

thanks @bahamas10 and congrats on first commit, and it's a significant one too!

@rvagg rvagg closed this Oct 3, 2015
@bahamas10
Copy link
Contributor Author

@rvagg + all. nice! thanks everyone.

@jasnell
Copy link
Member

jasnell commented Oct 6, 2015

Nice to see this landed 👍

@jasnell jasnell mentioned this pull request Oct 8, 2015
29 tasks
jasnell pushed a commit that referenced this pull request Oct 8, 2015
PR-URL: #2411
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
jasnell added a commit that referenced this pull request Oct 12, 2015
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
jasnell added a commit that referenced this pull request Oct 12, 2015
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
jasnell added a commit that referenced this pull request Oct 12, 2015
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
jasnell added a commit that referenced this pull request Oct 12, 2015
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
@jays1204
Copy link

I understand that we are done talking about this topic. However, I am writing to ask you one question.
If test.js file has below contents then 'node test.js' works.

  • test.js
if (true) {
  return;
}

But 'node -c test.js' occurs error "SyntaxError: Illegal return statement".

'return' statement works in function. But node module system wraps files in a self-executing function. so "node test.js" works but "node -c test.js" not works.
Is this okay?
If I bother you then I'm sorry.

@bnoordhuis
Copy link
Member

@jays1204 I'd consider that a bug. Can you open an issue?

@jays1204
Copy link

@bnoordhuis already someone opened it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.