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

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.

- Add 'ignoreNolint' option.

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

[usrbincc, 2013-03-30 16:45:08]

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.

[arv, 2013-04-02 17:51:33]

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.

[usrbincc, 2013-04-02 18:24:42]

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.

[usrbincc, 2013-04-05 15:51:31]

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.

[arv, 2013-04-05 16:52:45]

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

[usrbincc, 2013-04-05 19:04:35]

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

[arv, 2013-04-05 19:16:42]

LGTM

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

[arv, 2013-04-05 19:29:26]

Committed as f8914ea2fe3287840c6770a900bfbe5ffea7d9dd

[usrbincc, 2013-04-07 01:55:50]

Thanks for the commit. I promise, you'll love traceur's regained
semicolon-checking powers so much that you'll forget about the code
that makes it possible.

  I apologize in advance for the extra contortion coming in my next
  pull request.

Here is where I metaphorically lock the doors and turn the lights out
before leaving the codereview building for the last time.

I'm glad I got the chance to get the codereview experience -- I can
see the strengths and weaknesses of github that much more clearly,
having something to compare it against.

  A github pull request corresponds roughly to a single patch set of
  a codereview issue. I think if github made it possible to deal with
  pull requests the way codereview deals with patch sets, that would
  be the ultimate combination.

    And seriously, github, don't make pieces of the pull request
    suddenly morph and disappear just because someone did a
    'git push -f'! [end rant]

I feel like codereview seems to foster a more careful approach. I know
that constant iteration is still the side to bet on, but I can't help
thinking that this needs to be tempered with periods of more
thoughtfulness.

I'm sure I'll feel nostalgic about github when the time comes, but
we'll cross that bridge when we get to it (when the more frenetic
successor to github takes over).