1344711 - Add an eslint rule to report an error when a get*Pref call is the only instruction in a try block

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Florian Quèze [:florian]]

Now that bug 1338306 landed, we can cleanup the tree.

Comment 1

[Florian Quèze [:florian]]

Attachment: Add an eslint rule

Comment 2

[Florian Quèze [:florian]]

Attachment: xpcshell script

This will handle the most common cases, and try to do as little damage as possible.

It handles:

 let foo = 42;
 try {
   foo = branch.getIntPref("foo");
 } catch(e) {}

and

 try {
   foo = branch.getIntPref("foo");
 } catch(e) {
   foo = 42;
 }

If foo is not an identifier, or if 42 is not a literal or an identifier, it won't change the code.

Comment 3

[Florian Quèze [:florian]]

Attachment: script-generated patch

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8843967 [details] [diff] [review]
script-generated patch

When reviewing, I would suggest paying a bit more attention to getCharPref calls, because the default value can only be a string, so in some cases the rewrite will cause the result to be "" instead of null or undefined before.

When there's a null check on the result right after the getCharPref call, it won't many any difference. In a few cases it's less obvious that the change has no side effect, but I haven't seen anything too scary, and the green try run gives me some confidence.

Comment 5

[Florian Quèze [:florian]]

Attachment: hand cleanup of the script-generated patch

This includes things like:
- reverting the few SpecialPowers.get*Pref calls (the SpecialPowers pref API doesn't support default values)
- simplifying code like: let rv = branch.get*Pref(..., default); return rv;
- restoring meaningful comments that were in catch blocks and got nuked by the script.

I'm attaching this separately for easier review, so that the previous attachment is 100% script-generated, but my intent is to fold this into the previous attachment before landing to minimize the impact on blame.

Comment 6

[Florian Quèze [:florian]]

Attachment: cleanup what the script didn't dare touching

This patch is made by hand, it handles the cases that the script identified but didn't dare touching (due to a non-empty catch block, or due to the default value being more complicated than a litteral or an identifier).

Comment 7

[Florian Quèze [:florian]]

Attachment: cleanup cases reported by eslint

Handle the cases that remain to make the eslint rule happy.

Comment 8

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c70485eba53

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8843964 [details] [diff] [review]
Add an eslint rule

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

::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ +53,5 @@
>      "no-useless-parameters": 0,
>      "no-useless-removeEventListener": 0,
>      "reject-importGlobalProperties": 0,
>      "reject-some-requires": 0,
> +    "use-default-preference-values": 0,

Is there a bug on file already to convert these to string names instead of integers? I think I mentioned this in a previous review for a different bug.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8843967 [details] [diff] [review]
script-generated patch

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

rs=me (skimmed through the changes and looked closely at about 60% of them)

Comment 11

[Florian Quèze [:florian]]

Thanks for the reviews!

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)

> ::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
> @@ +53,5 @@
> >      "no-useless-parameters": 0,
> >      "no-useless-removeEventListener": 0,
> >      "reject-importGlobalProperties": 0,
> >      "reject-some-requires": 0,
> > +    "use-default-preference-values": 0,
> 
> Is there a bug on file already to convert these to string names instead of
> integers? I think I mentioned this in a previous review for a different bug.

I filed bug 1339042 as a result of bug 1338585 comment 4.

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a36e117f41369e0045582d15401b765ad064a6b
Bug 1344711 - Add an eslint rule to report an error when a get*Pref call is the only instruction in a try block, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bff53385b4941566f66332ddb51d339341069f32
Bug 1344711 - script-generated patch to remove try blocks around get*Pref calls, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c92165fef205c73e6ed11dcdd3f70c435ad3853
Bug 1344711 - hand cleanup of cases that were ignored by the script to remove try blocks around get*Pref calls, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/340e7f8a7229786fc8808d4a78983338997f81f3
Bug 1344711 - hand cleanup of remaining useless try blocks around get*Pref calls identified by eslint, r=jaws.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4a36e117f413
https://hg.mozilla.org/mozilla-central/rev/bff53385b494
https://hg.mozilla.org/mozilla-central/rev/9c92165fef20
https://hg.mozilla.org/mozilla-central/rev/340e7f8a7229