javascriptlint icon indicating copy to clipboard operation
javascriptlint copied to clipboard

The string 'use strict' on its own line shouldn't cause a warning

Open melloc opened this issue 9 years ago • 8 comments

The string 'use strict' on its own line causes part of the program to enter into strict mode, and changes the semantics of some operations (usually making more things throw errors instead of silently failing). This change prevents warning when it's used in a program.

melloc avatar Apr 01 '16 18:04 melloc

Nice. In general, I believe we haven't made changes to the javascriptlint in this repo except for the build system. I had assumed the original project was abandoned, but I see that there have been another 50 subversion commits since this fork. It may be worth trying to sync up with upstream again. Notably, they ripped out Spidermonkey in favor of a pure-JavaScript parser. (See https://sourceforge.net/p/javascriptlint/code/commit_browser; we branched at revision 302.)

davepacheco avatar Apr 01 '16 18:04 davepacheco

Ah, cool. I'll take a look into what needs to be done to sync with that repo at some point, and see if I can get this change (or a similar one) accepted upstream.

melloc avatar Apr 01 '16 18:04 melloc

Cool. Definitely not saying we can't land this in the meantime, though if we don't get it upstreamed and we do sync up, we'll probably lose it.

davepacheco avatar Apr 01 '16 18:04 davepacheco

@davepacheco I've synced up the repo with the latest changes to the subversion repo, as well as two other changes I made: one to not consider placing functions in an array "unusual", and the other to support the "use strict" string.

melloc avatar May 10 '16 01:05 melloc

Thanks for doing this. Sorry it's taken me a while to look closely at it. I tried to clone it but ran into this:

$ ./build/install/jsl 
Traceback (most recent call last):
  File "./build/install/jsl", line 13, in <module>
    import javascriptlint
  File "/home/dap/javascriptlint/build/install/javascriptlint/__init__.py", line 2, in <module>
    from jsl import main
  File "/home/dap/javascriptlint/build/install/javascriptlint/jsl.py", line 12, in <module>
    import conf
  File "/home/dap/javascriptlint/build/install/javascriptlint/conf.py", line 7, in <module>
    import version
  File "/home/dap/javascriptlint/build/install/javascriptlint/version.py", line 18, in <module>
    version = '0.5.0/%s' % _getrevnum()
  File "/home/dap/javascriptlint/build/install/javascriptlint/version.py", line 13, in _getrevnum
    raise _BuildError('Error running svnversion: %s' % stderr)
NameError: global name '_BuildError' is not defined

There are a few problems here:

  • my git is too old to support -C, so the command fails
  • _BuildError isn't found; it doesn't appear to be part of the repo anywhere
  • we probably shouldn't have a runtime dependency on the VCS anyway

I think @melloc is already working on this. I'm just updating the ticket to reflect the status.

Once this is resolved, I'll be interested to

  • run the new javascriptlint over a bunch of existing code bases that we check with javascriptlint to see how the output differs
  • make sure the Makefile and README are updated with exactly how to use it
  • make sure it's still easy to incorporate into other repos by submodule
  • make sure we've somehow separated our local changes from the ones upstream. I'm not sure yet what's the best way to do this.
  • announce a new version

davepacheco avatar Jun 03 '16 17:06 davepacheco

Thanks for fixing the issue above. On the joyent/manta-marlin repo, where we run it over 54 files, I found two things of note:

  • The new javascriptlint took 24 seconds (compared to 3 seconds for the old one). Definitely sucks, but I'm okay with that, assuming there really is value in keeping sync'd up. (Not having to build binary modules is nice, at least.)
  • The new javascriptlint does not appear to support named function expressions where the function name is used inside the function, like this:
setTimeout(function doSomething() {
        console.log('it works!');
        setTimeout(doSomething, 1000);
}, 1000);

It reports "doSomething" as an undeclared identifier on line 3.

davepacheco avatar Jun 03 '16 17:06 davepacheco

Another issue with the new version: the const keyword seems to produce a warning: missing semicolon before statement warning, which is not produced if you just change the const to a var.

arekinath avatar Jun 13 '16 23:06 arekinath

I've added support for the const keyword, and for introducing the name of a named function expression into the scope of the body of the function. For now, things declared with const are treated as if they are variables. If anyone wants to, it might be a good improvement in the future to warn when assignments are made to things that have been declared const.

I profiled the code with cProfile, and it looks like the majority of the program's time is being spent in the tokenizing code. It's unclear to me how it could be improved without further investigation, but it seems like that is something that can also be investigated in the future.

melloc avatar Jun 22 '16 21:06 melloc