Figure out if to make Cu.import with single-args discover exported variables (fix import-globals?), or to ban single-arg versions

RESOLVED FIXED in Firefox 52

Status

Firefox Build System
Lint and Formatting
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: standard8, Assigned: mossop)

Tracking

Version 3
mozilla52

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
In bug 1311312, we're trying to get eslint's no-undef rule to be enabled across all of browser/

One of the most obvious problems we're hitting at the moment is:

browser/modules/SocialService.jsm
   386:7    error  'AddonManagerPrivate' is not defined.  no-undef (eslint)
   390:7    error  'AddonManagerPrivate' is not defined.  no-undef (eslint)

browser/tools/mozscreenshots/mozscreenshots/extension/Screenshot.jsm
   65:12  error  'OS' is not defined.  no-undef (eslint)
  120:21  error  'OS' is not defined.  no-undef (eslint)

These files are doing imports such as:

Cu.import("resource://gre/modules/AddonManager.jsm");
Cu.import("resource://gre/modules/osfile.jsm");

Currently our import rules pick these up as `AddonManager` and `osfile` - when it should be picking up `AddonManager`, `AddonManagerPrivate` and `OS`.


Looking around, we need to either:

- ban the single argument Cu.import and be more explicit about what we're importing
- Fix the `import-globals` rule to go into the imported files & check what is actually exported.

The first will require a fairly big change to move away from the single argument version.

The second will need some code written to discover these files (which may need to learn a bit about the resource uris to do so), and then correctly extract the exported versions.

For me, I'm slightly torn as to which is better, though I think if we can fix the `import-globals` rule, then it prevents the case where someone tries to use a symbol that isn't actually exported.

Thoughts & ideas welcome.
We can simply change those entries to:

```
const {OS} = Cu.import("resource://gre/modules/osfile.jsm", {});
const {"AddonManager", "AddonManagerPrivate"} = Cu.import("resource://gre/modules/AddonManager.jsm", {})

```

It seems better practice to import them directly into declared constants. This also means that we only import what we really need.
To be clear, I don't think we should use the single argument version at all.
(Reporter)

Comment 3

2 years ago
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> We can simply change those entries to:
> 
> ```
> const {OS} = Cu.import("resource://gre/modules/osfile.jsm", {});
> const {"AddonManager", "AddonManagerPrivate"} =
> Cu.import("resource://gre/modules/AddonManager.jsm", {})
> 
> ```
> 
> It seems better practice to import them directly into declared constants.
> This also means that we only import what we really need.

I can agree that's better development wise, but from the linting perspective, it potentially hides the case where someone changes an export, or miss-types something, which then has the potential to break code.

Obviously unit tests should pick that up, but there's a lot of cases where our tests aren't covering enough.

We could create some sort of additional new rule I guess, which would allow that to be checked.
I think we should be willing to handle this in steps, and it seems the simplest step right now is to change how we import to not use the single-argument rule.

Once we get the no-undef rule enabled with no errors, then we can focus on making our Cu.import linter better, but for now Cu.import only accounts for a tiny number of variables per file. Therefore I don't think it is as likely to be the source for legitimate bugs with undefined variables.
(Assignee)

Comment 5

2 years ago
(In reply to Mark Banner (:standard8) from comment #0)
> Looking around, we need to either:
> 
> - ban the single argument Cu.import and be more explicit about what we're
> importing

I agree with Jared and Mike that this is the way to go. It doesn't just fix the eslint case but makes other tools like editors more aware of exactly what variables are being defined.

> - Fix the `import-globals` rule to go into the imported files & check what
> is actually exported.

Running eslint on the tree is already a slow process, three minutes on my machine. If we start to do this it will only get slower dropping developer productivity, and that's even ignoring the fact that we need a map for resource URIs and location in tree.

We should think about defining a new rule that verifies that EXPORTED_SYMBOLS contains a list of things that are defined in the jsm. That doesn't cover the cases where you import the wrong things but it is useful nonetheless.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8804072 [details]
Bug 1312355: Automatically convert Cu.import usage to two argument form.

I wrote a short script to automatically change all Cu.import references to the 2 arg form I know one bug it has is always adding all of the exported symbols when only some will be needed. Can you spot any other obvious issues?

Is an automatic rewrite like this a good idea, it touches 904 files which seems like it would be impossible to review as-is.
Attachment #8804072 - Flags: feedback?(standard8)
Attachment #8804072 - Flags: feedback?(mratcliffe)
(Reporter)

Comment 9

2 years ago
Comment on attachment 8804072 [details]
Bug 1312355: Automatically convert Cu.import usage to two argument form.

> Comment on attachment 8804072 [details]
> Bug 1312355: Automatically convert Cu.import usage to two argument form.
> 
> I wrote a short script to automatically change all Cu.import references to
> the 2 arg form I know one bug it has is always adding all of the exported
> symbols when only some will be needed. Can you spot any other obvious issues?

I think for all the symbols being added, we could quite easily reduce those down later with no-unused (bug 1312407), the ones in browser/components/migration are already shown up.

The alternative, is to just add the main one, then scan through no-undef output, but I think as no-undef isn't clean yet that would be harder.

The other issue I see is that toolkit/components/extensions/ and browser/extensions/formautofill/ both use `"object-curly-spacing": [2, "never"],` whereas you're supplying spaces. Personally, I prefer the version with spaces... so not sure what we should do there, unless we can convince them to change.

> Is an automatic rewrite like this a good idea, it touches 904 files which
> seems like it would be impossible to review as-is.

I think its going to be hardish to review anyway. I'd be happier giving it r+ if  we had no-undef / no-unused-vars enabled, as we're more likely to pick up any issues.

Maybe we should delay landing this until after 52 is merged to aurora, and give ourselves a bit more time to finish at least no-undef/no-unused-vars in the 53 cycle?

We could potentially have some sort of hack session at the all-hands to finish those off if we haven't already...
Attachment #8804072 - Flags: feedback?(standard8) → feedback+
(Assignee)

Comment 10

2 years ago
(In reply to Mark Banner (:standard8) from comment #9)
> The other issue I see is that toolkit/components/extensions/ and
> browser/extensions/formautofill/ both use `"object-curly-spacing": [2,
> "never"],` whereas you're supplying spaces. Personally, I prefer the version
> with spaces... so not sure what we should do there, unless we can convince
> them to change.

Looks like that rule is automatically fixable so I can just do that after my script runs I hope.

> > Is an automatic rewrite like this a good idea, it touches 904 files which
> > seems like it would be impossible to review as-is.
> 
> I think its going to be hardish to review anyway. I'd be happier giving it
> r+ if  we had no-undef / no-unused-vars enabled, as we're more likely to
> pick up any issues.
> 
> Maybe we should delay landing this until after 52 is merged to aurora, and
> give ourselves a bit more time to finish at least no-undef/no-unused-vars in
> the 53 cycle?

Seems reasonable.

I think we probably need to start a thread on firefox-dev talking about dropping the single argument version of Cu.import rather than taking it as read that all of us have the arguments necessary to make that decision too.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8804072 [details]
Bug 1312355: Automatically convert Cu.import usage to two argument form.

Ok, after a while tinkering with this I don't think an automatic rewrite is going to be of a lot of use. There are too many cases where the rewrite causes bustage due to how Cu.import is implemented. Some examples:

Cu.import defined variables, not constants. We do overwrite these sometimes so we can't use const declarations.

Some scripts import the same variable multiple times from different modules so defining as let or var fails.

Cu.import imports into the global scope but we have many places where we call it from an inner scope. We can't rewrite those places sensibly.

It would be a pain to do rewrites on xbl scripts.
Attachment #8804072 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
I think the simplest way forward short of manually changing things (not worth the time IMO) would be to generate a list of all the variables that a given jsm provides. I generated one for my script anyway and we can bake that into the eslint plugin so rather than just defining the one variable it defines the correct list of variables.

That will go out of date often but I think we can also use eslint to verify that EXPORTED_SYMBOLS matches what we expect for a given module so folks have to update it on checkin.
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
This implements a fixed lookup table for gathering the imported variables. There are probably some missing entries but this should cover the majority of cases. Surprisingly it doesn't make much of a dent on our no-undef count, fixing just 600 failures out of 21600 total :(
(Reporter)

Comment 16

2 years ago
(In reply to Dave Townsend [:mossop] from comment #15)
> This implements a fixed lookup table for gathering the imported variables.
> There are probably some missing entries but this should cover the majority
> of cases. Surprisingly it doesn't make much of a dent on our no-undef count,
> fixing just 600 failures out of 21600 total :(

I made it about 6000 out of about 51000 on the whole tree. So that's reasonable. Although I do think we're getting to the stage now where most of the modules/xpcshell tests are covered, and we need to look at the content code / mochitests which is basically where I think we need to get them using import-browserjs-globals. Though that's for another bug.
(Reporter)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8805252 [details]
Bug 1312355: Use a fixed lookup table for variables imported from JS modules.

https://reviewboard.mozilla.org/r/89032/#review88398

I think this looks good. Just a couple of queries though.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:35
(Diff revision 1)
>    /^this\.__defineGetter__\("(\w+)"/,
>    /^this\.(\w+) =/
>  ];
>  
>  var imports = [
> -  /^(?:Cu|Components\.utils)\.import\(".*\/(.*?)\.jsm?"(?:, this)?\)/,
> +  /^(?:Cu|Components\.utils)\.import\(".*\/((.*?).jsm?)"(?:, this)?\)/,

Shouldn't the dot before the `jsm` still be escaped?

::: tools/lint/eslint/modules.json:157
(Diff revision 1)
> +  "objects.js": ["getLength"],
> +  "observers.js": ["Observers"],
> +  "offlineAppCache.jsm": ["OfflineAppCacheHelper"],
> +  "OrientationChangeHandler.jsm": [""],
> +  "os.js": ["listDirectory", "getFileForPath", "abspath", "getPlatform"],
> +  "osfile_async_front.jsm": ["OS"],

I think we probably want `"osfile.jsm": ["OS"],` as well.

::: tools/lint/eslint/modules.json:240
(Diff revision 1)
> +  "userapi.js": ["UserAPI10Client"],
> +  "util.js": ["getChromeWindow"],
> +  "util.js": ["XPCOMUtils", "Services", "Utils", "Async", "Svc", "Str"],
> +  "utils.js": ["applicationName", "assert", "Copy", "getBrowserObject", "getChromeWindow", "getWindows", "getWindowByTitle", "getWindowByType", "getWindowId", "getMethodInWindows", "getPreference", "saveDataURL", "setPreference", "sleep", "startTimer", "stopTimer", "takeScreenshot", "unwrapNode", "waitFor"],
> +  "utils.js": ["btoa", "encryptPayload", "isConfiguredWithLegacyIdentity", "ensureLegacyIdentityManager", "setBasicCredentials", "makeIdentityConfig", "makeFxAccountsInternalMock", "configureFxAccountIdentity", "configureIdentity", "SyncTestingInfrastructure", "waitForZeroTimer", "Promise", "add_identity_test", "MockFxaStorageManager", "AccountState", "sumHistogram"],
> +  "utils.js": ["CommonUtils"],

Looking at this in browser/components/translation, this doesn't seem to be detected/included in the globals.
Attachment #8805252 - Flags: review?(standard8)
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8805252 [details]
Bug 1312355: Use a fixed lookup table for variables imported from JS modules.

https://reviewboard.mozilla.org/r/89032/#review88398

> I think we probably want `"osfile.jsm": ["OS"],` as well.

I've added a couple of others too. The script I used to generate this couldn't parse pre-processed files so there are probably still one or two missing, we can just add those in follow-ups.

> Looking at this in browser/components/translation, this doesn't seem to be detected/included in the globals.

I've worked around this for now. There are multiple modules called utils.js so I've combined them all into one entry and done the same in other cases. Ideally we'd use the full url to the module as the lookup key but I haven't worked out how to generate that list automatically yet.
Comment hidden (mozreview-request)
(Reporter)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8805252 [details]
Bug 1312355: Use a fixed lookup table for variables imported from JS modules.

https://reviewboard.mozilla.org/r/89032/#review88850

Thanks for the update. I think that's a good solution for now. I've not checked all the exports are correct, but I'll trust they are. r=Standard8
Attachment #8805252 - Flags: review?(standard8) → review+
(Assignee)

Updated

2 years ago
Assignee: nobody → dtownsend
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8805252 - Attachment is obsolete: true
(Reporter)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8806463 [details]
Bug 1312355: Use a fixed lookup table for variables imported from JS modules.

https://reviewboard.mozilla.org/r/89890/#review89388
Attachment #8806463 - Flags: review?(standard8) → review+

Comment 23

2 years ago
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c88863b64010
Use a fixed lookup table for variables imported from JS modules. r=standard8

Comment 24

2 years ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/8d658d7287ef
Backed out changeset c88863b64010 for eslint bustage. r=backout

Comment 25

2 years ago
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f20aef4c7308
Use a fixed lookup table for variables imported from JS modules. r=standard8

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f20aef4c7308
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

3 months ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.