Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Improve ASI support #950

Closed
valueof opened this Issue · 7 comments

5 participants

@valueof
Owner

Currently, the asi option in JSHint simply discards any warnings about missing semicolons. This leads to JSHint overlooking real bugs and generating false positives in some weird cases.

Examples

Example of a real bug that will be overlooked when asi option is turned on:

function test() {
  return
    { a: 1 }
}

Example of JSHint generating false positives:

var a = { b: 1 }
delete a.b

What needs to be done

JSHint should not generate any warnings about missing semicolon unless the new line is followed by the following symbols: (, [, +, -. In addition, JSHint should warn about return statements that are followed by a newlines that contain expressions that don't make sense on their own lines (like a string, or an object).

This bug consolidates #930 and #877.

There's bounty on this bug

You can get paid to fix this bug! See our page on BountySource for more info.

@guyzmo guyzmo referenced this issue from a commit in guyzmo/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) 4255c6b
This was referenced
@guyzmo guyzmo referenced this issue from a commit in guyzmo/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) f3d3a0f
@guyzmo guyzmo referenced this issue from a commit in guyzmo/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) 0918297
@guyzmo guyzmo referenced this issue from a commit in guyzmo/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) e516fa3
@guyzmo guyzmo referenced this issue from a commit in guyzmo/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) 55df77a
@guyzmo guyzmo referenced this issue from a commit in guyzmo/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) 1394629
@guyzmo guyzmo referenced this issue from a commit in guyzmo/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) 2980237
@guyzmo

done

@valueof
Owner

Landed. Thanks!

@valueof valueof closed this
@TehShrike

I observed this issue still occurring in 1.1.0. The code:

var a = { b: 1 }
delete a.b

Results in:

E033: Expected an operator and instead saw 'delete'.
@rwaldron
Owner

Try 2.5.2

@jugglinmike jugglinmike referenced this issue from a commit in jugglinmike/jshint
@guyzmo guyzmo updated automatic comma insertion support (cf #950) 7eb0036
@bob-eel

(new here)
I'm using v2.6.3 with rhino, asi=true, and this kind of code doesn't wake jshint up:


function a() {
    var b = 3 return 5
}

in other terms, it looks like every single semicolon warning is being silenced; does jshint treat newlines as if they weren't there at all?

just like the purportedly fixed bug of above, my version doesn't always warn me correctly against this type of thing:


var a = b
[3].foo() ////nothing :(

var c = d
('a'+'b').foo() ////nothing :(

var e = f
+g ////Bad line breaking before '+'. ok

var h = i
-g ////Bad line breaking before '-': ok

var j = k
/qwe/.foo() ////Bad line breaking before '/'.

////yet it goes on ignoring the first / and "throws" an error telling me
////that '.foo' isn't the indentifier it expected;
////it however doesn't have any problem, first '/' excepted, with this type of thing:

var foo = bar
/aze/4

////is it its normal behavior?

anyhow, quite annoying for someone who uses asi like me; quite absurd too that the two first asi exceptions (by far, more common than the three others) don't raise any warning :/

EDIT: checked the same stuff without asi=true, it happens that jshint treats the above expressions like any actual interpreter, ie without raising warnings:

var a = b
[3].foo()
////is treated just like:
var a = b[3].foo()

thus: it's truly missing some important thing wrt helping those using the asi syntax.

i'm starting to think that the above errors raised by +, -, /, have actually nothing to do with asi: it just happens that jshint doesn't recognize the two unary operators + and -, and visibly, the use of regexps with the slash syntax.

i'd say there's some base for quite a little more improvement...

@guyzmo

I do not understand what's your issue with:

function a() {
    var b = 3 return 5
}

because it's just totally invalid, and won't compile anyway! But it does not look to me like a failure for asi, as I see no asi would happen here.

About the following miss:

var a = b
[3].foo() ////nothing :(

you're right it might need to show off an error, as the code won't compile and is an obvious miss. Though, it should not be warning against asi there, but warning it because of the following example to consider:

var i = 3;
var b = [];
var a = b[i] = [1];

vs

var i = 3;
var b = [];
var a = b
[i] = [1];

both are legit JS, but the carriage return on second one could be an unwanted one, changing the semantics of the snipped.

The same logic applies to:

var c = d
('a'+'b').foo()

versus

var c = d('a'+'b').foo()

All in all, it's better to warn too much about possible asi problems, and enforce the user to insert them wherever it could be a possible ambiguity.

Anyway, you shalln't rely on automagic semi-column insertion, but instead insert them to make your code as precise as possible, on then use that warning to get attention where there could be some errors. It's actually part of Crockford's Javascript: The Good Parts piece, which led to the writing of jslint, jshint ancestor ;-)

@guyzmo

though because that issue has been closed a year and half, maybe shall we open a new issue to precise those scenarios?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.