306740 - add lint option to command line js/xpcshell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Shane Caraveo]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

The following patch adds a lint command line argument to the standalone js and
xpcshell executables.  This allows them to be used to syntax check a javascript
file without executing it.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Shane Caraveo]

Attachment: add lint to cmdline js interpreters

Comment 2

[Shane Caraveo]

the jsconfig.mk part of the patch can be ignored.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 194560 [details] [diff] [review]
add lint to cmdline js interpreters

> static JSBool reportWarnings = JS_TRUE;
>+static JSBool lintOnly = JS_FALSE;

Rename this to compileOnly -- lint(1) was all about picking fluff off of C
code, back when compilers such as PCC gave almost no warnings.	This option is
not making JS stricter (we have the strict option for that), it's just
compiling without executing.

For the command line option, -C might be best.	-l is out.  Thoughts?

>+            if (!lintOnly) {
>+                (void)JS_ExecuteScript(cx, obj, script, &result);
>+            }

Prevailing style does not overbrace single-line, single-statement then and else
clauses unless the other clause is multiline, or the condition is.  When in
Rome....

You can see examples in the diff, e.g.:

>@@ -260,6 +264,7 @@
>                 else
>                     ok = JS_FALSE;
>             }
>+          }
>             JS_DestroyScript(cx, script);
>         }
>     } while (!hitEOF && !gQuitting);
>@@ -271,7 +276,7 @@

So the if (!lineOnly)/else below shouldn't brace its dependent clauses.

>@@ -571,7 +581,11 @@
>         if (!script) {
>             ok = JS_FALSE;
>         } else {
>-            ok = JS_ExecuteScript(cx, obj, script, &result);
>+            if (!lintOnly) {
>+                ok = JS_ExecuteScript(cx, obj, script, &result);
>+            } else {
>+                ok = JS_TRUE;
>+            }
>             JS_DestroyScript(cx, script);
>         }
>         JS_SetOptions(cx, oldopts);

Thanks for doing this, I thought we had a shell function already for such a
purpose, but I can't find it.  It's probably attached to another bug.

How about another patch, but first I'll ask mrbkap if he has any comments. 
He's going to make a libjsshell.so to unify all the forked code between js.c
and xpcshell.cpp, which will save us all some pain.

/be

Comment 4

[Shane Caraveo]

I'll generate a new patch next week if there are no further comments by then.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194560 [details] [diff] [review]
add lint to cmdline js interpreters

Along with what Brendan pointed out:

>Index: js/src/js.c
>@@ -260,6 +264,7 @@
>                 else
>                     ok = JS_FALSE;
>             }
>+          }
>             JS_DestroyScript(cx, script);

The indentation here is a bit wonky.

>@@ -325,6 +330,7 @@
>           case 'c':
>           case 'f':
>           case 'e':
>+          case 'l':
>           case 'v':
>           case 'S':
>             ++i;

This isn't correct. The only options in this switch statement should be options
that take an argument, so you don't need to add your option here.

>@@ -571,7 +581,11 @@
>+            if (!lintOnly) {
>+                ok = JS_ExecuteScript(cx, obj, script, &result);
>+            } else {
>+                ok = JS_TRUE;
>+            }

I almost feel that this would look better as:

ok = !lintOnly
     ? JS_ExecuteScript(cx, obj, sscript, &result)
     : JS_TRUE;

But I tend to use ?: wrongly.

>Index: js/src/xpconnect/shell/xpcshell.cpp

Basically the same comments from js.c apply here, also.

Let's see a new patch. r- until the comments are addressed. Thanks for doing
this!

Comment 6

[Shane Caraveo]

Attachment: jslint patch

A patch that should address the comments.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 194585 [details] [diff] [review]
jslint patch

Looks good to me -- mrbkap should have a look, and check in.  This can go into
trunk and branch (not part of product builds).

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194585 [details] [diff] [review]
jslint patch

r=mrbkap. Requesting approval since xpcshell is part of the build.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 194585 [details] [diff] [review]
jslint patch

Looks good, thanks!

Shane, how are you guys using this?  Tinderbox-driven testing of Komodo?  If
so, we want a similar thing for our apps' chrome scripts.

/be

Comment 10

[Shane Caraveo]

Testing?  Your funny. ;)

But really, we use it for on the fly syntax checking of js files in Komodo.  You
get pretty green and red squiggly lines under warnings and errors in the editor.
 We tend to prefer external processes for stuff like this.  Before I did this
patch we used to eval an ugly, fragile and often non-functional javascript
wrapper.  I'm basicly getting around to submitting some of our patches in an
effort to avoid working on the plugin layer...

Can either of you commit this to cvs?
Thanks!
Shane

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: a couple of changes

Shane, I made a couple of changes before checking in. Firstly, I used the
ternary (?:) operator instead of the if/else as I mentioned in my comment
above. Secondly, I made -C default to non-interactive mode (i.e., after
processing arguments, the process exits instead of waiting on input) since
quitting from such a shell is sort of weird (you *have* to pass EOF). Since
this is really your patch, I wanted to run the changes by you, first.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194600 [details] [diff] [review]
a couple of changes

Shane says r=him on IRC.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 194600 [details] [diff] [review]
a couple of changes

sr+a=me, this is all good.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in on MOZILLA_1_8_BRANCH and trunk.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #9)
> want a similar thing for our apps' chrome scripts.

(bug 243252)