Closed Bug 1456686 Opened 6 years ago Closed 6 years ago

Treat unused explicit imports (ChromeUtils.defineLazyGetter, ...) as errors

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

3 Branch
enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(2 files)

At the moment, symbols defined by our custom global rules are ignored for the sake of the no-unused rule, which means we have a bunch of unused lazy imports across the tree. This should be pretty easy to deal with in the eslint plugin.

Unused ChromeUtils.import() calls are a bigger problem, since they're not lazy, but they're harder to deal with. I intend to change that API so that its callers destructure the values they're interested in from the return value rather than having it implicitly assign them, so I'm not going to deal with it right now.
Comment on attachment 8970787 [details]
Bug 1456686: Part 1 - Fix unused and shadowed explicit imports.

https://reviewboard.mozilla.org/r/239546/#review245264

Kris, thank you for fixing this. I had a brief look a while ago and could figured it out, obviously I missed the explicit option, so it is great to see these patches and the tidy up here.

Note I detected a few unusued variables when I ran the patches locally, so you might need to re-check just before landing:

mobile/android/components/extensions/ext-browserAction.js
  8:1  error  'Services' is defined but never used. Allowed unused vars must match /^console$/.  no-unused-vars (eslint)

toolkit/modules/subprocess/test/xpcshell/head.js
   5:1  error  'AppConstants' is defined but never used. Allowed unused vars must match /^console$/.  no-unused-vars (eslint)
   7:1  error  'OS' is defined but never used. Allowed unused vars must match /^console$/.            no-unused-vars (eslint)
   9:1  error  'Services' is defined but never used. Allowed unused vars must match /^console$/.      no-unused-vars (eslint)
  11:1  error  'Subprocess' is defined but never used. Allowed unused vars must match /^console$/.    no-unused-vars (eslint)

::: browser/components/migration/tests/unit/head_migration.js:18
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://testing-common/TestUtils.jsm");
>  ChromeUtils.import("resource://testing-common/PlacesTestUtils.jsm");
>  
> +// eslint-disable-next-line no-unused-vars

Just a side note, I'm starting to think we should disable no-ununsed-vars on the global scope for `test/**/head*.js files.`, I don't think it is worth having it there.

If you have any thoughts about that I'd be interested to hear them.

::: browser/extensions/pocket/content/AboutPocket.jsm:15
(Diff revision 1)
>  
>  // See LOG_LEVELS in Console.jsm. Common examples: "All", "Info", "Warn", & "Error".
>  const PREF_LOG_LEVEL = "loop.debug.loglevel";
>  
> +// eslint-disable-next-line no-unused-vars
>  XPCOMUtils.defineLazyGetter(this, "log", () => {

Why the disabling rather than removing? If it is simpler for now, that's fine, but could we get a follow-up bug on this?

This is strange anyway, since it was copied from the old loop (Hello) code and not updated correctly.
Attachment #8970787 - Flags: review?(standard8) → review+
Comment on attachment 8970788 [details]
Bug 1456686: Part 2 - Update ESLint plugin to treat explicit imports as real variable definitions.

https://reviewboard.mozilla.org/r/239548/#review245274

::: commit-message-ec0a4:1
(Diff revision 1)
> +Bug 1456686: Part 2 - Update ESLint plugin to treat explicit imports as real variable definitions. r?standard8

Please can you also bump the plugin version numbers in tools/lint/eslint/eslint-plugin-mozilla/package.json & tools/lint/eslint/eslint-plugin-mozilla/package-lock.json?

It'll help me with publishing this to our external repos, as I suspect they'll want the changes quickly.
Attachment #8970788 - Flags: review?(standard8) → review+
(In reply to Mark Banner (:standard8) from comment #3)
> Note I detected a few unusued variables when I ran the patches locally, so
> you might need to re-check just before landing:

Yeah, sorry about that. When I ran locally, I got 'Environment
key "mozilla/chrome-worker" is unknown' for those files and
didn't notice. I fixed them after my try run finished.

> ::: browser/components/migration/tests/unit/head_migration.js:18
> (Diff revision 1)
> >  ChromeUtils.import("resource://gre/modules/Services.jsm");
> >  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> >  ChromeUtils.import("resource://testing-common/TestUtils.jsm");
> >  ChromeUtils.import("resource://testing-common/PlacesTestUtils.jsm");
> >  
> > +// eslint-disable-next-line no-unused-vars
> 
> Just a side note, I'm starting to think we should disable no-ununsed-vars on
> the global scope for `test/**/head*.js files.`, I don't think it is worth
> having it there.
> 
> If you have any thoughts about that I'd be interested to hear them.

Yeah, I've been thinking that too. But I think it's a separate
bug. I'll file it and add a note that these comments should be
removed

> > +// eslint-disable-next-line no-unused-vars
> >  XPCOMUtils.defineLazyGetter(this, "log", () => {
> 
> Why the disabling rather than removing? If it is simpler for now, that's
> fine, but could we get a follow-up bug on this?
> 
> This is strange anyway, since it was copied from the old loop (Hello) code
> and not updated correctly.

I wasn't sure whether it was there to make it easier for people
to add debug logging when necessary. I'll remove it if you think
that makes sense.
See Also: → 1456939
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5)
> (In reply to Mark Banner (:standard8) from comment #3)
> > ::: browser/components/migration/tests/unit/head_migration.js:18
> > (Diff revision 1)
> > > +// eslint-disable-next-line no-unused-vars
> > 
> > Just a side note, I'm starting to think we should disable no-ununsed-vars on
> > the global scope for `test/**/head*.js files.`, I don't think it is worth
> > having it there.
> > 
> > If you have any thoughts about that I'd be interested to hear them.
> 
> Yeah, I've been thinking that too. But I think it's a separate
> bug. I'll file it and add a note that these comments should be
> removed

Great thanks for that, once this lands, I'll mark it up as a mentored bug.

> > > +// eslint-disable-next-line no-unused-vars
> > >  XPCOMUtils.defineLazyGetter(this, "log", () => {
> > 
> > Why the disabling rather than removing? If it is simpler for now, that's
> > fine, but could we get a follow-up bug on this?
> > 
> > This is strange anyway, since it was copied from the old loop (Hello) code
> > and not updated correctly.
> 
> I wasn't sure whether it was there to make it easier for people
> to add debug logging when necessary. I'll remove it if you think
> that makes sense.

Lets leave that as it is for now, I'll file a follow-up with Pocket to work out what they want to do with it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/95b7f4ae23fdaf6d806ffac8a402e5f043dbd1c5
Bug 1456686: Part 1 - Fix unused and shadowed explicit imports. r=standard8

https://hg.mozilla.org/integration/mozilla-inbound/rev/9800c0f0edc9049f3004bf23e0676d4d467fd8e6
Bug 1456686: Part 2 - Update ESLint plugin to treat explicit imports as real variable definitions. r=standard8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e316258ed9aeeed1257b138dbaa1f03899828b84
Bug 1456686: Follow-up: Remove unused import added after rebase. r=bustage CLOSED TREE
Blocks: 1355202
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.