1445813 - eslint fails on defineLazyScriptGetter with second argument as string

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect, P3)

Description

[Brendan Dahl [:bdahl]]

Over in bug 1442302, I noticed when I had:

XPCOMUtils.defineLazyScriptGetter(window, "PlacesTreeView",
  "chrome://browser/content/places/treeView.js");

eslint failed with:

Cannot read property 'map' of undefined
TypeError: Cannot read property 'map' of undefined
    at Object.convertCallExpressionToGlobals (/Users/bdahl/projects/gecko-hg/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:320:52)
    at GlobalsForNode.ExpressionStatement (/Users/bdahl/projects/gecko-hg/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js:132:25)
    at parser.(anonymous function) (/Users/bdahl/projects/gecko-hg/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js:230:36)
    at listeners.(anonymous function).forEach.listener (/Users/bdahl/projects/gecko-hg/node_modules/eslint/lib/util/safe-emitter.js:47:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/bdahl/projects/gecko-hg/node_modules/eslint/lib/util/safe-emitter.js:47:38)
    at NodeEventGenerator.applySelector (/Users/bdahl/projects/gecko-hg/node_modules/eslint/lib/util/node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (/Users/bdahl/projects/gecko-hg/node_modules/eslint/lib/util/node-event-generator.js:280:22)
    at NodeEventGenerator.enterNode (/Users/bdahl/projects/gecko-hg/node_modules/eslint/lib/util/node-event-generator.js:294:14)
    at CodePathAnalyzer.enterNode (/Users/bdahl/projects/gecko-hg/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
A failure occurred in the eslint linter.
✖ 1 problem (0 errors, 0 warnings, 1 failure)

Changing "PlacesTreeView" to ["PlacesTreeView"] fixed it.

Comment 1

[Mark Banner (:standard8)]

The reason this failed is because our regexps expect "this" rather than "window":

https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js#20-38

It worked with an array, as there's a fallback for defineLazyScriptGetter to handle the array case, but it doesn't check the first parameter:

https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js#337-343

I'm not quite sure what the right thing to do here is. We could probably change the regexps to handle `this` or `window`, but I think covering any value would be wrong. Also, I think this is quite a rare situation, so I'm not sure if it is worth changing it.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1445813 - Allow globalThis as a second parameter to defineLazyScriptGetter and friends. r?mconley!

Comment 3

[Mark Banner (:standard8)]

The attached patch allows globalThis as well as this as a parameter. I think now that globalThis is supported, that is a reasonable thing to do.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/539feb1c3bda
Allow globalThis as a second parameter to defineLazyScriptGetter and friends. r=mconley

Comment 5

[Sandor Molnar[:smolnar]]

Backed out for causing es lint failure

Backout link

Push with failures

Failure log

Comment 6

[Mark Banner (:standard8)]

I'd missed that callExpressionMultiDefinitions wasn't a set of regexps, fixed now.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d92ec21fbb0
Allow globalThis as a second parameter to defineLazyScriptGetter and friends. r=mconley

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4d92ec21fbb0