New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

irregular error with comma operator #56

Closed
tbranyen opened this Issue Feb 23, 2011 · 13 comments

Comments

Projects
None yet
@tbranyen
Copy link

tbranyen commented Feb 23, 2011

In my project within the following code, the last line errors out.

  var args = Array.prototype.slice.call( arguments );
  args[0] = error( args[0] );

  callback && callback.apply( commit, (args.push( commit ), args) );

Line 46 callback && callback.apply( commit, (args.push( commit ), args) );
Expected ')' to match '(' from line 46 and instead saw ','.

This is, however, completely valid use of the , operator.

@Raynos

This comment has been minimized.

Copy link
Contributor

Raynos commented Feb 23, 2011

It is indeed valid but it's still hard to read. The javascript community in general seems to avoid using , to have multiple expressions in one expression statement.

@tbranyen

This comment has been minimized.

Copy link

tbranyen commented Feb 23, 2011

Well I guess the question now becomes is JSHint going off opinionated assumptions or what actually breaks code?

It is designed to detect errors that actually break your code while 
skipping things that, according to Crockford, “are known to contribute 
mistakes in projects”.

The comma operator is described in detail in the JavaScript Definitive Guide and is used quite regularly by myself for if statements and function calls and loops.

I have started a patch that passes all existing unit tests, but fails on a few I have created.

@Raynos

This comment has been minimized.

Copy link
Contributor

Raynos commented Feb 23, 2011

The thing is. It's fine inside an if statement or a loop but in function calls the , operator is over loaded to represent both argument separators and a expression made of multiple expressions.

One could fathom it's likely to be a misplaced bracket in a function call (In my code that would always be the case).

There defiantly should be an option to check for this. Whether this should default to on or off is up for argument.

@paulirish

This comment has been minimized.

Copy link
Member

paulirish commented Feb 23, 2011

i think it'd be fine to augment the parser to identify this use, but i wouldnt leave it OK as a default.

@valueof

This comment has been minimized.

Copy link
Member

valueof commented Feb 23, 2011

This will be a good addition to the boss option. Marking as options and approved.

@Skalman

This comment has been minimized.

Copy link

Skalman commented Mar 15, 2011

Please don't put this in the boss option! It's much better to have multiple options, allowing for more fine-grained preferences. (I would use some of the current boss options, and ignore others)

@michaelficarra

This comment has been minimized.

Copy link

michaelficarra commented Mar 21, 2011

Currently, the comma operator is causing JSHint to stop processing my code. That definitely shouldn't happen, whether it wants to provide a notice or not.

@dandean

This comment has been minimized.

Copy link

dandean commented Jun 17, 2011

I'd also love to see this. Here's my use case:

function Model(data, block) {
  // ...
}

Model.create = function(base, options) {
  // If options is falsy, base wasn't provided so args need shuffling.
  options || (options = base, base = Model); // <-- massive amount of JSHint complaining here!
  console.log(base, options);
  // ...
};

var User = Model.create({ /* options */ });
// -> logs Model, options

var Admin = Model.create(User, { /* options */ });
// -> logs User, options

Yes, I know there are a million other ways to do this, but I really like this syntax.

@timmywil

This comment has been minimized.

Copy link

timmywil commented Aug 22, 2011

Enter this into http://jshint.com:

var channels = [{ 'one': 'channel' }];
for ( var i = 0, channel, len = channels.length; channel = channels[i], i < len; i++ ) {
    // Doing stuff
}

The comma operator breaks jshint and makes it unable to continue.

@goatslacker

This comment has been minimized.

Copy link
Member

goatslacker commented Nov 4, 2011

CoffeeScript lexer also has an error with the following:

var _ref, count, starts, compact, last;
_ref = test, count = _ref.count, starts = _ref.starts, compact = _ref.compact, last = _ref.last;
@wavded

This comment has been minimized.

Copy link

wavded commented Feb 2, 2012

getting an error with comma operator as well, see this issue for details:

https://github.com/jshint/node-jshint/issues/78

@valueof

This comment has been minimized.

Copy link
Member

valueof commented Mar 5, 2012

@mhart

This comment has been minimized.

Copy link

mhart commented May 16, 2013

I'm still seeing this error in valid JS, cf #1083

jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014

Better support for comma operator.
Before when JSHint was parsing expressions inside the prefix parens
it allowed for only one expression. This patch changes the function
to allow for multiple comma-separated expressions. This helps to deal
with tons of false-positive messages that we were spitting out before
(check out the reports for lodash and prototype.js for example).

Closes jshintGH-56.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment