1330006 - Enable eslint for self hosted code

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Tom S [:evilpie]]

Using eslint would ensure that certain errors can't happen and would enforce some general coding style rules. However this is complicated by our use of the preprocessor. We either need to find some way to run it before eslint, or remove all uses of #define, #if etc. from the .js files. All the defines from SelfhostingDefines.h could be put into the global section of eslintrc.

Comment 1

[Till Schneidereit [:till]]

(In reply to Tom Schuster [:evilpie] from comment #0)
> All the defines
> from SelfhostingDefines.h could be put into the global section of eslintrc.

We definitely should avoid having to add things to two places. Can we put the defines somewhere they can be used by the preprocessor and by eslint? Other than that, I think this would be very nice.

Comment 2

[Tom S [:evilpie]]

There is some way to define a pre-processor in eslint: http://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/processors/xbl-bindings.js. I tried adding a new empty processor, but I don't think it was getting executed at all.

Comment 3

[Tom S [:evilpie]]

Attachment: Self-hosting eslint plugin

This plugin "parses" the macro stuff used in our selfhosting code. We can't enable "no-undef", because we would have to add all macro definitions and all the functions defined in SelfHosting.cpp to the globals list. IIRC this isn't actually a big issue in self-hosted code, because accessing undefined variables should throw.

Comment 4

[Tom S [:evilpie]]

Attachment: Fix the single quotes error

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8832936 [details] [diff] [review]
Fix the single quotes error

Review of attachment 8832936 [details] [diff] [review]:
-----------------------------------------------------------------

Ok

Comment 6

[Tom S [:evilpie]]

Attachment: Fix "space-infix-ops"

Comment 7

[Tom S [:evilpie]]

Attachment: Fix various space related errors

Comment 8

[Tom S [:evilpie]]

Attachment: Fix "shorthand" error

Comment 9

[Tom S [:evilpie]]

Attachment: Fix "no-unused-vars"

This is a bit more complicated, because this would make an observable difference, but I am going to trust the comments and assume that these Intl functions are really internal.

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8833614 [details] [diff] [review]
Fix "space-infix-ops"

Review of attachment 8833614 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8833615 [details] [diff] [review]
Fix various space related errors

Review of attachment 8833615 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 8833616 [details] [diff] [review]
Fix "shorthand" error

Review of attachment 8833616 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, nice that we're enforcing this with a lint

Comment 13

[Till Schneidereit [:till]]

Comment on attachment 8833619 [details] [diff] [review]
Fix "no-unused-vars"

Review of attachment 8833619 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80899c8dc2ae
Fix the eslint single quotes error in self-hosted JS. r=till

Comment 15

[Tom S [:evilpie]]

We have a lot of no-redeclare errors for code like:

if (a()) {
  var x = 1;
   ...
  return
}
var x = 2

What should I do about that? Can we just use let and const or is this going to cause performance problems?

Comment 16

[Tom S [:evilpie]]

Ok let's ignore the no-redeclare errors until we can replace those with const/let without performance concerns.

Comment 17

[Tom S [:evilpie]]

Attachment: Fix "no-else-return" and "consistent-return" errors

Comment 18

[Tom S [:evilpie]]

Attachment: Self-hosting eslint plugin

This plugin remove all (C) preprocessor macros like #ifdef #else etc. It's a bit of a hack because we just comment out those lines so we might end up with duplicate. 

#if A
f(1);
#else
f(2);
#endif

produces
f(1);
f(2);

I think this is a good compromise, because it gives us eslint coverage for all code and not just one branch. There is only one case in the whole codebase were this was causing issues. (After stripping the ifdef/else we had two consecutive return statements)

Comment 19

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f31f23c8fa140383791f78bbd37f34add5110bc&selectedJob=74524611

Something I going wrong, most likely something in the no-unused-vars patch, like https://hg.mozilla.org/try/rev/e8f3854d34aa0312af37e8ebf298c97e6465c074#l2.12.

Comment 20

[Till Schneidereit [:till]]

Comment on attachment 8833622 [details] [diff] [review]
Fix "no-else-return" and "consistent-return" errors

Review of attachment 8833622 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: js/src/builtin/Module.js
@@ +230,5 @@
>      let module = this;
>  
>      // Step 5
>      if (GetModuleEnvironment(module) !== undefined)
> +        return undefined;

hrm, this is a stupid rule. A bare `return` *is* a `return undefined`, so requiring the explicit version doesn't make sense. But whatever.

Comment 21

[Till Schneidereit [:till]]

Comment on attachment 8833634 [details] [diff] [review]
Self-hosting eslint plugin

Review of attachment 8833634 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the js/src parts.

::: .eslintignore
@@ +153,5 @@
>  devtools/client/framework/test/code_ugly*
>  devtools/server/tests/unit/babel_and_browserify_script_with_source_map.js
>  devtools/server/tests/unit/setBreakpoint*
>  
> +# Exclude everything but selfhosted JS

Uber-nit: "self-hosted"

::: js/src/builtin/.eslintrc.js
@@ +12,5 @@
> +    "spaced-comment": "off",
> +    "no-lonely-if": "off",
> +    // SpiderMonkey's style doesn't match any of the possible options.
> +    "brace-style": "off",
> +    // Manually defining all the selfhosted methods is a slog.

Maybe add ", and undefined names cause an exception and abort during runtime initialization"?

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/80899c8dc2ae

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d97a94e909
Fix space-infix-ops eslint. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b2f8544656
Fix various space related eslint errors. r=till

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f1d97a94e909
https://hg.mozilla.org/mozilla-central/rev/55b2f8544656

Comment 25

[Tom S [:evilpie]]

I should also mention that my eslint plugin will run for every .js file, so this might be a performance concern?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/874be8845865
Fix eslint no-else-return and consistent-return errors. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c906492f0e
Fix eslint no-unused-vars error. r=till

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 8833634 [details] [diff] [review]
Self-hosting eslint plugin

Review of attachment 8833634 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/lint/eslint/eslint-plugin-mozilla/lib/processors/self-hosted.js
@@ +44,5 @@
> +      for (let message of messages[i]) {
> +        errors.push(message);
> +      }
> +    }
> +    return errors;

