[dojo-contributors] No Side Effects Rejection

Adam Peller peller at gmail.com
Mon May 11 14:28:59 EDT 2009


Just so you know, none of this is "jslint".  We're currently running
everything through Rhino with the "-strict" switch, which implements
only a handful of checks, some of them reminiscent of jslint, but no
where near as comprehensive (or offensive).  We started out just
running checkins through Rhino's parser to reject really obvious
stuff, like typos which would cause syntax errors.  I turned on the
strict mode mostly hoping it would catch the dreaded trailing comma,
as advertised.  Unfortunately, it only catches them in literals, not
in code :-(

Yeah, I've been scratching my head over the side-effects message too
-- sometimes it doesn't seem to make any sense.  Anyhow, this is why
we have an override with "!strict"  Until someone finds something
better, I think it's doing more good than harm to the codebase.

Also, FWIW, there are some really nasty examples out there where a
stray semi or return can actually alter the meaning of JavaScript.  I
don't know if this particular rule has anything to do with that, but
putting a semi after a code block simply shouldn't be allowed.

-Adam


On Mon, May 11, 2009 at 12:08 PM, Peter E Higgins <dante at dojotoolkit.org> wrote:
> Mike Wilcox wrote:
>> You are suggesting it's a format violation which is fine, but that's
>> not what was throwing errors.
>>
>>
> I know, but my comment was that the issue was moot because the patch
> should have been rejected based on the style guidelines alone.  The
> warning was just a warning, and not an error, so it is just lint talking
> to you. Our lint is clearly more strict than Komodo/Textmate/etc, and I
> agree it is overly aggressive.
>
> The issue is the ; is NOT required, and lint is complaining the "next
> line" has no side effect. For example, the following commit attempt
> failed. The number of Errors is equal to the number of excessive ;'s
>
> Index: FileUploader.js
> ===================================================================
> --- FileUploader.js     (revision 17515)
> +++ FileUploader.js     (working copy)
> @@ -18,7 +18,7 @@
>
>        var mixin = function(o1,o2){
>                // custom mixin
> -               var o = {}, nm;
> +               var o = {}, nm;;;;;;
>                for(nm in o1){
>                        if(dojo.isObject(o1[nm])){
>                                o[nm] = mixin({}, o1[nm]);
> sugr:form dante$ svn commit -m "refs #7340 - this is a test"
> Sending        form/FileUploader.js
> Transmitting file data .svn: Commit failed (details follow):
> svn: MERGE request failed on '/src/dojox/trunk/form'
> svn: 'pre-commit' hook failed with error output:
> Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
> Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
> Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
> Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
> Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
> ERROR: Syntax errors. Checkin aborted. To override strict mode errors,
> use "!strict" in comment.
>
> sugr:form dante$
>
>> As you proved though, it was the semi-colon in the wrong place. I
>> guess I can see "return" being invalid compared to "return;"
>>
>>
> I think it would have been fine if it were:
>
> if(this._formNode){ return }
>
> (note the lack of a ; after the }, which isn't an object or a var, its
> the 'if' block)
>
> Another interesting quirk here (don't quote me) return's cannot have a
> newline on the 'return' line:
>
> function bar(a){
>    var b = 1;
>    return
>       a || b;
> }
>
> var x = bar(2); // expects 2
> var y = bar(); // expects 1
>
> console.log(x, y); // undefined, undefined
>
> moving a || b to " return a || b; " works fine.
>
> As it is all over our code, this is completely valid:
>
> return someTruthy ?
>    trueValue :
>    falseValue;
>
>> And as it turned out, a semi-colon after my try-catch was breaking
>> embed.Flash:
>>
>> try{..}catch(e){...); <----- that semi colon wouldn't allow a commit!!
>>
>>
> Yep, same case as above.
>> I guess I would have to blame the validator for the improper message
>> on that second one. is it reading that as two statements?
>>
>>
> It is reading it as a line of code ";" that "does nothing", and "has no
> side effects".
>
> Please look at Shane's output, and ensure all modules you (all of us)
> own pass the basic style guideline rules in place and already being
> spotted safely.
>
> http://skynet.ie/~sos/misc/dojoCheckstyle/util/buildscripts/checkstyleReport.html
>
> Regards
>
> --
> Peter E Higgins
> Dojo Project Lead : http://dojotoolkit.org
>
> _______________________________________________
> dojo-contributors mailing list
> dojo-contributors at mail.dojotoolkit.org
> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>


More information about the dojo-contributors mailing list