Comment 1 Michael Rennie 2014-02-11 21:39:23 EST

(In reply to Mark Macdonald from comment #0)
 
> Expected to see a "missing semicolon" warning on the checkItem( .. ) call,
> but I don't see it.

The rule is only set to run for VariableDeclaration and ExpressionStatement.

So in the snippet there are two missing warnings:
1. for the ReturnStatement
2. for the FunctionDeclaration 'f'

The rule needs to be set to run for any node kind that can (and should) be terminated by a semicolon. We could add support for it to check BlockStatement, but then we would have to add in a bunch of checks for blocks where a semicolon is not needed or not allowed (if statements for example)

Comment 2 Michael Rennie 2014-02-11 22:03:04 EST

Expanded snippet showing more missing warnings:

var checkItem = function() {};
function f() {
	return checkItem("a", 1, 2)
}
 
var obj = {
	of: function() {
		return {}
	},
	of1: function() {
		return checkItem()
	},
	of2: function() {
		return 'foo'
	},
	of3: function() {
		return 1
	},
	of4: function() {
		function inner() {
			
		}
		return inner()
	}
}

function f2() {
	return {}
}

function f3() {
	return 'foo'
}

function f4() {
	return 2
}

every return statement and function decl are not marked

Comment 3 Michael Rennie 2014-02-11 22:55:09 EST

Created attachment 239840 [details]
fix

The patch makes all of the mentioned snippets properly report missing semicolons + includes mocha test coverage for the new support.

Comment 4 Michael Rennie 2014-02-12 11:07:04 EST

Comment on attachment 239840 [details]
fix

This patch is bogus. It ends up marking just about everything in your file. The problem was two-fold:
1. we don't actually need to check the call expression node
2. the ranged tokens for a FunctionDeclaration *do not* include the semi colon, regardless of it being there or not. In this case the AST holds the semi colon in an EmptyStatement as a sibling node to the decl

I have a mostly fixed up version I'll post right away

Comment 5 Mark Macdonald 2014-02-12 11:45:05 EST

(In reply to Michael Rennie from comment #1)

> So in the snippet there are two missing warnings:
> 1. for the ReturnStatement
> 2. for the FunctionDeclaration 'f'

Whoops, I missed your comment earlier. Case #2 is working as expected: there's no warning because FunctionDeclarations do not end with a semicolon. 

You can add semicolons if you want, but they're superfluous and will be parsed as EmptyStatements. Indeed, some day in the glorious future I would expect to see "Unnecessary semicolon" warnings on code like this:
> function f() {
> }; // unnecessary semicolon

Comment 6 Michael Rennie 2014-02-12 14:33:50 EST

Created attachment 239879 [details]
fix

This patch actually works as intended. Added more tests to the other new ones.

Comment 7 Michael Rennie 2014-02-12 14:56:42 EST

(In reply to Mark Macdonald from comment #5)

> You can add semicolons if you want, but they're superfluous and will be
> parsed as EmptyStatements. Indeed, some day in the glorious future I would
> expect to see "Unnecessary semicolon" warnings on code like this:
> > function f() {
> > }; // unnecessary semicolon

If we think in the future we want to warn to *not* add a semicolon on a function dec, then I should take the warning out of the fix.

Comment 8 Michael Rennie 2014-02-12 15:33:31 EST

Created attachment 239880 [details]
update

Updates the fix to not report missing semicolons on function decls

Comment 9 Michael Rennie 2014-02-12 15:37:51 EST

(In reply to Michael Rennie from comment #7)
> (In reply to Mark Macdonald from comment #5)
> 
> > You can add semicolons if you want, but they're superfluous and will be
> > parsed as EmptyStatements. Indeed, some day in the glorious future I would
> > expect to see "Unnecessary semicolon" warnings on code like this:
> > > function f() {
> > > }; // unnecessary semicolon
> 
> If we think in the future we want to warn to *not* add a semicolon on a
> function dec, then I should take the warning out of the fix.

opened bug 428040 to check for unnecessary semicolons

Comment 10 Mark Macdonald 2014-02-13 13:18:16 EST

Looks OK +1

Comment 11 Michael Rennie 2014-02-13 13:53:04 EST

Pushed fix + tests to: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=e95c257eb4e6ba87cb22e2a189ba99fe54b54978

Comment 12 Mark Macdonald 2014-02-13 13:55:36 EST

Some of the tests have unnecessary semicolons after a FunctionDeclaration, eg
> var topic = "function f() {return f();};";
>                                        ^ not necessary 
> var topic = "function f2() {return {}};";
>                                       ^ not necessary

but this is a minor nit, doesn't affect the correctness

Comment 13 Michael Rennie 2014-02-21 10:56:46 EST

*** Bug 425234 has been marked as a duplicate of this bug. ***