Can this be more succinctly written as:

    return Array.prototype.concat.apply([], messages);

Comment 28

[Tom S [:evilpie]]

Attachment: v2 - Fix "shorthand" error

This disables shorthand methods, because those were causing GetUnclonedValue crashes on try.

Comment 29

[Tom S [:evilpie]]

Attachment: Disable one no-unreachable error

This is an ugly workaround, because after stripping the if/else/endif we basically end up with:

return callFunction(..)
return callFunction(..)

This is the remaining eslint error, so I am going to enable eslint for js/src/ afterwards \o/.

Comment 30

[Till Schneidereit [:till]]

Comment on attachment 8834166 [details] [diff] [review]
Disable one no-unreachable error

Review of attachment 8834166 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 31

[Till Schneidereit [:till]]

Comment on attachment 8834165 [details] [diff] [review]
v2 - Fix "shorthand" error

Review of attachment 8834165 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem to be the right patch. It has seemingly unrelated changes in it and introduces a new function shorthand instead of removing any.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/874be8845865
https://hg.mozilla.org/mozilla-central/rev/88c906492f0e

Comment 33

[Tom S [:evilpie]]

Attachment: v3 - Fix "shorthand" error

Sorry, seems like I missed one method while changing this patch. I disabled eslint for that line in Intl.js as well. The other changes are something "shorthand" warns about as well, when you can use {a, b} instead of {a: a, b:b}.

Comment 34

[Till Schneidereit [:till]]

Comment on attachment 8834435 [details] [diff] [review]
v3 - Fix "shorthand" error

Review of attachment 8834435 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I misunderstood the intent of the patch: I thought it was meant to change {shorthand(){}} method definitions to {longhand: function(){}} ones because the former cause problems. It obviously makes sense for it to only deal with eslint issues seeing as how it's attached to this bug ...

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a77e19ab85
Fix eslint shorthand error. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5f5d8b4906
Work around no-unreachable error. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d103004bfc6
Land and enable self-hosted JS eslint plugin. r=till,mossop

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47a77e19ab85
https://hg.mozilla.org/mozilla-central/rev/9b5f5d8b4906
https://hg.mozilla.org/mozilla-central/rev/4d103004bfc6