Enable eslint for self hosted code

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(9 attachments, 2 obsolete attachments)

(Assignee)

Description

10 months ago
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.
(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.
(Assignee)

Comment 2

10 months ago
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.
(Assignee)

Comment 3

10 months ago
Created attachment 8832912 [details] [diff] [review]
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.
(Assignee)

Comment 4

10 months ago
Created attachment 8832936 [details] [diff] [review]
Fix the single quotes error
Attachment #8832936 - Flags: review?(till)
Comment on attachment 8832936 [details] [diff] [review]
Fix the single quotes error

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

Ok
Attachment #8832936 - Flags: review?(till) → review+
(Assignee)

Updated

10 months ago
Assignee: nobody → evilpies
(Assignee)

Updated

10 months ago
Keywords: leave-open
(Assignee)

Comment 6

10 months ago
Created attachment 8833614 [details] [diff] [review]
Fix "space-infix-ops"
Attachment #8833614 - Flags: review?(till)
(Assignee)

Updated

10 months ago
Attachment #8833614 - Attachment description: Fixe "space-infix-ops" → Fix "space-infix-ops"
(Assignee)

Comment 7

10 months ago
Created attachment 8833615 [details] [diff] [review]
Fix various space related errors
Attachment #8833615 - Flags: review?(till)
(Assignee)

Comment 8

10 months ago
Created attachment 8833616 [details] [diff] [review]
Fix "shorthand" error
Attachment #8833616 - Flags: review?(till)
(Assignee)

Comment 9

10 months ago
Created attachment 8833619 [details] [diff] [review]
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.
Attachment #8833619 - Flags: review?(till)
Comment on attachment 8833614 [details] [diff] [review]
Fix "space-infix-ops"

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

r=me
Attachment #8833614 - Flags: review?(till) → review+
Comment on attachment 8833615 [details] [diff] [review]
Fix various space related errors

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

r=me
Attachment #8833615 - Flags: review?(till) → review+
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
Attachment #8833616 - Flags: review?(till) → review+
Comment on attachment 8833619 [details] [diff] [review]
Fix "no-unused-vars"

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

r=me
Attachment #8833619 - Flags: review?(till) → review+

Comment 14

10 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80899c8dc2ae
Fix the eslint single quotes error in self-hosted JS. r=till
(Assignee)

Comment 15

10 months ago
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?
(Assignee)

Comment 16

10 months ago
Ok let's ignore the no-redeclare errors until we can replace those with const/let without performance concerns.
(Assignee)

Comment 17

10 months ago
Created attachment 8833622 [details] [diff] [review]
Fix "no-else-return" and "consistent-return" errors
Attachment #8833622 - Flags: review?(till)
(Assignee)

Comment 18

10 months ago
Created attachment 8833634 [details] [diff] [review]
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)
Attachment #8833634 - Flags: review?(till)
Attachment #8833634 - Flags: review?(mratcliffe)
(Assignee)

Updated

10 months ago
Attachment #8832912 - Attachment is obsolete: true
(Assignee)

Comment 19

10 months ago
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 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.
Attachment #8833622 - Flags: review?(till) → review+
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"?
Attachment #8833634 - Flags: review?(till) → review+

Comment 22

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/80899c8dc2ae

Comment 23

10 months ago
Pushed by evilpies@gmail.com:
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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1d97a94e909
https://hg.mozilla.org/mozilla-central/rev/55b2f8544656
(Assignee)

Comment 25

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

Comment 26

10 months ago
Pushed by evilpies@gmail.com:
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
(Assignee)

Updated

10 months ago
Attachment #8833634 - Flags: review?(mratcliffe) → review?(dtownsend)
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);
Attachment #8833634 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 28

10 months ago
Created attachment 8834165 [details] [diff] [review]
v2 - Fix "shorthand" error

This disables shorthand methods, because those were causing GetUnclonedValue crashes on try.
Attachment #8834165 - Flags: review?(till)
(Assignee)

Comment 29

10 months ago
Created attachment 8834166 [details] [diff] [review]
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/.
Attachment #8834166 - Flags: review?(till)
(Assignee)

Updated

10 months ago
Attachment #8834166 - Attachment description: Disable one no-unreachable errors → Disable one no-unreachable error
Comment on attachment 8834166 [details] [diff] [review]
Disable one no-unreachable error

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

r=me
Attachment #8834166 - Flags: review?(till) → review+
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.
Attachment #8834165 - Flags: review?(till)

Comment 32

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/874be8845865
https://hg.mozilla.org/mozilla-central/rev/88c906492f0e
(Assignee)

Comment 33

10 months ago
Created attachment 8834435 [details] [diff] [review]
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}.
Attachment #8834165 - Attachment is obsolete: true
Attachment #8834435 - Flags: review?(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 ...
Attachment #8834435 - Flags: review?(till) → review+

Comment 35

10 months ago
Pushed by evilpies@gmail.com:
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
(Assignee)

Updated

10 months ago
Keywords: leave-open

Comment 36

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47a77e19ab85
https://hg.mozilla.org/mozilla-central/rev/9b5f5d8b4906
https://hg.mozilla.org/mozilla-central/rev/4d103004bfc6
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1344932
You need to log in before you can comment on or make changes to this bug.