Handle '//:lint' and '//:nolint' comments. (issue 7801048)

[usrb...@yahoo.com]

Reviewers: arv-chromium, slightlylate, peterhal,

Message:
Note: I'm just cleaning out old issues. New patches will go through
github once I get up to speed on it.

----

Some open questions:
- Should '//:lint' force on checking?
- Should lint and nolint nest? Meaning, should the following be entirely
nolint?

//:nolint
var a
//:nolint
var b
//:lint
var c // ?
//:lint
- Should nolint disable checks inside an imported module?

#--cut--
dl_check() {
local f=$(basename $1)
test -e $f || curl -sO $1
openssl sha1 < $f | grep -q "= $2" && return 0
echo sha1 mismatch!
return 1
}

git checkout -b issue7801048-nolint

dl_check https://codereview.appspot.com/download/issue7801048_1.diff \
d33432ff858dbe5a && git apply issue7801048_1.diff && make test
#--cut--

----

I have two follow-up patches for this, but I'll try submitting them
through
github. I still need to get comfortable with the new workflow.


Description:
Handle '//:lint' and '//:nolint' comments.

Currently this only disables 'strictSemicolons' inside 'nolint' zones.

The transition from 'nolint' to 'lint' is a little tricky since comments
are not tokens, meaning that for proper transition back to 'lint' mode,
you need to make sure there is a semicolon (i.e. you have to manually
add a token) either before or after the 'lint' to make sure any errors
from the 'nolint' side don't leak through.

BUG=None
TEST=test/feature/Syntax/Error_StrictSemiColonsNoLint.js
test/feature/Syntax/StrictSemiColonsNoLint.js


Please review this at https://codereview.appspot.com/7801048/

Affected files:
M src/syntax/Parser.js
M src/syntax/Scanner.js
A test/feature/Syntax/Error_StrictSemiColonsNoLint.js
A test/feature/Syntax/StrictSemiColonsNoLint.js


