Turn on eslint no-undef rule in toolkit/mozapps/extensions

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(6 attachments)

Turning this rule on as a testcase to see just how bad it is. Part of this will be updating our plugins and configs to define more globals automatically, the rest will be fixing the bugs that are uncovered.
While working on turning on no-undef I discovered that the various rules we
have for defining globals are a little inconsistent in whether the files they
load recurse through import-globals-from directives and none of them imported
eslint globals directives.

I think we're better off putting all this global parsing code in a single place
rather than spread across multiple rules. Have one rule to turn it on for
parsed files and one function to load globals from other files and make them
share most of the code so we won't get inconsistent. If we find us needing to
turn on/off individual features we can figure out a way to do that in the
future.

This patch does that, the globals.js file does all global parsing with a shared
object that receives events from the AST, either through from an ESlint rule
or from a simple AST walker using estraverse.

Review commit: https://reviewboard.mozilla.org/r/34199/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34199/
Attachment #8717535 - Flags: review?(pbrosset)
To properly lint XBL files we need to support things like import-globals-from
and other ESlint comment directives so we have to pass comments through to the
code blocks that ESlint parses. Unfortunately the way the XBL processor works
now is by passing a separate code block for every method/property/etc. in the
XBL and ESlint doesn't retain state across the blocks so we would have to prefix
every block with every comment. Instead this change makes us output just a
single block that roughly looks like this:

<comments>
var bindings = {
  "<binding-id>": {
    <binding-part-name>: function() { ... }
  }
}

This has some interesting bonuses. Defining the same ID twice will cause a lint
failure. Same for the same field in a binding. The line mapping is a little
harder and there are still a few lines that won't map directly back to the
original file but they should be rare cases. The only downside is that since
some bindings have the same binding declared differently for different platforms
we have to exclude those from linting for now.

Review commit: https://reviewboard.mozilla.org/r/34201/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34201/
Attachment #8717536 - Flags: review?(mratcliffe)
This adds more of the scripts that browser.js relies on and also makes
browser-chrome head files import the browser.js globals.

The MOZ_JSDOWNLOADS block in contentAreaUtils only seems to hide a single
function, I don't see any need to keep hiding that now we're on by default.

Review commit: https://reviewboard.mozilla.org/r/34203/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34203/
Attachment #8717537 - Flags: review?(felipc)
This defines a few additional globals but also turns on the browser environment
for everything in browser and toolkit. This may lead to some false negatives
but we have lots of code that runs in a browser context so in the name of
getting rules turned on I think this is a useful step.

Review commit: https://reviewboard.mozilla.org/r/34205/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34205/
Attachment #8717538 - Flags: review?(felipc)
xpcshell tests used to use head_*.js files so this adds those for global
discovery.

Review commit: https://reviewboard.mozilla.org/r/34207/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34207/
Attachment #8717539 - Flags: review?(pbrosset)
Mostly just declaring globals that Cu.imports defines but there are some actual
bugs here that have been fixed as well as one test that just never ran because
of a hidden exception.

Review commit: https://reviewboard.mozilla.org/r/34209/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34209/
Attachment #8717540 - Flags: review?(rhelmer)
Comment on attachment 8717540 [details]
MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer

https://reviewboard.mozilla.org/r/34209/#review30889

::: toolkit/mozapps/extensions/AddonManager.jsm:981
(Diff revision 1)
> +      /*globals RemotePages*/

I've seen both this (implicitly importing the exported globals from a jsm) and alternatively:
`let {RemotePages} = Cu.import("resource://gre/modules/RemotePageManager.jsm", {});`

Not a deal-breaker for me as long as it's made explicit in some way (such as the /*globals*/ comment for eslint), just wondering if there's an advantage to not using an explicit `let`/`const`/etc. binding.

::: toolkit/mozapps/extensions/content/extensions.xml:12
(Diff revision 1)
> +<!-- import-globals-from extensions.js -->

So cool we can do this in XUL files too
Attachment #8717540 - Flags: review?(rhelmer) → review+
Comment on attachment 8717537 [details]
MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe

https://reviewboard.mozilla.org/r/34203/#review30925
Attachment #8717537 - Flags: review?(felipc) → review+
Comment on attachment 8717538 [details]
MozReview Request: Bug 1245916: Add additional default globals. r=felipe

