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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ and servers.

-p, --print print result of --eval

-c, --check syntax check script without executing

-i, --interactive always enter the REPL even if stdin
does not appear to be a terminal

Expand Down
15 changes: 15 additions & 0 deletions lib/internal/module.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';

module.exports.stripBOM = stripBOM;

/**
* Remove byte order marker. This catches EF BB BF (the UTF-8 BOM)
* because the buffer-to-string conversion in `fs.readFileSync()`
* translates it to FEFF, the UTF-16 BOM.
*/
function stripBOM(content) {
if (content.charCodeAt(0) === 0xFEFF) {
content = content.slice(1);
}
return content;
}
16 changes: 3 additions & 13 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const NativeModule = require('native_module');
const util = require('util');
const internalModule = require('internal/module');
const internalUtil = require('internal/util');
const runInThisContext = require('vm').runInThisContext;
const assert = require('assert').ok;
Expand Down Expand Up @@ -431,29 +432,18 @@ Module.prototype._compile = function(content, filename) {
};


function stripBOM(content) {
// Remove byte order marker. This catches EF BB BF (the UTF-8 BOM)
// because the buffer-to-string conversion in `fs.readFileSync()`
// translates it to FEFF, the UTF-16 BOM.
if (content.charCodeAt(0) === 0xFEFF) {
content = content.slice(1);
}
return content;
}


// Native extension for .js
Module._extensions['.js'] = function(module, filename) {
var content = fs.readFileSync(filename, 'utf8');
module._compile(stripBOM(content), filename);
module._compile(internalModule.stripBOM(content), filename);
};


// Native extension for .json
Module._extensions['.json'] = function(module, filename) {
var content = fs.readFileSync(filename, 'utf8');
try {
module.exports = JSON.parse(stripBOM(content));
module.exports = JSON.parse(internalModule.stripBOM(content));
} catch (err) {
err.message = filename + ': ' + err.message;
throw err;
Expand Down
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
'lib/zlib.js',
'lib/internal/child_process.js',
'lib/internal/freelist.js',
'lib/internal/module.js',
'lib/internal/socket_list.js',
'lib/internal/repl.js',
'lib/internal/util.js',
Expand Down
9 changes: 9 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ using v8::Value;

static bool print_eval = false;
static bool force_repl = false;
static bool syntax_check_only = false;
static bool trace_deprecation = false;
static bool throw_deprecation = false;
static bool abort_on_uncaught_exception = false;
Expand Down Expand Up @@ -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.

}

// -i, --interactive
if (force_repl) {
READONLY_PROPERTY(process, "_forceRepl", True(env->isolate()));
Expand Down Expand Up @@ -3099,6 +3105,7 @@ static void PrintHelp() {
" -v, --version print Node.js version\n"
" -e, --eval script evaluate script\n"
" -p, --print evaluate script and print result\n"
" -c, --check syntax check script without executing\n"
" -i, --interactive always enter the REPL even if stdin\n"
" does not appear to be a terminal\n"
" -r, --require module to preload (option can be repeated)\n"
Expand Down Expand Up @@ -3227,6 +3234,8 @@ static void ParseArgs(int* argc,
}
args_consumed += 1;
local_preload_modules[preload_module_count++] = module;
} else if (strcmp(arg, "--check") == 0 || strcmp(arg, "-c") == 0) {
syntax_check_only = true;
} else if (strcmp(arg, "--interactive") == 0 || strcmp(arg, "-i") == 0) {
force_repl = true;
} else if (strcmp(arg, "--no-deprecation") == 0) {
Expand Down
16 changes: 16 additions & 0 deletions src/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,22 @@
process.argv[1] = path.resolve(process.argv[1]);

var Module = NativeModule.require('module');

// 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.

var internalModule = NativeModule.require('internal/module');
// read the source
var filename = Module._resolveFilename(process.argv[1]);
var source = fs.readFileSync(filename, 'utf-8');
// remove shebang and BOM
source = internalModule.stripBOM(source.replace(/^\#\!.*/, ''));
// compile the script, this will throw if it fails
new vm.Script(source, {filename: filename, displayErrors: true});
process.exit(0);
}

startup.preloadModules();
if (global.v8debug &&
process.execArgv.some(function(arg) {
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/syntax/bad_syntax.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
var foo bar;
2 changes: 2 additions & 0 deletions test/fixtures/syntax/bad_syntax_shebang.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#!/usr/bin/env node
var foo bar;
1 change: 1 addition & 0 deletions test/fixtures/syntax/good_syntax.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
var foo = 'bar';
2 changes: 2 additions & 0 deletions test/fixtures/syntax/good_syntax_shebang.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#!/usr/bin/env node
var foo = 'bar';
84 changes: 84 additions & 0 deletions test/parallel/test-cli-syntax.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
'use strict';

const assert = require('assert');
const spawnSync = require('child_process').spawnSync;
const path = require('path');

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

var node = process.execPath;

// test both sets of arguments that check syntax
var syntaxArgs = [
['-c'],
['--check']
];

// test good syntax with and without shebang
[
'syntax/good_syntax.js',
'syntax/good_syntax',
'syntax/good_syntax_shebang.js',
'syntax/good_syntax_shebang',
].forEach(function(file) {
file = path.join(common.fixturesDir, file);

// loop each possible option, `-c` or `--check`
syntaxArgs.forEach(function(args) {
var _args = args.concat(file);
var c = spawnSync(node, _args, {encoding: 'utf8'});

// no output should be produced
assert.equal(c.stdout, '', 'stdout produced');
assert.equal(c.stderr, '', 'stderr produced');
assert.equal(c.status, 0, 'code == ' + c.status);
});
});

// test bad syntax with and without shebang
[
'syntax/bad_syntax.js',
'syntax/bad_syntax',
'syntax/bad_syntax_shebang.js',
'syntax/bad_syntax_shebang'
].forEach(function(file) {
file = path.join(common.fixturesDir, file);

// loop each possible option, `-c` or `--check`
syntaxArgs.forEach(function(args) {
var _args = args.concat(file);
var c = spawnSync(node, _args, {encoding: 'utf8'});

// no stdout should be produced
assert.equal(c.stdout, '', 'stdout 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);
});
});

// test file not found
[
'syntax/file_not_found.js',
'syntax/file_not_found'
].forEach(function(file) {
file = path.join(common.fixturesDir, file);

// loop each possible option, `-c` or `--check`
syntaxArgs.forEach(function(args) {
var _args = args.concat(file);
var c = spawnSync(node, _args, {encoding: 'utf8'});

// no stdout should be produced
assert.equal(c.stdout, '', 'stdout 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);
});
});