Index: src/syntax/Parser.js
diff --git a/src/syntax/Parser.js b/src/syntax/Parser.js
index
323065f9ae8ca7fe5767d788ada70239b0aaa26a..d74233ca0b38b386cb36ce6f4eafe346229c9ac2
100644
--- a/src/syntax/Parser.js
+++ b/src/syntax/Parser.js
@@ -134,7 +134,7 @@ export class Parser {
*/
constructor(errorReporter, file) {
this.errorReporter_ = errorReporter;
- this.scanner_ = new Scanner(errorReporter, file);
+ this.scanner_ = new Scanner(errorReporter, file, this);

/**
* Keeps track of whether we currently allow yield expressions.
@@ -142,6 +142,7 @@ export class Parser {
* @private
*/
this.allowYield_ = options.unstarredGenerators;
+ this.noLint = false;
}

// 14 Program
@@ -3410,7 +3411,7 @@ export class Parser {
eatPossibleImplicitSemiColon_() {
var token = this.peekTokenNoLineTerminator_();
if (!token) {
- if (!options.strictSemicolons)
+ if (!options.strictSemicolons || this.noLint)
return;
} else {
switch (token.type) {
@@ -3419,7 +3420,7 @@ export class Parser {
return;
case END_OF_FILE:
case CLOSE_CURLY:
- if (!options.strictSemicolons)
+ if (!options.strictSemicolons || this.noLint)
return;
}
}
Index: src/syntax/Scanner.js
diff --git a/src/syntax/Scanner.js b/src/syntax/Scanner.js
index
efb5126c1d455476418cda98b8ba7225c5f76c65..5f387230fa02dd38dac75968765c0df04ae07353
100644
--- a/src/syntax/Scanner.js
+++ b/src/syntax/Scanner.js
@@ -247,7 +247,7 @@ function isRegularExpressionFirstChar(code) {
}

var index, input, length, token, lastToken, lookaheadToken,
currentCharCode,
- lineNumberTable, errorReporter;
+ lineNumberTable, errorReporter, currentParser;

/**
* Scans javascript source code into tokens. All entrypoints assume the
@@ -263,7 +263,7 @@ export class Scanner {
* @param {ErrorReport} reporter
* @param {SourceFile} file
*/
- constructor(reporter, file) {
+ constructor(reporter, file, parser) {
// These are not instance fields and this class should probably be
refactor
// to not give a false impression that multiple instances can be
created.
errorReporter = reporter;
@@ -275,6 +275,7 @@ export class Scanner {
token = null;
lookaheadToken = null;
updateCurrentCharCode();
+ currentParser = parser;
}

get lastToken() {
@@ -608,9 +609,20 @@ function skipComment() {
}

function skipSingleLineComment() {
- while (!isAtEnd() && !isLineTerminator(currentCharCode)) {
- next();
+ var c, text;
+ index += 2;
+ if (!isAtEnd() && !isLineTerminator(c = input.charCodeAt(index++))) {
+ // Check for '//:' first so that we can immediately skip the expensive
slice
+ // and regexp for normal comments.
+ if (c === 58) { // :
+ text = input.slice(index, index + 6);
+ if (text.search(/^(?:no)?lint\b/) === 0)
+ currentParser.noLint = text[0] === 'n';
+ }
+ while (!isAtEnd() && !isLineTerminator(input.charCodeAt(index++))) {}
}
+
+ updateCurrentCharCode();
}

function skipMultiLineComment() {
Index: test/feature/Syntax/Error_StrictSemiColonsNoLint.js
diff --git a/test/feature/Syntax/Error_StrictSemiColonsNoLint.js
b/test/feature/Syntax/Error_StrictSemiColonsNoLint.js
new file mode 100644
index
0000000000000000000000000000000000000000..1833213a591d69a8d4676f1e62856d4db1385661
--- /dev/null
+++ b/test/feature/Syntax/Error_StrictSemiColonsNoLint.js
@@ -0,0 +1,34 @@
+// Should not compile.
+// Options: --strict-semicolons
+// Error: 34:1: Semi-colon expected
+
+//:this is a nolint error test.
+// It is identical to StrictSemiColonsNoLint.js except for one missing
+// semicolon.
+
+// Note: The leading letter in the var names changes for each expected
+// transition between lint and nolint.
+var a0;
+var a1;
+//:nolint
+var b2
+//:nolint These don't nest (should they?).
+var b3
+// hello lint
+var b4
+var b5
+var b6;
+var b7
+var b8
+//:lint
+//:nolint
+var c1
+var c2
+//:hello lint
+var c3
+var c4
+; // Necessary for nolint-to-lint transitions.
+//:lint
+var d1 // <<< missing semicolon here. The error location is at the next
token.
+//:lint
+var d2;
Index: test/feature/Syntax/StrictSemiColonsNoLint.js
diff --git a/test/feature/Syntax/StrictSemiColonsNoLint.js
b/test/feature/Syntax/StrictSemiColonsNoLint.js
new file mode 100644
index
0000000000000000000000000000000000000000..936fa9597ccdcf5aca565b769db86a09d8247a66
--- /dev/null
+++ b/test/feature/Syntax/StrictSemiColonsNoLint.js
@@ -0,0 +1,30 @@
+// Options: --strict-semicolons
+
+//:this is a nolint test.
+
+// Note: The leading letter in the var names changes for each expected
+// transition between lint and nolint.
+var a0;
+var a1;
+//:nolint
+var b2
+//:nolint These don't nest (should they?).
+var b3
+// hello lint
+var b4
+var b5
+var b6;
+var b7
+var b8
+//:lint
+//:nolint
+var c1
+var c2
+//:hello lint
+var c3
+var c4
+; // Necessary for nolint-to-lint transitions.
+//:lint
+var d1;
+//:lint
+var d2;

[a...@chromium.org]

I don't think we want to do this at this layer.

Can we either use a third party linter, like jshint or make linting a
tool that is run before compiling?

I also believe we should fix the parser to keep comments before we do
anything based on comments.

https://codereview.appspot.com/7801048/

[usrb...@yahoo.com]

The problem is that there doesn't currently exist a linter that accepts
ES6, and certainly not one that's going to keep up with the changes in
traceur.

Handling comments is a little problematic because you definitely don't
want them to be first-class tokens. I'm thinking you'd probably want to
attach them as attributes to the first real token that follows. Unless
it's a comment at the end of the file, in which case, it would make most
sense to attach it as an attribute of the Program, with some metadata to
indicate that it's the trailing comment.

----

An alternative, better solution (in my opinion) is to callback into the
parser with each comment encountered, and the parser can either look at
it or ignore it as needed. As long as we make it a low-cost operation
(basically, call the parser with the buffer, and the start and end
indexes), it shouldn't be too much of a performance drag.

I'm leaving the solution in the previous section just in case it sparks
some idea.

----

There are some things that seem easier to do at the scanner level.
Things like spaces between operators (and only one space) and line
length (including line length of comments, but not those lines
containing urls). Theoretically we can do the same thing using source
locations, but at first glance, it seems like the solution would be much
messier.

I'm thinking I'd have to hook into the SourceLocation constructor, and
have it update some kind of global state. And that global state needs to
know what tokens are associated with those locations.

Or I'd have to touch every single function in Parser.js, or add lint
self-evaluation code to be generated in ParseTrees.js -- the second
sounds a little better, but it's still a little scary.


https://codereview.appspot.com/7801048/

[usrb...@yahoo.com]

I moved the comment-handling into the parser. While this still doesn't
save comments, the parser is pretty close to being able to do so (once
we hook in 'handleMultiLineComment').

This is a partial implementation of the idea noted in my previous
message.

#--cut--
dl_check() {
local f=$(basename $1)
test -e $f || curl -sO $1
openssl sha1 < $f | grep -q "= $2" && return 0
echo sha1 mismatch!
return 1
}

git checkout af93a2273e581669 # last merge with branch master

git checkout -b issue7801048_5001-nolint

# I need to figure out how to do this automatically. It's easy for this
# to get out of sync.
rm test/test-list.js

dl_check https://codereview.appspot.com/download/issue7801048_5001.diff
\
b7e6878e91d227ba && git apply issue7801048_5001.diff && make test
#--cut--

----

I probably won't get another chance to use this phrase, so:

PTAL

It's only been what, a week? And I'm already feeling a little nostalgic.



https://codereview.appspot.com/7801048/diff/5001/src/options.js
File src/options.js (right):

https://codereview.appspot.com/7801048/diff/5001/src/options.js#newcode277
src/options.js:277: addBoolOption('ignoreNolint');
I already had this set up for a follow-up patch. It would require a
manual merge (I really don't like manual merges) at this point to keep
this as a separate commit, so I'm just including it. It's a pretty minor
addition, anyway.

An odd anecdote: the option was originally named 'ignoreNoLint' ... can
you see the bug? I didn't.

The problem is that commander.js treats options matching /-no-/
specially, when it should really be matching on /^--no-/. So

./traceur --ignore-no-lint

was always false. I started doubting my sanity while chasing down this
bug.

https://codereview.appspot.com/7801048/

[a...@chromium.org]

On 2013/04/05 15:51:31, usrbincc wrote:
> I moved the comment-handling into the parser. While this still doesn't
> save comments, the parser is pretty close to being able to do so (once
> we hook in 'handleMultiLineComment').

I like the parser changes but I'm still not sold that we want to do the
//:nolint part.

> https://codereview.appspot.com/7801048/diff/5001/src/options.js
> File src/options.js (right):


https://codereview.appspot.com/7801048/diff/5001/src/options.js#newcode277
> src/options.js:277: addBoolOption('ignoreNolint');
> I already had this set up for a follow-up patch. It would require a
> manual merge (I really don't like manual merges) at this point to keep
> this as a separate commit, so I'm just including it. It's a pretty
minor
> addition, anyway.

> An odd anecdote: the option was originally named 'ignoreNoLint' ...
can
> you see the bug? I didn't.

> The problem is that commander.js treats options matching /-no-/
> specially, when it should really be matching on /^--no-/. So

> ./traceur --ignore-no-lint

> was always false. I started doubting my sanity while chasing down this
> bug.

Wow, another reason we should move away from commmander.js



https://codereview.appspot.com/7801048/

[usrb...@yahoo.com]

> I like the parser changes but I'm still not sold that we want to do
> the //:nolint part.

Note: I'm assuming here that you're referring to //:nolint as part
of traceur, and not the concept of nolint itself.

I agree that it's not the cleanest separation of concerns, but there
are practical benefits. Piggybacking on the main build means that your
compile times remain about the same, because you parse once, not twice.

----

My main motivation for this patch is to be able to turn on strict
semicolons checking again in the traceur build.

I already have a follow-up patch for 'expand-js-template.js' that
inserts nolint and lint around all included files matching a given
pattern (/^node_modules/ in our specific case).

Okay, I promise, that's the last follow-up patch in this series.

I'd like to achieve this goal without impacting build time, especially
after so much effort was put into optimizing it. (I'm not overly
attached to any particular implementation that meets that criteria.)

----

In general, I'd like to automate as much error-checking as possible,

Enabling '--free-variable-checker' would be another advance in this
area.

and to have the process be as fast as possible. Let the coders be
coders, the compilers be compilers, and let the compilers wait on the
coders, rather than vice versa. Viva humanity! Down with the machines!

So to speak. A command prompt is still better company than many
people.

----

> Wow, another reason we should move away from commmander.js

I wouldn't be so concerned with this if the pull request queue over at
https://github.com/visionmedia/commander.js/pulls wasn't so long. I
started putting together a few bugfixes, but kind of got discouraged
after seeing the lack of response to the other pulls. I'll probably
still give it a try, it's just not a top priority at present.

I think part of the issue is that the maintainer is an extremely
prolific coder, and of all the projects he's handling, this one
doesn't seem to be very high up the list.


https://codereview.appspot.com/7801048/

[a...@chromium.org]

LGTM

I'll commit this for you since I have the tools ready

https://codereview.appspot.com/7801048/

[a...@chromium.org]

Committed as f8914ea2fe3287840c6770a900bfbe5ffea7d9dd

https://codereview.appspot.com/7801048/