https://reviewboard.mozilla.org/r/34205/#review30927
Attachment #8717538 - Flags: review?(felipc) → review+
Comment on attachment 8717535 [details]
MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset

https://reviewboard.mozilla.org/r/34199/#review31177

Just a few comments and questions. Overall looks like a really good cleanup.

::: .eslintrc:7
(Diff revision 1)
> -    "mozilla/components-imports": 1,
> +    "mozilla/import-globals": 1,

Did I miss something or are Cu.import, loader, XPCOMUtils things not going to be recognized as globals now?

::: testing/eslint-plugin-mozilla/lib/globals.js:17
(Diff revision 1)
> +const globalCache = new Map();

nit: please add a line comment explaining what this globalCache is used for.

::: testing/eslint-plugin-mozilla/lib/globals.js:20
(Diff revision 1)
> + * An object that returns found globals for given AST node types. Each property

s/Eeach property/Each prototype property

Just makes it easier when reading, you know which property you're talking about.

::: testing/eslint-plugin-mozilla/lib/globals.js:36
(Diff revision 1)
> +    if (match) {

We've taken the habit, in the devtools code base, to invert these conditions and early return, so that the "actual" code wouldn't be indented, and more easily readable.
Up to you.

```
if (!match) {
  return [];
}

value = match[1].trim();

let filePath = value;

...
```

::: testing/eslint-plugin-mozilla/lib/globals.js:39
(Diff revision 1)
> +      let filePath = value;

Why set `value` again here?
Why not just `let filePath = match[1].trim();`

::: testing/eslint-plugin-mozilla/lib/globals.js:92
(Diff revision 1)
> +      // comment directives

Isn't there a way to import an eslint helper here that would do this for us?

::: testing/eslint-plugin-mozilla/lib/globals.js:95
(Diff revision 1)
> +        let match = /^globals?\s+(.+)$/.exec(value);

eslint accepts both `globals` and `global`

::: testing/eslint-plugin-mozilla/lib/globals.js:100
(Diff revision 1)
> +          for (let global of value.split(/\s*,\s*/)) {

Turns out eslint also allows globals to be separated by a space character.
Attachment #8717539 - Flags: review?(pbrosset) → review+
Comment on attachment 8717539 [details]
MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset

https://reviewboard.mozilla.org/r/34207/#review31183

::: testing/eslint-plugin-mozilla/lib/helpers.js:311
(Diff revision 1)
>     *         e.g. helpers.getIsBrowserMochitest(this)

s/getIsBrowserMochitest/getIsHeadFile

::: testing/eslint-plugin-mozilla/lib/helpers.js:327
(Diff revision 1)
> +   *         e.g. helpers.getIsBrowserMochitest(this)

s/getIsBrowserMochitest/getIsXpcshellTest
Comment on attachment 8717536 [details]
MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker

https://reviewboard.mozilla.org/r/34201/#review31193
Attachment #8717536 - Flags: review?(mratcliffe) → review+
https://reviewboard.mozilla.org/r/34199/#review31177

> Did I miss something or are Cu.import, loader, XPCOMUtils things not going to be recognized as globals now?

No they all still apply through GlobalsForNode.prototype.ExpressionStatement.
https://reviewboard.mozilla.org/r/34199/#review31177

> Turns out eslint also allows globals to be separated by a space character.

I've just copied the function from eslint.js for this.
https://reviewboard.mozilla.org/r/34199/#review31239

::: testing/eslint-plugin-mozilla/lib/globals.js:92
(Diff revision 1)
> +      // comment directives

Not that I can see, it all happens at https://github.com/eslint/eslint/blob/master/lib/eslint.js#L270 and the only way to really get there is to just do a full eslint pass on the file. I guess that's an option but will involve more work than we need for now.

::: testing/eslint-plugin-mozilla/lib/globals.js:95
(Diff revision 1)
> +        let match = /^globals?\s+(.+)$/.exec(value);

The ? after the "s" allows for that.
https://reviewboard.mozilla.org/r/34209/#review30889

> I've seen both this (implicitly importing the exported globals from a jsm) and alternatively:
> `let {RemotePages} = Cu.import("resource://gre/modules/RemotePageManager.jsm", {});`
> 
> Not a deal-breaker for me as long as it's made explicit in some way (such as the /*globals*/ comment for eslint), just wondering if there's an advantage to not using an explicit `let`/`const`/etc. binding.

let bindings are probably better since they work even outside of eslint parsing. They don't have the same effect though. It doesn't actually matter here but inside a block the declaration would only be for the block rather than globally.

Mostly for now I don't want to get into changing those that need it to let bindings because then I'd be tempted to change them all to let bindings and those are cycles I'd rather not spin right now!
Comment on attachment 8717535 [details]
MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34199/diff/1-2/
Attachment #8717535 - Attachment description: MozReview Request: Bug 1245916: Unify eslint global discovery rules. r?pbro → MozReview Request: Bug 1245916: Unify eslint global discovery rules. r?pbrosset
Attachment #8717535 - Flags: review?(pbrosset)
Comment on attachment 8717536 [details]
MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34201/diff/1-2/
Attachment #8717536 - Attachment description: MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r?miker → MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker
Attachment #8717537 - Attachment description: MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r?felipe → MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe
Comment on attachment 8717537 [details]
MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34203/diff/1-2/
Comment on attachment 8717538 [details]
MozReview Request: Bug 1245916: Add additional default globals. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34205/diff/1-2/
Attachment #8717538 - Attachment description: MozReview Request: Bug 1245916: Add additional default globals. r?felipe → MozReview Request: Bug 1245916: Add additional default globals. r=felipe
Comment on attachment 8717539 [details]
MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34207/diff/1-2/
Attachment #8717539 - Attachment description: MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r?pbro → MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset
Comment on attachment 8717540 [details]
MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34209/diff/1-2/
Attachment #8717540 - Attachment description: MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r?rhelmer → MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer
Attachment #8717535 - Flags: review?(pbrosset) → review+
Comment on attachment 8717535 [details]
MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset

https://reviewboard.mozilla.org/r/34199/#review31399

Looks good thanks. And thanks for comment 13, I understand now.
Comment on attachment 8717535 [details]
MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34199/diff/2-3/
Attachment #8717535 - Attachment description: MozReview Request: Bug 1245916: Unify eslint global discovery rules. r?pbrosset → MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset
Comment on attachment 8717536 [details]
MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34201/diff/2-3/
Comment on attachment 8717537 [details]
MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34203/diff/2-3/
Comment on attachment 8717538 [details]
MozReview Request: Bug 1245916: Add additional default globals. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34205/diff/2-3/
Comment on attachment 8717539 [details]
MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34207/diff/2-3/
Comment on attachment 8717540 [details]
MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34209/diff/2-3/
And backed out the rest in https://hg.mozilla.org/integration/fx-team/rev/8bd1a25ac261 for browser-chrome timeouts in several chunks, and devtools in browser_responsiveui.js.
Comment on attachment 8717535 [details]
MozReview Request: Bug 1245916: Unify eslint global discovery rules. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34199/diff/3-4/
Comment on attachment 8717536 [details]
MozReview Request: Bug 1245916: XBL bindings should support global declarations in comments. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34201/diff/3-4/
Comment on attachment 8717537 [details]
MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34203/diff/3-4/
Comment on attachment 8717538 [details]
MozReview Request: Bug 1245916: Add additional default globals. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34205/diff/3-4/
Comment on attachment 8717539 [details]
MozReview Request: Bug 1245916: Import more head files for xpcshell tests. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34207/diff/3-4/
Comment on attachment 8717540 [details]
MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34209/diff/3-4/
Comment on attachment 8717537 [details]
MozReview Request: Bug 1245916: Add additional browser window scripts to eslint globals. r=felipe

I had to fix a couple of things so please review these small changes: https://reviewboard.mozilla.org/r/34203/diff/3-4/
Attachment #8717537 - Flags: review+ → review?(felipc)
Comment on attachment 8717540 [details]
MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer

A couple of minor fixes here: https://reviewboard.mozilla.org/r/34209/diff/3-4/
Attachment #8717540 - Flags: review+ → review?(rhelmer)
Attachment #8717537 - Flags: review?(felipc) → review+
Comment on attachment 8717540 [details]
MozReview Request: Bug 1245916: Turn on no-undef in toolkit/mozapps/extensions. r=rhelmer

https://reviewboard.mozilla.org/r/34209/#review31817
Attachment #8717540 - Flags: review?(rhelmer) → review+
You need to log in before you can comment on or make changes to this bug.